-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
vc: add v1.4.5 #22064
base: master
Are you sure you want to change the base?
vc: add v1.4.5 #22064
Conversation
This comment has been minimized.
This comment has been minimized.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hi @valgur, there is a new release! https://github.com/VcDevel/Vc/releases/tag/1.4.5 |
Conan v1 pipeline ✔️All green in build 4 (
Conan v2 pipeline ✔️
All green in build 4 ( |
"cpu_architecture": [ | ||
"auto", "generic", "none", "x86-64", "x86-64-v2", "x86-64-v3", "x86-64-v4", | ||
# Intel | ||
"core", "merom", "penryn", "nehalem", "westmere", "sandy-bridge", "ivy-bridge", "haswell", | ||
"broadwell", "skylake", "skylake-xeon", "kaby-lake", "cannonlake", "silvermont", "goldmont", | ||
"knl", "atom", | ||
# AMD | ||
"k8", "k8-sse3", "barcelona", "istanbul", "magny-cours", "bulldozer", "interlagos", | ||
"piledriver", "AMD 14h", "AMD 16h", "zen", "zen3" | ||
], |
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.
Could this not be detected from the host profile? Wouldn't that make a bit more sense? It might not be possible but just thinking out loud
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.
Hmm, that's a very good point. Not with the current settings, but this could well be a separate arch.microarchitecture
or something similar. It could even set the appropriate -march=...
automatically, probably.
Many projects provide an option to set -march=native
, but this always gets disabled by package managers to build for the lowest common denominator. This is a place where the precise host profile capabilities of Conan could really shine.
Also, duplicating these options across packages is not ideal either. I applied the exact same logic in the OpenMVG recipe as well: #23836.
tc.variables["TARGET_ARCHITECTURE"] = self.options.get_safe("cpu_architecture", "none") | ||
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW" |
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.
Modifying tc.variables
directly is only recommended for very special cases, prefer modifying directly cache_variables
, removing the need of CMAKE_POLICY_DEFAULT_CMP0077
usage
tc.variables["TARGET_ARCHITECTURE"] = self.options.get_safe("cpu_architecture", "none") | |
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW" | |
tc.cache_variables["TARGET_ARCHITECTURE"] = self.options.get_safe("cpu_architecture", "none") |
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've seen discussions on whether variables
/cache_variables
on the CCI and Conan client repo before and they have not favored either too strongly, afaik. Has there been an offline discussion and a decision made regarding this? I would not mind using cache_variables
, but the switch would be applicable to nearly all other open PRs as well.
I've personally preferred variables
as using the values via a toolchain file is more flexible and convenient than providing them via a CMake preset. Not that it matters as much in CCI recipes, though.
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.
Using cache variables as a default approach is what we recommend in the documentation since Nov 2022: https://docs.conan.io/2/reference/tools/cmake/cmaketoolchain.html#variables
tc.variables
should be reserved for things that "belong in a toolchain file" - for that, I would refer users to CMake's documentation as to which variables belong in a CMake toolchain file. The recommendation we are following also comes from Craig Scott's book (https://crascit.com/professional-cmake/) - where it is recommended for toolchain files to define as few variables as possible as a general rule.
There was never an internal discussion - this was the conclusion of this issue reported here: conan-io/conan#11937 - which is detailed and discussed in the open. The conclusion was that unless it's a variable CMake expects to be defined in a toolchain file, passing it as a cache_variable
is less likely to cause any issues.
We hope that the conclusion of that issue and the Conan documentation which are clear in this regard, are enough to clarify this - but if could be expressed better, please let us know.
As for other recipes having it - a lot of recipes used tc.variables
before there was a clarification - in a lot of cases it works without issues too, and it's not particularly worth the time changing them unless they do cause issues.
For open PRs, we'll keep an eye out and ensure consistency - what caught our eye in this case was having to enable a policy alongside the tc.variables
definition.
Also replaced automatic CPU SIMD feature handling with an explicit architecture option to fix incompatibility with Apple Silicon x86.