Conversation
d3f7569 to
1af5c7a
Compare
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
13c55bd to
d637e4d
Compare
|
@jcfr thank you for looking into this! Regarding windows, in the past I used Appveyor to build ITK and DCMTK dependencies in order to fit the total build time within limits that were in place at that time. I tried switching to build all dependencies as part of the overall workflow, but that unfortunately failed with "error MSB8066", which I suspect was because of the path length exceeding allowance. I then opened a separate PR where I also reduced the directory name a little: #516. I also tried to update the Appveyor project to make package, but Appveyor fails without clear indication why (this is not entirely surprising, since last time I used it was 2 years ago when DCMTK was updated last time!). I spent couple of hours on this today, but didn't find a solution. Can you for now focus on mac, where tests are still failing? |
d637e4d to
8968e29
Compare
|
@jcfr after looking a bit more, I realized that the external dependencies were downloaded but not used in the workflow. As you can see in the windows build for #516, it completes correctly, so the build error in this PR is a new regression. Perhaps this is due to the path length exceeding the limit. Reducing the top-level folder to single character didn't fix the problem. I merged #516 and rebased this PR to get rid of that misleading portion of the workflow. |
|
Thanks for working on this. I am now headed out in the wilderness for a few days and will be able to have a look when back. 🙏 thanks for the patience. |
|
Note that DCMTK 3.6.9 has been released 3 months ago. I there any reason from your side to stick to 3.6.8 for now (which was current when this pull request has been opened)? Of course, if you need any information about 3.6.9 or DCMTK in general, let me know. |
…3e373 Changes originally integrated through 4ff223e ("COMP: use https instead of git for clones", 2022-03-23) are reverted. Instead, similar changes are added through this update. List of changes: ``` $ git shortlog dff914a..613e373 --no-merges Andras Lasso (1): Add support for EXTERNAL_PROJECT_ADDITIONAL_DIRS Jean-Christophe Fillion-Robin (29): ci: Update CircleCI to use centosbuild/centos6:latest BUG: ExternalProjectDependency: Ensure USES_TERMINAL_* options are enabled ExternalProjectDependency: Ignore CACHE value of type INTERNAL circleci: Use dockbuild/centos6 instead of unmaintained centosbuild/centos6 ExternalProjectDependency: If set, propagate CMAKE_JOB_* variables Add test checking that variable ending with "-NOTFOUND" are passed test: Remove unused debug statement, initalize variable ExternalProjectDependency: Support ALL_PROJECTS variable ending with NOTFOUND Add MarkAsSuperBuild-EP_ARGS_VAR test ExternalProjectDependency: Remove debug statement inadvertently integrated Add support for "ExternalProject_Add_Dependencies" ExternalProject_SetIfNotDefined: Add test checking interaction with cache variable ExternalProject_SetIfNotDefined: If any, display corresponding cache value ExternalProject_SetIfNotDefined: Update docstring ExternalProject_Include_Dependencies: Check if superbuild variable is improperly set ExternalProjectDependency: Fix spellcheck ExternalProjectDependency: Fix variable type of automatically propagated options circleci: Use dockbuild/centos7-devtoolset7-gcc7 instead of dockbuild/centos6 ExternalProjectDependency: Ensure ``User Package Registry`` look up using ``find_*`` commands is disabled ExternalProjectDependency: Fix CMake version check for disabling ``User Package Registry`` doc: Update website links to https versions ExternalProjectDependency: Force git protocol to https ci: Add GitHub Actions based workflow ci: Exclude "IncludeDependencies-DifferentGenerator-Test" with CMake 2.8.x DOC: Update README adding GitHub Actions badge ci: cancel in-progress GitHub Actions workflow on repeated pushes ExternalProjectDependency: Set DOWNLOAD_EXTRACT_TIMESTAMP to 1 if CMake >= 3.24 style: Fix typos identified using codespell ci: Add "codespell" GitHub Actions workflow ```
Thanks for checking. We are matching the version currently used in Slicer. Once this PR is finalized and integrated, we will look into transitioning both to 3.6.9. |
|
@fedorov Once the CI completes and is green, this will be ready for integration using either "Merge" or "Rebase & Merge". 🙏 Thanks again for the patience. |
I have a plan to address the issue, and I should be able to follow up in the next few days based on my access to the internet. |
* Include runtime libraries to ensure compatibility in SlicerExecutionModel
This update resolves a termination fault that occurs when running the CLI
executable built with a newer MSVC runtime on systems with older runtime
libraries.
By explicitly including the runtime libraries, compatibility is ensured,
avoiding reliance on the system's default runtime version.
* Report missing longflag in module description XML
List of changes:
```
$ git shortlog 432552634..91b921bd5 --no-merges
Andras Lasso (1):
ENH: Return error compile-time if required longflag property is not set
James Butler (1):
COMP: Include system runtime libraries
```
Update CMAKE_CXX_STANDARD from 14 to 17, and simplify settings of CMAKE_CXX_STANDARD variable based of Slicer/Slicer@d762c0465e ("COMP: Configure DCMTK with CMAKE_CXX_STANDARD", 2022-02-21) Update ITK version to match Slicer ones Fix macOS support simplifying CMakeOXVariables module based of the following commits: * Slicer/Slicer@f60a86f4ae ("ENH: Use CMake 3.0 default method to find OSX SDK root", 2019-07-02) * Slicer/Slicer@ac191b40f7 ("ENH: Simplify OSX Deployment target setting", 2022-02-16) In DCMTK and ITK external projects, respectively remove reference to unused CMake variables `ep_common_cache_args` and `ep_project_include_arg`. Remove the `DCMTK_ENABLE_BUILTIN_DICTIONARY` option, obsolete since commit DCMTK/dcmtk@4ab084d3f ("Harmonize dictionary options.", 2021-09-01), and set `DCMTK_DEFAULT_DICT` instead. Introduce `EXTERNAL_PROJECT_OPTIONAL_CMAKE_CACHE_ARGS` CMake variable to pass CMake arguments.
|
Windows error is the following: To mitigate, we will build ITK with testing disabled. |
Adapted from Slicer/Slicer@b703c30c55a ("COMP: Update ITK to 5.4.3", 2025-03-26) List of ITK changes: ``` $ git shortlog 29b78d7..55c47b7 --no-merges Andras Lasso (1): ENH: Backport libtiff fix of not displaying warning for unknown tags Brad King (1): COMP: itktiff: Suppress C99 inline only on MSVC from VS 2013 and below Bradley Lowekamp (13): BUG: Check output of StatisticsUniqueLabelMapFilterTest1 BUG: Address bugs in unique label map filters BUG: Remove dilation output in test ENH: A GTest for LabelUniqueLabelMapFilter BUG: Address race condition in SLIC filter DOC: Correct Documentation for PadImageFilter::SizeGreatestPrimeFactor BUG: Enable system TIFF with modern TIFF and cmake BUG: Remove SpatialObjectProperty's writable string methods from SWIG ENH: Download x86 clangformat on Darwin arm64 ENH: remove repetitive monochorme1 warning BUG: Fix 32-bit truncation on VectorImage Accessors BUG: Update LabelErodeDilate remote module ENH: Use get_schedaffin to determine number of threads Dženan Zukić (2): COMP: Get ITK_USE_FLOAT_SPACE_PRECISION=ON to compile again DOC: Remove duplicate new author name (Matthieu LAURENDEAU) Hans Johnson (1): COMP: macOS-11 Azure CI environment EOL James Butler (1): [Backport MR-4910] COMP: Update DCMTK targets based on new DCMTK namespace Jean-Christophe Fillion-Robin (1): COMP: Add support for customizing ITK namespace (draft) Matt McCormick (8): DOC: Update .zenodo BUG: Update HDF5 name mangled symbols DOC: Improve itk_hdf5_mangle.h.in instructions BUG: Move InputSpaceName, OutputSpaceName from Transform to TransformBase COMP: Set CastXML flag based on CMAKE_CXX_STANDARD STYLE: clang-format fixes to itkSLICImageFilter.hxx COMP: Remove unused setuptools upgrade in Linux builds COMP: Bump ITKTotalVariation remote module Matthew McCormick (27): ENH: Initial pixi configuration DOC: Document development with Pixi ENH: Add Pixi dev environment ENH: Add Pixi GitHub Actions Configuration BUG: Fix pixi caching of Python Debug builds ENH: Add Pixi python-exe and python-exe-debug tasks ENH: Python dispatch on the first RequiredInputName ENH: Add Pixi osx-64 configuration ENH: Add Intel Mac to Pixi GitHub Action tests ENH: Add Pixi mac ARM configuration ENH: Add ARM Mac to Pixi GitHub Action tests ENH: Bump ITK version to 5.3.1 DOC: Updates for release testing data uploads BUG: GenerateImageSource sets Size from ReferenceImage COMP: Use .clang-format from release-5.4 branch STYLE: clang-format updates to itkGridImageSourceTest2.cxx ENH: Bump ITK version to 5.4.2 DOC: Update maintenance branch for 5.4 series DOC: Point to RTD Doxygen documentation, ITKDoxygen Docker build DOC: Update Doxygen class links to use ReadTheDocs DOC: Update versioned Doxygen links to point to ReadTheDocs DOC: ITK 5.4.0 release notes DOC: Add ITK 5.4.2 release notes BUG: Remove VNL Netlib rpoly files COMP: Bump MeshToPolyData Remote Module to 2024-03-14 main BUG: Clamp GlobalDefault threads with GlobalMaximum with initialization ENH: Bump ITK version to 5.4.3 Mihail Isakov (1): BUG: Fix ExposeMetaData<std::vector<double>> im MetaImageIO Niels Dekker (3): ENH: Add nested CoordinateType aliases as alternative to CoordRepType ENH: Add ...CoordinateType aliases as alternative to ...CoordRepType BUG: `QuadEdgeMeshPoint` should be properly initialized by `{}` Simon Rit (1): BUG: Remove check on WrapITK.pth existence in itk_python_add_test Ziv Yaniv (1): BUG: Missing writing of the nifti descrip field. ntustison (1): BUG: Incorrect size for closed parametric dimension. ```
|
Turns out that ITK is already built with Before doing so, we just updated the version of ITK to match the one recently updated in Slicer through the following pull request: |
|
|
Do you know why this issue is not happening in Slicer superbuild? |
This was likely related to the fact we are building the libraries statically when building dcmqi standalone. Considering that updating ITK address this, there is not need to explicitly disable the |
|
This is ready to be integrated using either |
|
Once this is integrated, we will likely have to also fix the "publish" step that is now using an outdated approach. |
|
JC, thank you, so great to see this ready! I pinged @michaelonken in case he would like to take a look.
This is probably why the latest release workflow didn't route the linux and mac binaries correctly. |
|
Once this is integrated, we will on updating dcmth to 3.6.9 in both Slicer and DCMQI. Also, if you grant me admin access (at least for the next few weeks) to dcmqi I will be able to more easily address the upload of release packages. |
|
admin access granted! |
🙏 This will be helpful to finalize |
Update
CMAKE_CXX_STANDARDfrom 14 to 17, and simplify settings ofCMAKE_CXX_STANDARDvariable based of Slicer/Slicer@d762c0465e ("COMP: Configure DCMTK with CMAKE_CXX_STANDARD", 2022-02-21)Update ITK version to match Slicer ones
Fix macOS support simplifying CMakeOXVariables module based of the following
commits:
"ENH: Use CMake 3.0 default method to find OSX SDK root", 2019-07-02)"ENH: Simplify OSX Deployment target setting", 2022-02-16)In DCMTK and ITK external projects, respectively remove reference to unused CMake variables
ep_common_cache_argsandep_project_include_arg.Remove the
DCMTK_ENABLE_BUILTIN_DICTIONARYoption, obsolete since commit DCMTK/dcmtk@4ab084d3f ("Harmonize dictionary options.", 2021-09-01), and setDCMTK_DEFAULT_DICTinstead.Introduce
EXTERNAL_PROJECT_OPTIONAL_CMAKE_CACHE_ARGSCMake variable to pass CMake arguments.