Improve CMake build option handling and API

[betamaxbandit]

Summary:

• Add some new API to make it easier for ISVs to provide defaults.
• Fully connect UI elements to CMake build process
• Add some missing UI elements (such as customizing generator)
• CMake generator default within CDT changed to Ninja

Details:

Add API to set CMake generator default (eg Ninja) ISV can set their desired CMake generator by overriding
CMakeBuildConfiguration.getDefaultProperties. ISVs can also further fine tune the build process by overriding
CMakeBuildConfiguration.getDefaultProperties

Remove API IOsOverrides and related code. IOsOverrides was a partial implementation to achieve builds in Docker containers, however the work was not complete and it the extra code was complicating some basic use cases of setting defaults

Add support for all generators to CMake build settings UI page by using a Combo instead of radio buttons. The non-deprecated generators that are built-in to CDT populate the Combo, but additional generators can be manually entered in the Combo.

Rename clean command to clean target to better reflect its use as the argument passed to cmake's --target command line.

Add all target for the argument passed to cmake's --target command line when doing a normal build.

Clarify usage of UI overrides and change the UI to be "use defaults" (i.e. invert the checkbox). This is a breaking change as it means user projects that were using UI overrides will revert to using defaults. This is done on purpose as so many little things have changed in CMake settings, that reverting to defaults on upgrade seems like a logical decision. In addition use defaults matches the other GUIs in Eclipse, for example the MBS build command settings.

Populate all defaults in getDefaultProperties() so that all CMake build settings are displayed as used (greyed out) and can be used as a starting point when editing settings.

Simplify some of the code in CMakeBuildTab.

Fix parsing of extra args so that quoted strings work.

Refactored manual tests document and brought it up to date.

Correct command line option for CMake's --warn-unused-vars

Correct command line option for CMake's --warn-uninitialized

Overall this is an API breaking change and the CHANGELOG-API.md has been updated with all the API changes in and around ICMakeProperties, including fixing typos in WarnUninitialized methods.

Fixes #1055
Fixes #818
Part of #1000

[betamaxbandit]

Hi @jonahgraham ,
I've created a draft PR so we can iron out any issues before releasing to wider community.

WRT our discussion on the yaml file yesterday, as far as I can tell the org.eclipse.cdt.cmake.core.internal.CMakePropertiesController.save(ICMakeProperties) method is never called by CDT code, which means the yaml file will not exist for regular (non-ISV extended) CDT products.
Let me know if you think I should still consider the yaml file questions for an ISV modified CDT product.

You'll see that I've added an API for setting the cmake generator.
I've also used that API in the CMakeBuildConfiguration constructors and added logic so it's only done the first time. Maybe there is a better way of doing things just the first time. Anyway it means it's easy for vanilla CDT to use this mechanism and set the generator and have those values used in the UI (CMakeBuildTab.

[github-actions]

Test Results

   600 files   -    35     600 suites   - 35   13m 25s ⏱️ - 22m 25s
10 206 tests  - 1 211  10 183 ✅  - 1 091  23 💤  - 120  0 ❌ ±0 
10 244 runs   - 1 119  10 221 ✅  - 1 001  23 💤  - 118  0 ❌ ±0 

Results for commit a023407. ± Comparison against base commit 11dfaa4.

This pull request removes 1220 and adds 9 tests. Note that renamed tests count towards both.

org.eclipse.cdt.cmake.core.internal.CMakePropertiesControllerTest ‑ testLoad
org.eclipse.cdt.cmake.core.internal.CMakePropertiesControllerTest ‑ testSaveLoad
org.eclipse.cdt.cmake.core.internal.CMakePropertiesEvolutionTest ‑ testSaveLoadEvolution_1
org.eclipse.cdt.debug.gdbjtag.core.tests.jtagdevice.GDBJtagDeviceContributionTest ‑ testGdbJtagDeviceContribution
org.eclipse.cdt.debug.gdbjtag.core.tests.launch.GDBJtagLaunchTest ‑ testGdbJtagLaunch[gdb]
org.eclipse.cdt.debug.gdbjtag.core.tests.launch.GDBJtagLaunchTest ‑ testGdbJtagLaunch[gdbserver]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithQuotes[gdb]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithQuotes[gdbserver]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithSpecialSymbols[gdb]
org.eclipse.cdt.tests.dsf.gdb.tests.CommandLineArgsTest ‑ testSettingArgumentsWithSpecialSymbols[gdbserver]
…

