-
Notifications
You must be signed in to change notification settings - Fork 199
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
Add user additional CMake arugments at the end. #814
Add user additional CMake arugments at the end. #814
Conversation
The additional CMake arguments added by the user in the launch configuration UI are added at the end of the CMake command, such that the user is able to override arguments set by the system.
Hi @betamaxbandit |
Hi @ewaterlander , For example, the following CMake invocation has 2 CMAKE_BUILD_TYPE defines and it is the last, Debug, that CMake uses: cmake -G MinGW Makefiles -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON C:\Users\betamax\cdt-only2\runtime-New_configuration_CDT2\cmake_hw -DCMAKE_BUILD_TYPE=Debug Is this correct? |
Yes, that is correct. The last -DCMAKE_BUILD_TYPE= argument prevails. |
I looked a bit further at the code. Above my change, on line 80, there is also a call CommandDescriptorBuilder.appendCMakeArguments(args, cmakeProperties.getExtraArguments()); cmakeProperties.getExtraArguments() is getting settings that were stored in /.settings/CDT-cmake.yaml In CMakeBuildConfiguration.build() the settings from this file are loaded at line 162: There is no place in the code where extra arguments are stored in this file, so you will always get an empty list. The launch configuration UI stores the additional CMake arguments in the project's preference store, and I think that is the correct place. The method I moved to the end also gets the settings from there. I think the the YAML file can go. It is actually not used and it only makes the code confusing. There is no need for an extra YAML file to store CMake settings. I will make a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review LGTM.
I would urge caution.
So I suggest not removing:
|
If it would work correctly then the line Secondly there is a single YAML file for both Run and Debug. This is wrong. I locally saved the YAML file. The user arguments are not stored correctly in the YAML file. So it is totally broken. I will not remove it, but deprecate it. This is how the YAML file looks like. Instead of arguments you get some identifier.
|
Hi @15knots , |
The only usage of the CMakePropertiesController is creating an empty new CMakeProperties by loading a non-existent YAML file. So my conclusion is that we can deprectate the CMakePropertiesController completely and remove all its usage. I guess this is some old code that was not removed after somebody stored the CMake preferences in the project preferences, as it should be. |
CMakePropertiesController was only used to create a new CMakeProperties. The load() method loads a YAML file .settings/CDT-cmake.yaml which never existed, because the save() method was never called. There was no separate YAML file for Run and Debug. CMake properties are stored in the project preferences by the launch config UI and these properties are used to construct the cmake command line.
I deprecated CMakePropertiesController and removed its usage in this PR. |
I was too fast. This will break your code. I will undo the last commit |
Am Freitag, 7. Juni 2024, 12:27:09 CEST schrieb betamax:
Hi @15knots ,
Can you say what the purpose of the YAML file is please?
I planned to allowto persist option values for the cmake command-line on a
per-projecz level in the yaml file.
But when I found out that some of the cmake options are passed through totally
undocumented properties from a project not being part of CDT (launchbar
plugin) I gave up.
You may safely delete the stuff dealing with that or find a way to combine both
the YAML stettings and the launchbar settings.
Martin
…--
Cd wrttn wtht vwls s mch trsr.
|
Hi @15knots , Please feel free to add comments or extra detail as you see fit. Cheers John |
1c915bc
to
a9fc3ed
Compare
Hi @jonahgraham @betamaxbandit John approved commit 1. Commit 3 reverted commit 2. I think this PR is OK for merging. regards, Erwin |
The additional CMake arguments added by the user in the launch configuration UI are added at the end of the CMake command, such that the user is able to override arguments set by the system.