Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[fmt] conan v2 compatibility #10456
[fmt] conan v2 compatibility #10456
Changes from 10 commits
6bfd336
9fe6e23
79bb1f9
3ea2751
9cebf42
2801039
67340bf
53c0cfc
101085b
06f4b6b
7cf7414
092b7a0
85d87a0
3e69682
e2ea162
bd48a19
1d26a93
02f1e94
666768f
f2ae8d0
56c874a
0e28400
dc34ef4
48eecaf
0156611
32eced7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
This should be at the
CMakeToolchain
level, that allows local user builds to be consistent with optionsThere 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 think this can't be changed until c3i uses Conan 1.50 and then must be translated to https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#cache-variables
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.
CCI uses 1.49 ATM
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 am curious about the other variable mechanics https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#variables
If the toolchain sets the value before the options are declared then they do nothing according to https://cmake.org/cmake/help/latest/command/option.html
If this is set to use 1.49 then I think we could get away for using just
tc.variables
however I agree the cache ones are better UX this seems to be working for my projectThere 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.
there was a bug using
tc.variables
had a zero effect. it causes CI to build more things than needed (e.g. it build tests that were intended to be disabled viaFMT_TEST
).it turns out to be that diff:
https://github.com/fmtlib/fmt/blob/f94b7364b9409f05207c3af3fa4666730e11a854/CMakeLists.txt#L41-L49
https://github.com/fmtlib/fmt/blob/b6f4ceaed0a0a24ccf575fab6c56dd50ccf6f1a9/CMakeLists.txt#L66-L81
in general it harmless in this particular case (just causes longer builds), but in other cases it can produce a wrong package if some option is ignored.
more info at conan-io/conan#11476
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.
Ahhh, there will need to be a hook so people dont use the other one in CCI 🙈
The options before project is an obvious limitation + it's easy to imagine the separate steps for the c3i could be problematic
Thanks for filling in the details
This file was deleted.