org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ customCMakeGeneratorEntryAuto
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ customCMakeGeneratorEntryManual
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ extraArgumentsParseCorrectly
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ getCMakePropertiesTestSetExtraArguments
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ getCMakePropertiesTestSetGenerator
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ getDefaultProperties
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ getDefaultPropertiesTestExtraArgs
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ targetsAndCommand
org.eclipse.cdt.cmake.core.CMakeBuildConfigurationTests ‑ targetsAndCommandDefaults

♻️ This comment has been updated with latest results.

[jonahgraham]

WRT our discussion on the yaml file yesterday, as far as I can tell the org.eclipse.cdt.cmake.core.internal.CMakePropertiesController.save(ICMakeProperties) method is never called by CDT code, which means the yaml file will not exist for regular (non-ISV extended) CDT products.
Let me know if you think I should still consider the yaml file questions for an ISV modified CDT product.

I don't think you need to consider the yaml file - thank you for pointing out that save was never called. I guess this was incomplete work once upon a time.

I guess that you will start calling save now though (in a subsequent commit)? Perhaps after migrating from yaml to something else?

[betamaxbandit]

Hi @jonahgraham ,
thanks for taking the time to review this.

I like all your suggestions here and think the structure is better the way you have suggested.

However, it removes a key requirement. Each time the build is performed with this patch, it uses the default values. If the user changes the generator value in the UI, that change is ignored. Hence the need for the CMAKE_PROPERTIES_INITIALSED.

I'm struggling to come up with a better way of doing things at the moment, so if you have any ideas I'd be grateful of them please.

[betamaxbandit]

Hi @jonahgraham , thanks for taking the time to review this.

I like all your suggestions here and think the structure is better the way you have suggested.

However, it removes a key requirement. Each time the build is performed with this patch, it uses the default values. If the user changes the generator value in the UI, that change is ignored. Hence the need for the CMAKE_PROPERTIES_INITIALSED.

I'm struggling to come up with a better way of doing things at the moment, so if you have any ideas I'd be grateful of them please.

Maybe using CMakePropertiesController.save(ICMakeProperties) will help with the issue I mentioned in my previous comment about CMAKE_PROPERTIES_INITIALSED. But as well as saving the props, I'd need to do some other stuff in the load, like:

setProperty(CMAKE_GENERATOR, getPlatformOsOverrides(properties).getGenerator().getCMakeName());

[jonahgraham]

@betamaxbandit I think if the load/save are matched then the problem is resolved - right now it seems a bit mismatched between the incomplete yaml and the launch config properties. Can we get it down to only one place that user information is stored?

[betamaxbandit]

Hi @jonahgraham ,

I have implemented all your suggestions AFAIK.

These are some notes about the status of this PS. Sorry it's taking so long to do this, but I'm facing a number of issues with this. I wonder if it would be easier to split into smaller tickets?

0. Current block
   Getting the correct defaults when the user opens the UI.
   After creating a LC with run launch mode, when then switching to debug mode, the correct defaults are not used.
   Can this be done as a separate change?

1. LC names do not include launch mode
   CoreBuildLocalLaunchConfigProvider does not create different launch configurations based on the run mode. When debug launch mode is selected and the LC is opened, I'd expect it to create a LC with debug appended to the name. And similarly, when run launch mode is selected and the LC is opened, I'd expect it to create a LC with run appended to the name.
   However, the cbuildconfig does append the launch mode to it's name, so there can be 2 different cbuildconfig (debug and run), but only a single LC.

Q) Should run and debug launch configurations share the same or different names?

Q) Why does the CMakeBuildTab use properties and not LC attrs to save values?

2. YAML file.
   Do we need this? Adding in a save will generate it and complicate things as they are.
   Having the yaml file is an extra feature so should be done in a different ticket (eg add portability or similar).

[jonahgraham]

@betamaxbandit let's have a chat about this in a call and then we'll record any outcomes here.

[ewaterlander]

Hi @jonahgraham and @betamaxbandit ,

This issue was raised first in the discussion in #814

Then my conclusion was that CMakePropertiesController could be deprecated, but I restored it, because it would break usage by betamaxbandit.

The Core Build System Build Settings tab stores values in the preferences and not in the Launch Configuration, because then it can store different CMake arguments for Run and Debug. In Managed Build projects you can also set different Make arguments for the Run and the Debug build configuration. I think this is a good feature.

