Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug579242: allow user to override CMake Settings #683

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

betamaxbandit
Copy link
Contributor

The Launch Bar Launch Configuration, Build Settings tab allows the user to customise the CMake Settings (CMake generator, extra arguments, build command and clean command). But changing these settings did not affect the CMake build. This is now fixed.
A "Use these settings" checkbox allows the user to choose settings from the UI or use the operating system defaults.

Fixes bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=579242

@betamaxbandit
Copy link
Contributor Author

betamaxbandit commented Jan 29, 2024

The updated UI, containing the "Use these settings" checkbox, looks like this:

Build Settings

Aside: the "Build command" and "Clean command", highlighted above in red, are probably not relevant to this UI. It looks like they may have been a hang-over from before this was a CMake specific page.
I have hooked them up in this patch, so they do work, but should probably be removed in a subsequent ticket.

@betamaxbandit
Copy link
Contributor Author

I have tested this change as described in the following file:
Bug579242_manual_tests.md
I'm not sure whether to add this file to the repo. If yes, then can someone suggest a suitable path please?
How about?
cdt/cmake/org.eclipse.cdt.cmake.ui.tests/manual_tests/

@betamaxbandit
Copy link
Contributor Author

@Kummallinen

@betamaxbandit
Copy link
Contributor Author

@Kummallinen @jonahgraham

I don't understand why the code cleaniness check is failing. This is a snippet.

diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java
index 7ea[42](https://github.com/eclipse-cdt/cdt/actions/runs/7699868526/job/20982441590#step:7:43)b7cae..ff8c6145aa 1006[44](https://github.com/eclipse-cdt/cdt/actions/runs/7699868526/job/20982441590#step:7:45)
--- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java
+++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/internal/CMakeBuildConfiguration.java
@@ -596,8 +596,7 @@ public class CMakeBuildConfiguration extends CBuildConfiguration {
 		private IOsOverrides getUiOrOsOverrides(IOsOverrides osOverrides) {
 			IOsOverrides retVal = Objects.requireNonNull(osOverrides, "osOverrides must not be null"); //$NON-NLS-1$
 			Preferences settings = getSettings();
-			boolean useUiOverrides = Boolean
-					.valueOf(settings.get(CMAKE_USE_UI_OVERRIDES, Boolean.FALSE.toString()));
+			boolean useUiOverrides = Boolean.valueOf(settings.get(CMAKE_USE_UI_OVERRIDES, Boolean.FALSE.toString()));
 			if (useUiOverrides) {
 				// Set UI override for generator
 				String gen = settings.get(CMAKE_GENERATOR, ""); //$NON-NLS-1$
Tree is dirty - something needs to be cleaned up in your commit (see above for git status/diff). The 'something'
is likely a misformatted file, extra whitespace at end of line, or something similar. The diff above
shows what changes you need to apply to your patch to get it past the code cleanliness check.
Error: Process completed with exit code 1.

@Kummallinen
Copy link
Contributor

@Kummallinen @jonahgraham

I don't understand why the code cleaniness check is failing. This is a snippet.

Have you run the code formatter on the file in PDE? The error shows a mismatch between format as committed and the formatting rules applied when doing a format in PDE.

Could however be a PDE version mismatch

@jonahgraham
Copy link
Member

@betamaxbandit try to format the file - the automatic formatter gets things wrong when refactor -> rename is run as it doesn't respect line lengths if the renamed symbol becomes longer.

As @Kummallinen says I suspect its actually a version mismatch. JDT occasionally improves code formatter leading to different results. We are using a couple of years old JDT on the build machine to verify formatting. If you can submit with the line manually done like the code cleanliness reports that would be great, either way I'll look at upgrading the version of JDT we are using.

We are using:

curl -sL https://download.eclipse.org/eclipse/downloads/drops4/R-4.23-202203080310/eclipse-SDK-4.23-linux-gtk-x86_64.tar.gz | tar xz

Can you tell me which version of Eclipse IDE you are using (e.g 2023-12?)

@jonahgraham
Copy link
Member

I don't think the JDT version difference is the problem. That file, if formatted with either version of Eclipse gives a different format than your commit has.

Therefore, please reformat the file and update your commit.

@betamaxbandit betamaxbandit force-pushed the Bug_579242 branch 3 times, most recently from ea68c6d to 250c3e5 Compare January 30, 2024 09:28
@betamaxbandit betamaxbandit marked this pull request as ready for review January 30, 2024 10:01
@betamaxbandit
Copy link
Contributor Author

@jonahgraham
TODO:
Where to place manual test document? See previous comment.
Create an "issue" for removal of "Build command" and "Clean command"
Update New and Noteworthy?

The Launch Bar Launch Configuration Build Settings tab has been updated so it can now correctly control the CMake Generator setting. The "Additional CMake arguments" field can also be used to inject CMake defines into the CMakeCache.txt file to populate it with customizable settings for the project.

Build Settings - Use these settings unchecked. The UI settings are not used in the CMake build.
Build Settings - Use these settings unchecked

Build Settings - Use these settings checked. The UI settings are used in the CMake build.
Build Settings - Use these settings checked

@betamaxbandit
Copy link
Contributor Author

Hi @jonahgraham , @Kummallinen ,
Do you have any comments please?

@jonahgraham
Copy link
Member

@jonahgraham TODO: Where to place manual test document?

Add it to https://github.com/eclipse-cdt/cdt/blob/main/TESTING.md (as a link or directly as content)

Create an "issue" for removal of "Build command" and "Clean command"

+1

Update New and Noteworthy?

+1

Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me - just one style issue about how settings are used.

@betamaxbandit
Copy link
Contributor Author

betamaxbandit commented Feb 5, 2024

@jonahgraham TODO: Where to place manual test document?

Add it to https://github.com/eclipse-cdt/cdt/blob/main/TESTING.md (as a link or directly as content)

DONE. I hope I have linked this correctly.

Create an "issue" for removal of "Build command" and "Clean command"

+1

DONE
#691

Update New and Noteworthy?

+1

DONE.

TESTING.md Outdated Show resolved Hide resolved
The Launch Bar Launch Configuration, Build Settings tab allows the user
to customise the CMake Settings (CMake generator, extra arguments, build
command and clean command). But changing these settings did not affect
the CMake build. This is now fixed.
A "Use these settings" checkbox allows the user to choose settings from
the UI or use the operating system defaults.
@jonahgraham jonahgraham merged commit b7fa359 into eclipse-cdt:main Feb 7, 2024
5 checks passed
@jonahgraham
Copy link
Member

Thank you @betamaxbandit

@betamaxbandit betamaxbandit deleted the Bug_579242 branch February 7, 2024 19:18
jonahgraham added a commit to jonahgraham/cdt that referenced this pull request Feb 8, 2024
jonahgraham added a commit that referenced this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants