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

docs: when to use CMakeToolchain's variable choices #13971

Merged

Conversation

prince-chrismc
Copy link
Contributor

Docs!

trying to capture #13794 (comment)

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

SSE4
SSE4 previously approved these changes Nov 2, 2022
@prince-chrismc
Copy link
Contributor Author

Just as a note the policies that are currently being set with cache variables will go away with the next client since there was a fix :)

docs/v2_migration.md Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

1 similar comment
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

Copy link
Contributor

@jwillikers jwillikers left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot
Copy link
Collaborator

This PR has been triggered again because there are existing changes in master branch that are related to these same references resulting in different recipe-revisions. The build is being triggered on the PR context in case some additional fix is needed after merging those changes.

## CMakeToolchain

The old `CMake.definition` should be replaced by `CMakeToolchain.variables` and moved to the `generate` method.
However, certain options need to be passed as `cache_variables`. You'll need to check project's `CMakeLists.txt`
Copy link
Contributor

Choose a reason for hiding this comment

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

should I check just project's root CMakeLists.txt, or all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on the project... I mean maybe if they have multiple layers with different options and multiple projects all being combined yeah you might possibly need to -- but I have not see that thankfully

@SSE4
Copy link
Contributor

SSE4 commented Nov 15, 2022

I am reading cmake doc:

In CMake there are two types of variables: normal variables and cache variables.
Normal variables are meant for the internal use of the script 
(just like variables in most programming languages); 
they are not persisted across CMake runs. 
Cache variables (unless set with INTERNAL) are mostly intended 
for configuration settings where the first CMake run determines 
a suitable default value, which the user can then override, 
by editing the cache with tools such as ccmake or cmake-gui.

so, non-cache variables are considered internal, and are not supposed to be modified from the outside.
given that, maybe it's a good practice to always set variables we use in conanfile as CACHE?
otherwise, it indicates we're trying to modify from the outside something that is not supposed to be modified that way. it may results in a logic errors in CMake script (like, variables may get contradictory values).

@prince-chrismc
Copy link
Contributor Author

prince-chrismc commented Nov 15, 2022

so, non-cache variables are considered internal, and are not supposed to be modified from the outside.
given that, maybe it's a good practice to always set variables we use in conanfile as CACHE?
otherwise, it indicates we're trying to modify from the outside something that is not supposed to be modified that way. it may results in a logic errors in CMake script (like, variables may get contradictory values).

The problem here is the language does not come from the variables but the present. So the meaning and invocation are different https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html#configure-preset

In conan toolchain lingo "cache variables are command line inputs" and "variables are value input before the project is loaded". Both are valid ways but has different magnitudes of importance. This is significant because if a human passes something at the command line - it should take precedence - they should be able to have the final say.

In the most general case, of course.

Hopefully that clears up things.

@SSE4
Copy link
Contributor

SSE4 commented Nov 16, 2022

so, non-cache variables are considered internal, and are not supposed to be modified from the outside.
given that, maybe it's a good practice to always set variables we use in conanfile as CACHE?
otherwise, it indicates we're trying to modify from the outside something that is not supposed to be modified that way. it may results in a logic errors in CMake script (like, variables may get contradictory values).

The problem here is the language does not come from the variables but the present. So the meaning and invocation are different https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html#configure-preset

In conan toolchain lingo "cache variables are command line inputs" and "variables are value input before the project is loaded". Both are valid ways but has different magnitudes of importance. This is significant because if a human passes something at the command line - it should take precedence - they should be able to have the final say.

In the most general case, of course.

Hopefully that clears up things.

maybe it deserves a better explanation in docs then. but we can add it later, if needed.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 9969cbe into conan-io:master Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants