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

dcmtk: bump deps #21985

Closed
wants to merge 6 commits into from
+26 −6
Closed

dcmtk: bump deps #21985

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fada8c5
dcmtk: bump deps
mayeut Dec 27, 2023
a5cc508
dcmtk: fix macOS build with host_arch=x86_64 & build_arch=arm64
mayeut Dec 28, 2023
d3e988a
Merge branch 'master' into dcmtk-bump-deps
mayeut Apr 7, 2024
1e3dc38
check rosetta support
mayeut Apr 7, 2024
a6b5141
use version range for libxml2 & libpng
mayeut Apr 18, 2024
858991d
Merge branch 'master' into dcmtk-bump-deps
mayeut Apr 18, 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
Prev Previous commit
Next Next commit
dcmtk: fix macOS build with host_arch=x86_64 & build_arch=arm64 
Requires rosetta & `tools.build.cross_building:can_run=True`
@mayeut
mayeut committed Dec 28, 2023
commit a5cc5081c002e1557cfee8c3f415f51024057278
14 changes: 12 additions & 2 deletions recipes/dcmtk/all/conanfile.py
View file
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@

from conan import ConanFile
from conan.errors import ConanInvalidConfiguration
from conan.tools.build import cross_building
from conan.tools.build import can_run, cross_building
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout
from conan.tools.files import apply_conandata_patches, copy, export_conandata_patches, get, replace_in_file, rmdir, save
from conan.tools.microsoft import is_msvc, is_msvc_static_runtime
@@ -118,12 +118,17 @@ def validate(self):
self.settings.os == "Macos" and self.settings.arch == "armv8":
# FIXME: Probable issue with flags, build includes header 'mmintrin.h'
raise ConanInvalidConfiguration("Cross building to Macos M1 is not supported (yet)")
if hasattr(self, "settings_build") and cross_building(self) and \
self.settings.os == "Macos" and self.settings.arch == "x86_64" and not can_run(self):
raise ConanInvalidConfiguration("Cross building to macOS x86_64 is only supported from macOS arm64 using rosetta with 'tools.build.cross_building:can_run=True'")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the new macOS builders in CI using an arm64 build profile to build for x86_64 host, this recipe will now raise a ConanInvalidConfiguration exception.

Will the config be updated with tools.build.cross_building:can_run=True ?
If not, is there a simple workaround recommended ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@uilianries @danimtb Tagging you as you were the most visibly involved with the M2 upgrade.

Can the M2 machines run both armv8 and x86_64 executables? And even if they can run both, should we still probably try to use the armv8 versions of the executables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And even if they can run both, should we still probably try to use the armv8 versions of the executables?

In this specific instance, there's no armv8 version of the executable as it only there to allow some try_run calls in the CMake file.
Having tools.build.cross_building:can_run=True would also mean that tests could run entirely which is not the case anymore for macOS x86_64 builds.

I could add a specific _can_run property to the recipe in order not to require tools.build.cross_building:can_run=True if rosetta is available.

Copy link
Member

Choose a reason for hiding this comment

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

Can the M2 machines run both armv8 and x86_64 executables? And even if they can run both, should we still probably try to use the armv8 versions of the executables?

Yes, M2 can build and run both amrv8 and x86_64. As M2 can run anything, there is no favorite target when running, when a Mac machine is available, it will run tests or build binaries as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will run tests

it won't run any executable built by the tests and protected with if can_run(self): while it could. That's what I meant by "tests could run entirely which is not the case anymore for macOS x86_64 builds".


def source(self):
get(self, **self.conan_data["sources"][self.version], strip_root=True)

def generate(self):
tc = CMakeToolchain(self)
if cross_building(self) and can_run(self):
tc.variables["CMAKE_CROSSCOMPILING_EMULATOR"] = ""
# DICOM Data Dictionaries are required
tc.variables["CMAKE_INSTALL_DATADIR"] = self._dcm_datadictionary_path.replace("\\", "/")
tc.cache_variables["DCMTK_USE_FIND_PACKAGE"] = True
@@ -193,7 +198,12 @@ def _collect_cmake_required(self, host_dependency):

def _patch_sources(self):
apply_conandata_patches(self)

replace_in_file(
self,
os.path.join(self.source_folder, "CMake", "dcmtkTryRun.cmake"),
"if(CMAKE_CROSSCOMPILING)",
"if(CMAKE_CROSSCOMPILING AND NOT DEFINED CMAKE_CROSSCOMPILING_EMULATOR)",
)
# Workaround for CMakeDeps bug with check_* like functions.
# See https://github.com/conan-io/conan/issues/12012 & https://github.com/conan-io/conan/issues/12180
if self.options.with_openssl: