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

test toolchain configs #7160

Merged
merged 7 commits into from Jun 30, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Jun 8, 2020

Changelog: Fix: Define user variables in the conan_toolchain.cmake file, not in the project-include file.
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@memsharded memsharded added this to the 1.27 milestone Jun 8, 2020
@memsharded
Copy link
Member Author

@memsharded memsharded commented Jun 8, 2020

@jgsogo, please check this when possible, it seems that multi-config cmake vars can also work in the toolchain file, no need to put them in the conan_project_include.cmake file?

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jun 9, 2020

IIRC the problem with this approach is that those variables cannot be stored in the CMake cache (you cannot store and read the generator expression). This shouldn't be a problem as long as you run configure+generate together because you are not using the cache, but if you need to generate again it would be broken (is this what happens when CMake detects a file has been modified and generates again the project before compiling?)

I'm 90% sure about it.

@memsharded
Copy link
Member Author

@memsharded memsharded commented Jun 9, 2020

IIRC the problem with this approach is that those variables cannot be stored in the CMake cache (you cannot store and read the generator expression). This shouldn't be a problem as long as you run configure+generate together because you are not using the cache, but if you need to generate again it would be broken (is this what happens when CMake detects a file has been modified and generates again the project before compiling?)

Good point, I'll add that case to the tests

@memsharded memsharded requested a review from jgsogo Jun 9, 2020
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jun 29, 2020

I'm playing with this PR and so far it works because CMake is adding CMAKE_TOOLCHAIN_FILE to the cache and the file is parsed/used each time CMake runs... which lead me to the conclusion that we don't need to store in the cache (CACHE FORCE) any of the variables we are defining in the toolchain, we will have a cleaner cache and no one will see those variables in the cache and be tempted to modify them.

So I will probably delay this to 1.28, we can get rid of everything we are writing to the cache... and check what happens with MD/MT substitution if we move them to the toolchain too (does it fail? do we have a confirmation?)

@memsharded
Copy link
Member Author

@memsharded memsharded commented Jun 29, 2020

So I will probably delay this to 1.28, we can get rid of everything we are writing to the cache... and check what happens with MD/MT substitution if we move them to the toolchain too (does it fail? do we have a confirmation?)

Precisely for this, I would merge this now, as the toolchain is experimental, and we could have earlier feedback if this works or not for more use cases.

@memsharded memsharded marked this pull request as ready for review Jun 30, 2020
@memsharded memsharded mentioned this pull request Jun 30, 2020
jgsogo
jgsogo approved these changes Jun 30, 2020
Copy link
Member

@jgsogo jgsogo left a comment

pa'dentro, let's see if something happens

@memsharded memsharded merged commit 023b881 into conan-io:develop Jun 30, 2020
2 checks passed
@memsharded memsharded deleted the feature/toolchain_defs_config branch Jul 28, 2020
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.

None yet

2 participants