https://github.com/SyneRBI/SIRF-SuperBuild/pull/933
(1 file)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
https://cmake.org/cmake/help/latest/module/FindPythonInterp.html deprecated since 3.12, while we require 3.16.2.
Current change is minimal. We probably should pass Python_EXECUTABLE as opposed to PYTHON_EXECUTABLE to dependencies, but this needs a check which ones we use. Probably easiest to pass both. I think we should no longer attempt to pass include and other paths, and let CMake sort it out.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
In SuperBuild.cmake:
> @@ -47,7 +47,7 @@ endif() # Find CUDA option(DISABLE_CUDA "Disable CUDA" OFF) if (NOT DISABLE_CUDA) - find_package(CUDA) + find_package(CUDAToolkit REQUIRED)⬇️ Suggested change
- find_package(CUDAToolkit REQUIRED) + find_package(CUDA)
While this is needed, let's do it in another PR, as both of these could create discussions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
> if (PYTHONLIBS_FOUND)
message(STATUS "Found PYTHON_INCLUDE_DIRS=${PYTHON_INCLUDE_DIRS}")
message(STATUS "Found PYTHON_LIBRARIES=${PYTHON_LIBRARIES}")
endif()
# Set destinations for Python files
- set (BUILD_PYTHON ${PYTHONLIBS_FOUND})
+ set (BUILD_PYTHON = ${Python3_FOUND})
⬇️ Suggested change
- set (BUILD_PYTHON = ${Python3_FOUND})
+ set (BUILD_PYTHON ${Python3_FOUND})
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Need to update how we look for cython I guess
[100%] Generating build/timestamp
Traceback (most recent call last):
File "/home/runner/work/SIRF-SuperBuild/SIRF-SuperBuild/build/builds/TomoPhantom/build/Wrappers/Python/setup.py", line 19, in <module>
from Cython.Distutils import build_ext
ModuleNotFoundError: No module named 'Cython'
gmake[5]: *** [Wrappers/Python/CMakeFiles/PythonWrapper.dir/build.make:76: Wrappers/Python/build/timestamp] Error 1
gmake[4]: *** [CMakeFiles/Makefile2:157: Wrappers/Python/CMakeFiles/PythonWrapper.dir/all] Error 2
gmake[3]: *** [Makefile:136: all] Error 2
gmake[2]: *** [CMakeFiles/TomoPhantom.dir/build.make:86: builds/TomoPhantom/stamp/TomoPhantom-build] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:755: CMakeFiles/TomoPhantom.dir/all] Error 2
This error is different from why I saw in #932 but it's possible that @NicoleJurjew only saw the Tomophantom error when using this branch I guess.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
With the suggested changes, the PYTHONPATH isn't exported correctly.
I think some changes need to be done here, as well:
https://github.com/SyneRBI/SIRF-SuperBuild/blob/5f1a7f498bf6f41551c19eb53798b5d33af43eec/SuperBuild.cmake#L366C1-L386C8
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
that's a bit strange. What is the content of env_sirf.sh etc then? Can you just do a
message(STATUS "PYTHON_EXECUTABLE=${PYTHON_EXECUTABLE}")
before those statement to check what it is?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
This is the content of env_sirf.sh:
grafik.png (view on web)
Where will that message be written to then? Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
See also #313
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 4 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Looks like a gadgetron build error related to https://groups.google.com/g/linux.debian.bugs.dist/c/JpHAw3GY0Bk
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
This was all discussed in #937. I've contributed a PR to Gadgetron. However, moving on with Gadgetron created a lot of other problems. So @paskino and I decided to wait with that, and just fix Ubuntu 22.04. This is done in #943. So suggestion therefore is to finish #943, merge it, and merge that back on here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
Did you check if the dependencies indeed accept Python3_EXECUTABLE. In any case, I'm fairly sure this needs to be Python_EXECUTABLE. Easiest might be to pass both of course, although that's a bit ugly.
Also, in SIRF and STIR, we have a "backwards compatible" line to set Python_EXECUTABLE if PYTHON_EXECUTABLE is set. Maybe no longer need it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl I've merged #943.
Note that we'll also need to update CHANGES.md here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 5 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl commented on this pull request.
On SuperBuild.cmake:
technically
find_package |
-D hint |
|---|---|
Python3 |
Python3_EXECUTABLE |
Python |
Python_EXECUTABLE |
so perhaps we could just use Python without the 3-suffix everywhere?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans requested changes on this pull request.
In SuperBuild.cmake:
> @@ -431,7 +431,7 @@ if (WIN32) endif() if (PYTHONINTERP_FOUND)⬇️ Suggested change
- if (PYTHONINTERP_FOUND) + if (Python_EXECUTABLE)
In SuperBuild/External_NiftyPET.cmake:
> + -DPython_EXECUTABLE=${NiftyPET_PYTHON_EXECUTABLE}
+ -DPython3_Executable=${NiftyPET_PYTHON_EXECUTABLE}
note that we are stuck to NiftyPET 2. Are you sure this is ok?
In VirtualBox/scripts/UPDATE_functions.sh:
> @@ -185,7 +185,8 @@ SuperBuild_install(){
-DDEVEL_BUILD="$DEVEL_BUILD" \
-DNIFTYREG_USE_CUDA=OFF\
-DBUILD_CIL=ON\
- -DPYTHON_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython3_EXECUTABLE="$PYTHON_EXECUTABLE"\
⬇️ Suggested change
- -DPython3_EXECUTABLE="$PYTHON_EXECUTABLE"\
This isn't necessary for our own SB.
In SuperBuild/External_ROOT.cmake:
> + -DPython_EXECUTABLE:FILE=${Python_EXECUTABLE}
+ -DPython3_EXECUTABLE:FILE=${Python_EXECUTABLE}
⬇️ Suggested change
- -DPython_EXECUTABLE:FILE=${Python_EXECUTABLE}
- -DPython3_EXECUTABLE:FILE=${Python_EXECUTABLE}
+ ${PYTHONLIBS_CMAKE_ARGS}
In SuperBuild.cmake:
> message(STATUS "Python version ${PYTHON_VERSION_STRING}")
endif()
-
- find_package(PythonLibs 3)
-
+
if (PYTHONLIBS_FOUND)
Python_*, and different conditional
In SuperBuild.cmake:
> CONDA: do nothing")
set_property(CACHE PYTHON_STRATEGY PROPERTY STRINGS PYTHONPATH SETUP_PY CONDA)
endif()
# set PYTHONLIBS_CMAKE_ARGS to be used in the ExternalProject_add calls
# note: Find_package(PythonLibs) takes PYTHON_INCLUDE_DIR and PYTHON_LIBRARY as input
- set (PYTHONLIBS_CMAKE_ARGS -DPYTHON_EXECUTABLE:FILEPATH=${PYTHON_EXECUTABLE})
+ set (PYTHONLIBS_CMAKE_ARGS
+ -DPython_EXECUTABLE:FILEPATH=${Python_EXECUTABLE}
+ -DPython3_EXECUTABLE:FILEPATH=${Python_EXECUTABLE})
if (EXISTS "${PYTHON_INCLUDE_DIR}")
Python, not PYTHON
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl commented on this pull request.
In SuperBuild/External_NiftyPET.cmake:
> + -DPython_EXECUTABLE=${NiftyPET_PYTHON_EXECUTABLE}
+ -DPython3_Executable=${NiftyPET_PYTHON_EXECUTABLE}
Any reason why? Current NiftyPET is Python3 only.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl commented on this pull request.
In VirtualBox/scripts/UPDATE_functions.sh:
> @@ -185,7 +185,8 @@ SuperBuild_install(){
-DDEVEL_BUILD="$DEVEL_BUILD" \
-DNIFTYREG_USE_CUDA=OFF\
-DBUILD_CIL=ON\
- -DPYTHON_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython3_EXECUTABLE="$PYTHON_EXECUTABLE"\
I was hesitant just in case people use the new UPDATE_functions.sh on an old SB version.
Hopefully a niche problem, though, so will remove this line.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
There seem to be 2 different Python versions used in the CIL install, resulting in the cil not being found at import. There are 2 causes for this.
For the SB, CMake finds a surprising version (which is then passed on)
Found Python: /opt/hostedtoolcache/Python/3.10.12/x64/bin/python3.10 (found version "3.10.12") found components: Interpreter Development Development.Module Development.Embed
Found Python_EXECUTABLE=/opt/hostedtoolcache/Python/3.10.12/x64/bin/python3.10
The CMake strategy for selecting a python version is still far too complicated/error-prone. I suggest therefore to explicitly set
Python_EXECUTABLE=which python` in the Action.
In addition, it looks like the CIL version that is being built (I only checked with DEVEL_BUILD=OFF) uses old-style:
-- Found PythonLibs: /usr/lib/x86_64-linux-gnu/libpython3.10.so (found version "3.10.12")
-- Found PYTHON_EXECUTABLE=/home/runner/virtualenv/bin/python
I think therefore we need to add PYTHON_EXECUTABLE to the PYTHONLIBS_CMAKE_ARGS variable. That's safest in any case.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
In VirtualBox/scripts/UPDATE_functions.sh:
> @@ -185,7 +185,8 @@ SuperBuild_install(){
-DDEVEL_BUILD="$DEVEL_BUILD" \
-DNIFTYREG_USE_CUDA=OFF\
-DBUILD_CIL=ON\
- -DPYTHON_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython3_EXECUTABLE="$PYTHON_EXECUTABLE"\
oh, right. This is on the VM. Sorry. Let's leave this in then.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
In VirtualBox/scripts/UPDATE_functions.sh:
> @@ -185,7 +185,8 @@ SuperBuild_install(){
-DDEVEL_BUILD="$DEVEL_BUILD" \
-DNIFTYREG_USE_CUDA=OFF\
-DBUILD_CIL=ON\
- -DPYTHON_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython_EXECUTABLE="$PYTHON_EXECUTABLE"\
+ -DPython3_EXECUTABLE="$PYTHON_EXECUTABLE"\
Certainly as on GHA, CMake found the wrong python...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
In SuperBuild/External_NiftyPET.cmake:
> + -DPython_EXECUTABLE=${NiftyPET_PYTHON_EXECUTABLE}
+ -DPython3_Executable=${NiftyPET_PYTHON_EXECUTABLE}
We currently only use NiftyPET via STIR, but that all happens on the C++ front. NiftyPET3 no longer has the appropriate libraries, and I have no energy for it.
See UCL/STIR#1294 to jog your memory :-)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
In SuperBuild/External_NiftyPET.cmake:
> + -DPython_EXECUTABLE=${NiftyPET_PYTHON_EXECUTABLE}
+ -DPython3_Executable=${NiftyPET_PYTHON_EXECUTABLE}
It could be worthwile wrapping NiftyPET in CIL instead, but it's nowhere on my (our?) list of priorities.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
ModuleNotFoundError: No module named 'Cython'
when building TomoPhantom. Not sure why this happens now? Wrong python used? TBH, We shouldn't really build TomoPhantom as part of our CI I think. @paskino ?
Run actions/upload-artifact@v4
with:
name: build_log_files-ubuntu-22.04-gcc9-Release-DEVEL=OFF
...
Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run
I guess we have a problem with the naming somewhere.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
TomoPhantom v2.0.0 (and also the slightly more recent v3.0) doesn't specify build deps. Its setup.py needs Cython & numpy. Adding them to the build step works.
Current test failures due to lack of CIL deps... CIL:scripts/requirements-test.yml are not included in CIL:pyproject.toml and I think they should be.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
In SuperBuild.cmake:
> CONDA: do nothing")
set_property(CACHE PYTHON_STRATEGY PROPERTY STRINGS PYTHONPATH SETUP_PY CONDA)
endif()
# set PYTHONLIBS_CMAKE_ARGS to be used in the ExternalProject_add calls
- # note: Find_package(PythonLibs) takes PYTHON_INCLUDE_DIR and PYTHON_LIBRARY as input
- set (PYTHONLIBS_CMAKE_ARGS -DPYTHON_EXECUTABLE:FILEPATH=${PYTHON_EXECUTABLE})
- if (EXISTS "${PYTHON_INCLUDE_DIR}")
- set (PYTHONLIBS_CMAKE_ARGS ${PYTHONLIBS_CMAKE_ARGS}
- -DPYTHON_INCLUDE_DIR:PATH=${PYTHON_INCLUDE_DIR})
- endif()
- if (EXISTS "${PYTHON_LIBRARY}")
+ set (PYTHONLIBS_CMAKE_ARGS
+ -DPython_EXECUTABLE:FILEPATH=${Python_EXECUTABLE}
+ -DPython3_EXECUTABLE:FILEPATH=${Python_EXECUTABLE}
+ -DPYTHON_EXECUTABLE:FILEPATH=${Python_EXECUTABLE})
+ if (Python_Development_FOUND)
we require it, so no extra if
In SuperBuild.cmake:
> + -DPython_INCLUDE_DIRS=${Python_INCLUDE_DIRS}
+ -DPython_LIBRARIES=${Python_LIBRARIES}
+ -DPYTHON_INCLUDE_DIR=${Python_INCLUDE_DIRS}
+ -DPYTHON_LIBRARY=${Python_LIBRARIES})
I don't think we should be setting the *DIRS and *LIBRARIES variables, only the "singular" ones, as they are the input ones, while the "plural" variables are the output. (It could go wrong when there are multiple directories in the "plural" variables).
That means though that we should only set those if the input ones are set.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
For the SB, CMake finds a surprising version (which is then passed on)
Found Python: /opt/hostedtoolcache/Python/3.10.12/x64/bin/python3.10 (found version "3.10.12") found components: Interpreter Development Development.Module Development.Embed Found Python_EXECUTABLE=/opt/hostedtoolcache/Python/3.10.12/x64/bin/python3.10
The CMake strategy for selecting a python version is still far too complicated/error-prone. I suggest therefore to explicitly set
Python_EXECUTABLE=${which python}in the Action.
This is still the case, hence the failures.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
@casperdcl pushed 1 commit.
You are receiving this because you are subscribed to this thread.![]()
Ah I finally understood; you don't want to use the system Python.
TL;DR we need to unset the cmake helpers defined by https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#environment-variables
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Ah I finally understood; you don't want to use the system Python.
TL;DR we need to unset the cmake helpers defined by
actions/setup-python.
pretty confusing... I don't care which python is being used here, but obviously we should always use the same. As we seem to be using setup-python with pip, and then create a venv, we should be using that one. I guess that's what you did now, so ... great.
Sadly, some zenodo download issues ATM.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans approved this pull request.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
@KrisThielemans commented on this pull request.
oh. still need to update CHANGES.md!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Merged #933 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Success! Thanks @casperdcl . however, can you now have a different PR to update CHANGES.md, saying that we've switched to FindPython (maybe a link), and that they should use Python_EXECUTABLE or any other variables as indicated in the CMake doc? (Sorry, I shouldn't have approved the PR :-))
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Already updated the changelog; feel free to update further
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
Sorry. Confused. Which changelog?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()
https://github.com/SyneRBI/SIRF-SuperBuild/blob/master/CHANGES.md
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.![]()