When you change launch mode you keep seeing the same launch configuration. This is always. The tabs change, but it is all stored in the same .launch file.

CMakePropertiesController made no difference for launch mode.

[ewaterlander]

Q) Should run and debug launch configurations share the same or different names?

Q) Why does the CMakeBuildTab use properties and not LC attrs to save values?

2. YAML file.
   Do we need this? Adding in a save will generate it and complicate things as they are.
   Having the yaml file is an extra feature so should be done in a different ticket (eg add portability or similar).

CBS projects still have separate build configurations for Run and Debug like MBS projects, only they are hidden for the user. Instead of explicitly setting an active build configuration in MBS projects, the build configuration is implicitly selected by changing the launch mode in CBS projects.

Build configurations store their settings in the preferences. The methods are there. I see no reason to do it differently.
Like I said in my previous comment, changing launch mode is only a different view on the same launch configuration.

CMake settings are build settings, not launch settings. I think we should not make separate launch configurations for different build settings. Same is true for Make settings in the CBS Makefile projects.

We don't need the YAML file. We can use the preferences. The infrastructure is there.

[betamaxbandit]

Hi @ewaterlander ,
Thanks for your comments.

This issue was raised first in the discussion in #814

I'll update that when I complete this change.

I'm not planning changing the way the build settings are stored. I was just thinking aloud.

I plan to deprecate CMakePropertiesController.

Cheers John

[betamaxbandit]

Hi @jonahgraham ,
I've updated my draft PR. I need to add the JUnit test and do further testing tomorrow, but I think it is nearly there now. Hope you agree?
Cheers John

[jonahgraham]

@betamaxbandit , @Kummallinen and I had a meeting/review on this today. In short this is progressing in the correct direction. Along with the review, a part of the outcome of this review is we decided having a fully fleshed out example of an ISV CMake customized project is a useful to both demonstrate for other ISVs and to ensure that the API we create has a workable use case. I have provided that in #1052

[jonahgraham]

The TODO in the example project needs to be completed as well. I have merged the required commits.

TODO:

cdt/cmake/org.eclipse.cdt.cmake.example/src/org/eclipse/cdt/cmake/example/ExtendedCMakeBuildConfiguration.java

Line 37 in 5e62200

// TODO: Here goes the example extending that is being developed in #1046 https://github.com/eclipse-cdt/cdt/pull/1046

[jonahgraham]

@jonahgraham jonahgraham force-pushed the IDE-82683-REQ-012 branch from 6a77bf8 to afb8c41
now

^^^ was rebase onto main.

[jonahgraham]

As discussed on a call today between @betamaxbandit, @Kummallinen and myself we have decided to remove the incomplete work related to IOsOverrides. This code was worked on to support use cases like building in docker containers and automatically switching cmake commands, but that work was never completed. For now that extra API is a lot of overhead in complication.

Therefore I have removed it as a new commit on this PR, it will be squashed later, but I have kept it as an independent commit so that @betamaxbandit can review what I have done. @betamaxbandit have a look at 1218066 (pull link) when you get a chance.

[jonahgraham]

As discussed on a call today between @betamaxbandit, @Kummallinen and myself I added some additional refactoring which I have again left as an extra commit (link to commit in pull request).

Please see the commit message for a summary of all the changes.

@betamaxbandit can you have a look. If this is all ok then I will squash the commits and update the description as this has grown to more than just the API change, but now includes the user visible changes that naturally flow out of the API changes.

[jonahgraham]

There have been many discussions about how to handle CMake preferences. I cross reference some of those key points here. AFAICT this PR now resolves all these questions in the generally agreed upon way.

From #1046 (comment)

We don't need the YAML file. We can use the preferences. The infrastructure is there.

From #814 (comment)

You may safely delete the stuff dealing with that or find a way to combine both the YAML stettings and the launchbar settings.

From #818 (comment)

A) Will removing the YAML file have any impact on the cmake4eclipse (https://github.com/15knots/cmake4eclipse) integration in CDT?

No, it won't (from the author of cmake4eclipse)

From #818 (comment)

I also vote for the preferences approach (option 1).

[jonahgraham]

@betamaxbandit I have done what I hope is the final cleanup and squash on this work.

[jonahgraham]

PS I updated the title and description in #1046 (comment) of the PR to reflect where the PR ended up.

[betamaxbandit]

LGTM.
I like the move of constants into ICMakeBuildConfiguration interface

I tried your latest commit (a02340) on Windows 10 and all looks good.