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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+27 −1
Open

vc: add v1.4.5 #22064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fde8a12
vc: add v1.4.4
valgur Dec 29, 2023
c4a5183
vc: replace automatic CPU feature handling with options
valgur May 26, 2024
e81b66a
vc: cpu_architecture option
valgur May 26, 2024
3321ba9
vc: bump to v1.4.5
valgur Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions recipes/vc/all/conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
sources:
"1.4.5":
url: "https://github.com/VcDevel/Vc/archive/1.4.5.tar.gz"
sha256: "eb734ef4827933fcd67d4c74aef54211b841c350a867c681c73003eb6d511a48"
"1.4.3":
url: "https://github.com/VcDevel/Vc/archive/1.4.3.tar.gz"
sha256: "988ea0053f3fbf17544ca776a2749c097b3139089408b0286fa4e9e8513e037f"
Expand Down
23 changes: 22 additions & 1 deletion recipes/vc/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from conan import ConanFile
from conan.tools.apple import is_apple_os
from conan.tools.build import check_min_cppstd
from conan.tools.cmake import CMake, CMakeToolchain, cmake_layout
from conan.tools.files import copy, get, replace_in_file, rmdir
Expand All @@ -18,14 +19,31 @@ class VcConan(ConanFile):
settings = "os", "arch", "compiler", "build_type"
options = {
"fPIC": [True, False],
# https://github.com/VcDevel/Vc/blob/1.4.4/cmake/OptimizeForArchitecture.cmake
"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"
],
Comment on lines +23 to +32
Copy link
Member

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

Copy link
Contributor Author

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.

}
default_options = {
"fPIC": True,
"cpu_architecture": "sandy-bridge", # sse sse2 sse3 ssse3 sse4.1 sse4.2 avx
}

def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
if is_apple_os(self):
# sse sse2 sse3 ssse3 sse4.1 sse4.2, no avx
self.options.cpu_architecture = "westmere"
if self.settings.arch not in ["x86", "x86_64"]:
del self.options.cpu_architecture

def layout(self):
cmake_layout(self, src_folder="src")
Expand All @@ -39,11 +57,14 @@ def source(self):

def generate(self):
tc = CMakeToolchain(self)
# https://github.com/openMVG/openMVG/blob/v2.1/src/cmakeFindModules/OptimizeForArchitecture.cmake
tc.variables["TARGET_ARCHITECTURE"] = self.options.get_safe("cpu_architecture", "none")
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW"
Comment on lines +61 to +62
Copy link
Contributor

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

Suggested change
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")

Copy link
Contributor Author

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.

Copy link
Contributor

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.

tc.generate()

def _patch_sources(self):
cmakelists = os.path.join(self.source_folder, "CMakeLists.txt")
replace_in_file(self, cmakelists, "AddCompilerFlag(\"-fPIC\" CXX_FLAGS libvc_compile_flags)", "")
replace_in_file(self, cmakelists, 'AddCompilerFlag("-fPIC" CXX_FLAGS libvc_compile_flags)', "")

def build(self):
self._patch_sources()
Expand Down
2 changes: 2 additions & 0 deletions recipes/vc/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
versions:
"1.4.5":
folder: all
"1.4.3":
folder: all
"1.4.2":
Expand Down
Loading