-
Notifications
You must be signed in to change notification settings - Fork 988
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
Allow tools.gnu:make_program
to work in every case
#14223
Allow tools.gnu:make_program
to work in every case
#14223
Conversation
tools.gnu:make_program
to work in every case
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.
Looks good, if it doesn't break anything, I think we can add an integration test to cover it and it could be merged.
I didn't initially add one becase the test that is already there looked fine, but after double checking, it does not hurt to add an extra check! Good catch :) |
By definition, the test is not there, because the necessary test is one that will not be using mingw. It should be a red-green test, failing without the fix, passing with the fix. |
default=cache_variables.get("CMAKE_MAKE_PROGRAM")) | ||
if cmake_make_program: | ||
cmake_make_program = cmake_make_program.replace("\\", "/") | ||
cache_variables["CMAKE_MAKE_PROGRAM"] = cmake_make_program |
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.
I know this was already there, and not strictly related to this PR. But why is it a cache variable (presets) and not a toolchain one? Because it means that this is an extra things that users not using presets should remember to pass in the command line too. It would be worth checking if this works in the toolchain (probably will fail, and this is the reason it is a cache variable)
Changelog: Fix: Allow
tools.gnu:make_program
to affect every CMake configuration.Docs: Omit
The PR that implemented it was #10770 and already had a check for only mingw, but sadly no insight into why it was gated off of the rest of configurations. After checking a bit and not seeing anything that might be wrong with allowing it, I'm proposing to allow it in every configuration
Closes #14197