-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
itk: simplify patching, restore DCMTK support #21651
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
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'm picking this one up now @valgur thanks a lot for the links in the custom cmake, they are quite useful!
Do you have any extra insight that might be useful when trying to add the new 5.4 version in a follow-up PR? Thanks!
@AbrilRBS It has been a while and I don't remember a single thing about this PR or recipe anymore, sorry. I'll see if I can get this PR to pass at least, though. |
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.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Failure in build 5 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Failure in build 5 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
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'm not really sure about this "simplification". Skipping ITK CMake files requires not double check, but triple check due its complexity and what are are missing. Even vcpkg is not doing, but keeping the original files.
# Unvendor ZLIB | ||
for path in [ | ||
os.path.join(self.source_folder, "Modules", "IO", "GIPL", "src", "itkGiplImageIO.cxx"), | ||
os.path.join(self.source_folder, "Modules", "IO", "PhilipsREC", "src", "itkPhilipsRECImageIO.cxx"), | ||
os.path.join(self.source_folder, "Modules", "Nonunit", "Review", "src", "itkVoxBoCUBImageIO.cxx"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "GDCM", "src", "gdcm", "Utilities", "gdcm_zlib.h"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "GIFTI", "src", "gifticlib", "gifti_io.h"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "NIFTI", "src", "nifti", "znzlib", "znzlib.h"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "MetaIO", "src", "MetaIO", "src", "localMetaConfiguration.h"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "MINC", "src", "libminc", "nifti", "znzlib.h"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "NrrdIO", "src", "NrrdIO", "unteem.pl"), | ||
os.path.join(self.source_folder, "Modules", "ThirdParty", "NrrdIO", "src", "NrrdIO", "privateNrrd.h"), | ||
]: | ||
replace_in_file(self, path, "itk_zlib.h", "zlib.h") | ||
# Truncate some third-party modules that are provided by conan_cmake_project_include.cmake | ||
for pkg in ["DCMTK", "DoubleConversion", "GDCM", "Expat", "HDF5", "JPEG", "PNG", "TIFF", "ZLIB"]: |
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.
# Unvendor ZLIB | |
for path in [ | |
os.path.join(self.source_folder, "Modules", "IO", "GIPL", "src", "itkGiplImageIO.cxx"), | |
os.path.join(self.source_folder, "Modules", "IO", "PhilipsREC", "src", "itkPhilipsRECImageIO.cxx"), | |
os.path.join(self.source_folder, "Modules", "Nonunit", "Review", "src", "itkVoxBoCUBImageIO.cxx"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "GDCM", "src", "gdcm", "Utilities", "gdcm_zlib.h"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "GIFTI", "src", "gifticlib", "gifti_io.h"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "NIFTI", "src", "nifti", "znzlib", "znzlib.h"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "MetaIO", "src", "MetaIO", "src", "localMetaConfiguration.h"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "MINC", "src", "libminc", "nifti", "znzlib.h"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "NrrdIO", "src", "NrrdIO", "unteem.pl"), | |
os.path.join(self.source_folder, "Modules", "ThirdParty", "NrrdIO", "src", "NrrdIO", "privateNrrd.h"), | |
]: | |
replace_in_file(self, path, "itk_zlib.h", "zlib.h") | |
# Truncate some third-party modules that are provided by conan_cmake_project_include.cmake | |
for pkg in ["DCMTK", "DoubleConversion", "GDCM", "Expat", "HDF5", "JPEG", "PNG", "TIFF", "ZLIB"]: | |
# Truncate some third-party modules that are provided by conan_cmake_project_include.cmake | |
for pkg in ["DCMTK", "DoubleConversion", "GDCM", "Expat", "HDF5", "JPEG", "PNG", "TIFF"]: |
Not needed to manipulate zlib, all files have a ifdef to detect what should be included. Those that are using itk_zlib.h will point to zlib as well due cmake configuration.
tc.variables["ITK_USE_SYSTEM_DCMTK"] = True | ||
tc.variables["ITK_USE_SYSTEM_DOUBLECONVERSION"] = True | ||
tc.variables["ITK_USE_SYSTEM_EIGEN"] = True | ||
tc.variables["ITK_USE_SYSTEM_FFTW"] = True | ||
tc.variables["ITK_USE_SYSTEM_GDCM"] = True | ||
tc.variables["ITK_USE_SYSTEM_HDF5"] = True | ||
tc.variables["ITK_USE_SYSTEM_ICU"] = True |
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.
Why does ICU is no longer listed? I still see it available for 5.1.2: https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.2/Modules/ThirdParty/DCMTK/CMakeLists.txt#L11
It started out reasonable-ish, but after all of the additional required fixes and the added unvendorings, the change became a lot less useful, I agree. I'll probably get rid of it when I'm working on the recipe again. |
Moves most of the patching logic to the included .cmake file and _patch_sources().
Mostly as a preparation for a version bump to v5.3 or v5.4-rc1, which require further work.