[SyneRBI/SIRF-SuperBuild] defer installation of some CIL run requirements after build (PR #937)
Edoardo Pasca
- fixes #935
Defers the installation of CIL run requirements which confuse CMake.
You can view, comment on, or merge this pull request online at:
https://github.com/SyneRBI/SIRF-SuperBuild/pull/937
Commit Summary
- 42c81af defer installation of some CIL run requirements after build
File Changes
(2 files)
- M Dockerfile (9)
- M docker/user_demos.sh (2)
Patch Links:
- https://github.com/SyneRBI/SIRF-SuperBuild/pull/937.patch
- https://github.com/SyneRBI/SIRF-SuperBuild/pull/937.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
There is an issue with Gadgetron or dcmtk and GCC? https://github.com/SyneRBI/SIRF-SuperBuild/actions/runs/13265453075/job/37031643450?pr=937
/usr/include/dcmtk/ofstd/ofstdinc.h:121:2: error: #error "Macro INCLUDE_CSTRING not supported anymore. Include <cstring> directly."
121 | #error "Macro INCLUDE_CSTRING not supported anymore. Include <cstring> directly."
This is the error on my docker instance, potentially I haven't redirected stderr so the output seems incomplete.
#24 86.45 ninja: build stopped: subcommand failed.
#24 86.45 ninja: build stopped: subcommand failed.
#24 ERROR: process "/bin/bash -o pipefail -c BUILD_FLAGS=\"-G Ninja -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DSTIR_ENABLE_OPENMP=${STIR_ENABLE_OPENMP} -DUSE_SYSTEM_Armadillo=${USE_SYSTEM_Armadillo} -DUSE_SYSTEM_Boost=${USE_SYSTEM_Boost} -DUSE_SYSTEM_FFTW3=${USE_SYSTEM_FFTW3} -DUSE_SYSTEM_HDF5=${USE_SYSTEM_HDF5} -DUSE_ITK=${USE_ITK} -DUSE_SYSTEM_SWIG=${USE_SYSTEM_SWIG} -DUSE_NiftyPET=${USE_NiftyPET} -DBUILD_siemens_to_ismrmrd=${BUILD_siemens_to_ismrmrd} -DBUILD_pet_rd_tools=${BUILD_pet_rd_tools} -DGadgetron_USE_CUDA=${Gadgetron_USE_CUDA} -DBUILD_CIL=${BUILD_CIL}\" EXTRA_BUILD_FLAGS=\"${EXTRA_BUILD_FLAGS}\" bash /opt/scripts/user_sirf-ubuntu.sh && fix-permissions /opt/SIRF-SuperBuild /opt/ccache" did not complete successfully: exit code: 1
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
There is. gadgetron/gadgetron#1277. However, this shouldn't occur on ubuntu 22.04. Does it say somewhere what version of dcmtk is being used?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
Good spot! My docker image says it's 24.04
(base) root@2a418c82a9b7:~# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
I suppose the base docker image has upgraded to 24.04 more or less recently.
We hit a similar issue but for a too old image with CIL.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 561937d use patched Gadgetron
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- b1c3014 revert gadgetron changes and use ubuntu22.04 for docker base image
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
On my docker instance with the ubuntu-22.04 base image it builds
(base) root@2035ef435116:/opt/SIRF-SuperBuild# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 22.04.4 LTS
Release: 22.04
Codename: jammy
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 4ac8902 added changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In Dockerfile:
> RUN if test "$BUILD_GPU" != 0; then \ sed -ri 's/^(\s*)#\s*(- \S+.*#.*GPU.*)$/\1\2/' /opt/scripts/requirements.yml; \ fi \ && if test "$BUILD_CIL" != "OFF"; then \ - sed -r -i -e '/^\s*- (cil|ccpi-regulariser).*/d' /opt/scripts/requirements.yml; \ + sed -r -i -e '/^\s*- (cil|ccpi-regulariser|h5py|dxchange).*/d' /opt/scripts/requirements.yml; \ + else \ + sed -r -i -e '/^\s*- (h5py|dxchange|cil|ccpi-regulariser|pillow|olefile|pywavelets|cil-data|tqdm|numba|zenodo_get).*/d' /opt/scripts/requirements.yml; \
I think SIRF-exercises do need numba and tqdm, zenodo_get would be useful as well
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
Why did you move the python setup later on GHA? GHA is using the system python and pip. This doesn't have anything to do with conda discussions/problems on docker.
By the way, why not pin the hdf5 version (both conda and pip I guess to be the same as the system one. Then it wouldn't matter which one is picked up. For conda, I suppose we'd still need to install cxx-compiler to avoid what we saw at #935 (comment)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
Let's install hdf5 from conda/pip of the same version of the system one to minimise incompatibilities.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino commented on this pull request.
In .github/workflows/c-cpp.yml:
> + - uses: actions/setup-python@v5
+ with:
+ python-version: '${{ steps.deps.outputs.python-version }}'
+ cache: pip
+ - name: pip install
+ working-directory: docker
+ run: PYTHON_EXECUTABLE=python3 PYTHON_INSTALL_DIR="$HOME/virtualenv" ./user_python-ubuntu.sh
+ - uses: hendrikmuhs/ccache-action@v1
+ with:
+ key: ${{ matrix.os }}-${{ env.COMPILER }}-${{ matrix.compiler_version }}-${{ env.BUILD_TYPE }}-${{ github.ref_name }}-${{ github.run_id }}
+ restore-keys: |
+ ${{ matrix.os }}-${{ env.COMPILER }}-${{ matrix.compiler_version }}-${{ env.BUILD_TYPE }}-${{ github.ref_name }}
+ ${{ matrix.os }}-${{ env.COMPILER }}-${{ matrix.compiler_version }}-${{ env.BUILD_TYPE }}
+ append-timestamp: false
let's revert this once the same version of HDF5 that is in the system is installed via pip/conda.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- aa49ed2 install hdf5 version 1.10.7 as on Ubuntu 22.04
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- c1a6e38 revert change of pip install order
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 6b20771 try to find an appropriate version by relaxing patch level
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 97a4988 pip no use cache
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- e3184a1 build h5py in pip
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 48e0ef1 do not install hdf5 from ubuntu packages and force build of Armadillo on docker
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
- GHA and the VM re-use the docker scripts, so they are broken now.
- I think you will need
cxx-compilersfor CMake - Logically speaking, it seems now to make more sense to add armadillo as a conda-forge package https://anaconda.org/conda-forge/armadillo, For pip, there seems to be
pyarmahttps://pyarma.sourceforge.io/, which might be ok.
Seems we're on a long path here...
> - pillow # cil - olefile >=0.46 # cil - pywavelets # cil - cil-data >=21.3.0 # cil - ipp >=2021.10 # cil - tqdm # cil - numba # cil + - h5py # gadgetron and CIL + - hdf5 # gadgetron and CIL
and STIR actually
In CHANGES.md:
> @@ -1,7 +1,9 @@ # ChangeLog ## v3.x.x -- Udated Versions: +- Docker image updates: + - defer installation of CIL requirements after build (#937) +- Updated Versions:
looks like you need to list gadgetron update as well.
In Dockerfile:
> @@ -25,19 +25,27 @@ ARG BUILD_CIL="OFF" COPY docker/requirements.yml /opt/scripts/ # https://jupyter-docker-stacks.readthedocs.io/en/latest/using/common.html#conda-environments # https://github.com/TomographicImaging/CIL/blob/master/Dockerfile +# First remove the CIL run-dependencies from requirements.yml as they install hdf5 and CMake gets confused
no longer correct comment?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 8f89c5c update GHA
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino commented on this pull request.
In Dockerfile:
> @@ -25,19 +25,27 @@ ARG BUILD_CIL="OFF" COPY docker/requirements.yml /opt/scripts/ # https://jupyter-docker-stacks.readthedocs.io/en/latest/using/common.html#conda-environments # https://github.com/TomographicImaging/CIL/blob/master/Dockerfile +# First remove the CIL run-dependencies from requirements.yml as they install hdf5 and CMake gets confused
not correct indeed, but it's still not passing unittests!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
paskino left a comment (SyneRBI/SIRF-SuperBuild#937)I tried armadillo from conda-forge but the superbuild didn't find the devel files.
Yes, as usual this is a long and painful road.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
paskino left a comment (SyneRBI/SIRF-SuperBuild#937)I get the following error on Gadgetron configuration. Indeed the system boost is 1.78.0.3 on Ubuntu 22.04
CMake Error at /usr/local/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
Could NOT find Boost: Found unsuitable version "1.74.0", but required is at
least "1.80.0" (found /usr/include, found components: coroutine system
date_time program_options filesystem timer context chrono)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
> - pillow # cil - olefile >=0.46 # cil - pywavelets # cil - cil-data >=21.3.0 # cil - ipp >=2021.10 # cil - tqdm # cil - numba # cil + - h5py # gadgetron and CIL + - hdf5 # gadgetron and CIL
I meant, hdf5 is needed by STIR
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF-SuperBuild#937)The Gadgetron boost error will be because you update Gadgetron (which I told you to do). Let's keep it at the original version for now to get this through and see.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 3 commits.
- 20daee0 build with one thread
- 73a4b6a install hdf5 via conda
- de11e91 add environment variable to install HDF5 from apt
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF-SuperBuild#937)Downgrade gadgetron!
However, building with conda hdf5 fails due to UCL/STIR#1557. So, if we do this, you'll need to fix that STIR (actually, cmake!) problem, and update STIR in version_config.cmake
Note that I successfully built Gadgetron via conda packages now. Update on https://github.com/SyneRBI/SIRF/wiki/Building-SIRF-and-CIL-with-conda.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 72a0947 loads of new args to cmake
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 2 commits.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
> - pillow # cil - olefile >=0.46 # cil - pywavelets # cil - cil-data >=21.3.0 # cil - ipp >=2021.10 # cil - tqdm # cil - numba # cil + - h5py # gadgetron and CIL + - hdf5 # Gadgetron, STIR and CIL + - armadillo =12.8.4 # Gadgetron + - cxx-compiler # compiler which built hdf5 from conda-forge + - libstdcxx-ng # Gadgetron + - libgcc-ng # Gadgetron + - dcmtk # Gadgetron⬇️ Suggested change
- - dcmtk # Gadgetron + - dcmtk=3.6.1 # Gadgetron
seems I forgot that on the wiki
In docker/build_system-ubuntu.sh:
> @@ -54,11 +54,14 @@ else
fi
${APT_GET_INSTALL} \
- libfftw3-dev \
- libhdf5-serial-dev \
- hdf5-helpers \
+ libfftw3-dev \
shouldn't this be in the if USE_UBUNTU_SYSTEM_LIBRARIES
In docker/build_gadgetron-ubuntu.sh:
> @@ -27,6 +23,14 @@ apt-get install -yq --no-install-recommends \ pkg-config \ golang +if USE_UBUNTU_SYSTEM_LIBRARIES=1; then
surely missing $ here (and below)
In docker/build_gadgetron-ubuntu.sh:
> libxml2-dev \ libfreetype6-dev \ libxslt-dev \
these don't seem to be required anymore.
In docker/build_gadgetron-ubuntu.sh:
> jq \ - hdf5-tools \ libopenblas-dev \
this should be in the USE_UBUNTU_SYSTEM_LIBRARIES
In docker/build_gadgetron-ubuntu.sh:
> liblapack-dev \ liblapacke-dev \ libplplot-dev \
this should be in the USE_UBUNTU_SYSTEM_LIBRARIES
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF-SuperBuild#937)Not 100% sure what we do with this now, but surely we urgently need the diff
- ARG BASE_CONTAINER=quay.io/jupyter/scipy-notebook:ubuntu-latest
+ ARG BASE_CONTAINER=quay.io/jupyter/scipy-notebook:ubuntu-22.04
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
In Dockerfile:
> @@ -1,5 +1,5 @@ # syntax=docker/dockerfile:1 -ARG BASE_CONTAINER=quay.io/jupyter/scipy-notebook:latest +ARG BASE_CONTAINER=quay.io/jupyter/scipy-notebook:ubuntu-22.04
required?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
paskino left a comment (SyneRBI/SIRF-SuperBuild#937)Hash before rebase 40605c7
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
@paskino pushed 45 commits.
- 4def967 [GHA] add action checkout CITATION.cff
- 82466ed GHA: use Ubuntu 22.04 (#943)
- 25e7ece Updated updated modules in find_package command for Python3
- 698cafb revert change in finding CUDA
- 4f27200 correct typo
- 7e18546 cmake: FindPython 3
- 1cbc258 cmake: fix FindPython hints
- fda719d Merge pull request #933 from NicoleJurjew/update_find_package
- b56b85b Update CHANGES.md to clarify use of FindPython in CMake (#944)
- 49372f1 Windows: find conda fftw libs
- f9d0b7c Windows: change check on platform
- 55dd1a5 Pass through CMAKE_GENERATOR and CMAKE_BUILD_TYPE
- e6e0b4e rename local variable to avoid confusion
- c5251d1 Merge pull request #950 from KrisThielemans/WindowsFixes
- 3be314a Update zenodo link
- 24abf0b VM: set matplotlib backend to `tkagg` (#952)
- de7c65c Update VirtualBox/documentation/INSTALL.md with macOS(ARM64) instruction (#956)
- 0f7a05c Update VM INSTALL instructions to point to most recent VM on Zenodo (#957)
- 614bf85 avoid Gadgetron's conversion of warnings to errors
- af07124 support Gadgetron_EXTRA_CMAKE_ARGS
- 991b579 update CHANGES.md
- e4a9627 Merge pull request #958 from KrisThielemans/Gadgetron_build_options
- 283e6db various updates to VirtualBox documentation
- 847a6ca minor fixes to VB scripts
- 82c5a79 Merge pull request #961 from KrisThielemans/VB_doc_improvements
- d0eace2 defer installation of some CIL run requirements after build
- 9dc9e81 use patched Gadgetron
- 7b5a9b0 revert gadgetron changes and use ubuntu22.04 for docker base image
- 93a3c6b add date to Gadgetron hash
- 1f64c7f added changes
- 9e48711 defer pip install in GHA
- 47f369c added zenodo_get for CIL >=24.2.0
- 5edf6f5 install hdf5 version 1.10.7 as on Ubuntu 22.04
- c65c194 revert change of pip install order
- 2b6a23f try to find an appropriate version by relaxing patch level
- 0f09ed0 pip no use cache
- 459d684 build h5py in pip
- 1002cdc do not install hdf5 from ubuntu packages and force build of Armadillo on docker
- 62c3531 update GHA
- 823bc17 build with one thread
- f350fcb install hdf5 via conda
- 769165d add environment variable to install HDF5 from apt
- 4b38d65 loads of new args to cmake
- 2f4fe01 change way to build
- 91ca9c0 Merge branch 'system_hdf5' of github.com:paskino/SIRF-SuperBuild into system_hdf5
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Edoardo Pasca
paskino left a comment (SyneRBI/SIRF-SuperBuild#937)Currently encountering a weird error. If building with SB the build fails because of link errors with ISMRMRD. If just configuring with SB and commenting out the build line and then launching the image and executing bash within the container, ISMRMRD builds nicely.
It boils down to the incomplete environment in the case of the docker run build, despite we seem to source the same environment file the environments look completely different and the docker one lacks CXXFLAGS for instance. The CMakeCache for ISMRMRD does not have any linker flags and therefore it fails.
- Why are the environments different?
- Who sets the
CXXFLAGS?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Casper da Costa-Luis
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.