[CCPPETMR/SIRF-SuperBuild] Version Tagging (#29)
Edoardo Pasca
I've added a file version_config.cmake with the versions of all the packages we build.
You can view, comment on, or merge this pull request online at:
https://github.com/CCPPETMR/SIRF-SuperBuild/pull/29
Commit Summary
- load versions file
- workaround for CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT
- added all versions for dependencies
- Use specific versions from version_config.cmake
- removed comment
File Changes
- M CMakeLists.txt (2)
- M SuperBuild.cmake (2)
- M SuperBuild/External_Armadillo.cmake (5)
- M SuperBuild/External_FFTW3.cmake (12)
- M SuperBuild/External_HDF5.cmake (17)
- M SuperBuild/External_SIRF.cmake (2)
- M SuperBuild/External_SWIG.cmake (11)
- M SuperBuild/External_googletest.cmake (1)
- M version_config.cmake (43)
Patch Links:
- https://github.com/CCPPETMR/SIRF-SuperBuild/pull/29.patch
- https://github.com/CCPPETMR/SIRF-SuperBuild/pull/29.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
thanks. a few small comments, and one question what DEVEL_BUILD should do.
Also, travis seems to fail.
In SuperBuild/External_HDF5.cmake:
>
if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalProjName}}" ) )
message(STATUS "${__indent}Adding project ${proj}")
### --- Project specific additions here
- set(HDF5_Install_Dir ${SUPERBUILD_INSTALL_DIR})
+ set(HDF5_Install_Dir ${CMAKE_CURRENT_BINARY_DIR}/INSTALL)
why this change? Others install to SUPERBUILD_INSTALL_DIR. That is set to CMAKE_INSTALL_PREFIX, so this would be expected behaviour.
> set(ISMRMRD_TAG v1.3.2) +## HDF5 +set(HDF5_URL https://support.hdfgroup.org/ftp/HDF5/releases/hdf5-1.10/hdf5-1.10.0-patch1/src/CMake-hdf5-1.10.0-patch1.tar.gz ) +set(HDF5_MD5 6fb456d03a60f358f3c077288a6d1cd8 ) + +## SWIG +if (WIN32) + set(SWIG_URL http://prdownloads.sourceforge.net/swig/swigwin-3.0.12.zip ) + set(SWIG_MD5 a49524dad2c91ae1920974e7062bfc93 ) +else(WIND32)
WIN32 not WIND32
In SuperBuild/External_SIRF.cmake:
> @@ -62,7 +62,7 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr
ExternalProject_Add(${proj}
${${proj}_EP_ARGS}
GIT_REPOSITORY ${${proj}_URL}
- GIT_TAG release-0.9.0
+ GIT_TAG ${default_SIRF_TAG}
${SIRF_TAG}?
> option(DEVEL_BUILD "Use current versions of major packages" OFF)
+## Googletest
+set(googletest_TAG release-1.8.0)
+
+
if (DEVEL_BUILD)
set (default_SIRF_GIT_TAG master)
else()
set(default_SIRF_GIT_TAG v0.9.0)
endif()
option(GIT_TAG_SIRF "git tag for SIRF" ${default_SIRF_GIT_TAG})
option var has to be SIRF_TAG to be consistent with others.
> + +## FFTW3 +if(WIN32) + set(FFTW3_URL https://s3.amazonaws.com/install-gadgetron-vs2013/Dependencies/FFTW/zip/FFTW3.zip ) + set(FFTW3_MD5 a42eac92d9ad06d7c53fb82b09df2b6e ) +else(WIN32) + set(FFTW3_URL http://www.fftw.org/fftw-3.3.5.tar.gz ) + set(FFTW3_MD5 6cc08a3b9c7ee06fdd5b9eb02e06f569 ) +endif(WIN32) + +set(FFTW3double_URL $FFTW3_URL) +set(FFTW3double_MD5 $FFTW3_MD5) + + + +## STIR set(STIR_TAG stir_rel_3_00)
has to be git hash at top of current CCPPETMR fork.
Need URL as well? We said we'd use the "upstream" version (UCL/STIR) such that it's easier to get a brand-new version
> set(STIR_TAG stir_rel_3_00) + +## Gadgetron set(GADGETRON_TAG v3.8.2)
as for STIR
> set(GADGETRON_TAG v3.8.2) + +## ISMRMRD set(ISMRMRD_TAG v1.3.2)
as for STIR, but this needs git tag f98df72fcd85e739fb41083561d8d96c951520bd (which was the one for 0.9.0)
> set(ISMRMRD_TAG v1.3.2) +## HDF5 +set(HDF5_URL https://support.hdfgroup.org/ftp/HDF5/releases/hdf5-1.10/hdf5-1.10.0-patch1/src/CMake-hdf5-1.10.0-patch1.tar.gz ) +set(HDF5_MD5 6fb456d03a60f358f3c077288a6d1cd8 ) + +## SWIG +if (WIN32) + set(SWIG_URL http://prdownloads.sourceforge.net/swig/swigwin-3.0.12.zip ) + set(SWIG_MD5 a49524dad2c91ae1920974e7062bfc93 ) +else(WIND32) + set(SWIG_URL http://prdownloads.sourceforge.net/swig/swig-3.0.12.tar.gz ) + set(SWIG_MD5 82133dfa7bba75ff9ad98a7046be687c ) +endif(WIN32) option(DEVEL_BUILD "Use current versions of major packages" OFF)
currently this only works for SIRF and only allows binary choice.
We need to be able to specify a specific tag as well. And we need the same for Gadgetron etc. So I guess they all the tags need to be options (mark_as_advanced). Not 100% if the DEVEL_BUILD should set STIR et al to master as well.
Edoardo Pasca
@paskino commented on this pull request.
In SuperBuild/External_HDF5.cmake:
>
if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalProjName}}" ) )
message(STATUS "${__indent}Adding project ${proj}")
### --- Project specific additions here
- set(HDF5_Install_Dir ${SUPERBUILD_INSTALL_DIR})
+ set(HDF5_Install_Dir ${CMAKE_CURRENT_BINARY_DIR}/INSTALL)
This is a a previous attempt to fix an issue which I fixed with this commit: ac26906
I'll remove that.
Actually the fix is a workaround but I couldn't have it running as it should otherwise.
Edoardo Pasca
@paskino commented on this pull request.
In SuperBuild/External_HDF5.cmake:
>
if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalProjName}}" ) )
message(STATUS "${__indent}Adding project ${proj}")
### --- Project specific additions here
- set(HDF5_Install_Dir ${SUPERBUILD_INSTALL_DIR})
+ set(HDF5_Install_Dir ${CMAKE_CURRENT_BINARY_DIR}/INSTALL)
About the DEVEL_BUILD, I just took inspiration by your comment here:
#24 (comment)
Edoardo Pasca
@paskino commented on this pull request.
> set(ISMRMRD_TAG v1.3.2) +## HDF5 +set(HDF5_URL https://support.hdfgroup.org/ftp/HDF5/releases/hdf5-1.10/hdf5-1.10.0-patch1/src/CMake-hdf5-1.10.0-patch1.tar.gz ) +set(HDF5_MD5 6fb456d03a60f358f3c077288a6d1cd8 ) + +## SWIG +if (WIN32) + set(SWIG_URL http://prdownloads.sourceforge.net/swig/swigwin-3.0.12.zip ) + set(SWIG_MD5 a49524dad2c91ae1920974e7062bfc93 ) +else(WIND32)
obviously!
Edoardo Pasca
> set(STIR_TAG stir_rel_3_00) + +## Gadgetron set(GADGETRON_TAG v3.8.2)
to resume upstream should be:
- gadgetron/gadgetron
- ismrmrd/ismrmrd
- UCL/STIR
Edoardo Pasca
@paskino pushed 6 commits.
- 5c8b8f0 Use version_config.cmake
- 7ca85f3 use SUPERBUILD_INSTALL_DIR variable as install directory
- f0da9c0 use repository from version_config.cmake
- 3bc0d67 rename default_SIRF_GIT_TAG to SIRF_TAG
- 2502fe3 use repository from version_config.cmake
- 7fb5c91 Consistent naming and bugfixes
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
I have fixed all the issues you pointed out and a couple more.
Travis is building with system HDF5, SWIG, FFTW3, Boost.
Travis actually builds the superbuild, without those sub-projects.
However, build of SIRF actually fails because it does not find STIR and ISMRMRD and therefore it builds only the utils. The main difference with the update scripts is that in that case cmake was called specifying the CMAKE_PREFIX_PATH pointing to STIR and ISMRMRD etc. https://github.com/CCPPETMR/CCPPETMR_VM/blob/master/scripts/UPDATE.sh#L85
Kris Thielemans
See my comments on #30. If necessary, set STIR_DIR but you must have changed something that breaks this.
Kris Thielemans
@KrisThielemans commented on this pull request.
yes
Kris Thielemans
you might be able to see which commit broke the build by going through Travis logs (although it looks like it only runs for every push, not every commit)
Kris Thielemans
@KrisThielemans commented on this pull request.
In SuperBuild/External_HDF5.cmake:
>
if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalProjName}}" ) )
message(STATUS "${__indent}Adding project ${proj}")
### --- Project specific additions here
- set(HDF5_Install_Dir ${SUPERBUILD_INSTALL_DIR})
+ set(HDF5_Install_Dir ${CMAKE_CURRENT_BINARY_DIR}/INSTALL)
yeah. I guess we could have 2 options SIRF_DEVEL_BUILD and SIRF_DEPENDENCIES_DEVEL_BUILD (to have STIR, ISMRMRD and Gadgetron at master). what do you think?
Edoardo Pasca
Investigating I found that STIR doesn't actually create the INSTALL/lib/cmake directory and doesn't fill it.
Additionally, it seems that including the version_config.cmake in the initial CMakeList.txt was creating other troubles as Gadgetron not configuring...
Kris Thielemans
i'm guessing it's not checking out the correct version. STIR 3.0 didn't create the config, which is why you need the git hash. possibly the same for gadgetron. You can go into the STIR directory that was created and do a "git status" there to check.
Edoardo Pasca
That's exactly the same conclusion I came to: all the packages that are git fetched do not behave as expected. Probably for the reason that you mention.
Now I'd like to know which are the exact commit hash I have to use for STIR, Gadgetron and ISMRMRD. The ones I assumed to be correct aren't and I can't go trial-and-error.
Kris Thielemans
git hashes are in my previous comments for version_config.cmake.
Edoardo Pasca
@paskino pushed 1 commit.
- cfc269e Use latest commit for dependency
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
The culprit was the outdated version in the version_config.cmake file for STIR, Gadgetron and ISMRMRD.
Now it does compile and passes the tests and I've run all the python examples. The only bit I had to run manually was to make (copy) a configuration file for gadgetron and source the file to set environment variables.
I've tried to use the hash you gave me for STIR but it wouldn't work. Currently this build works with all the latest code from the CCPPETMR branches, not the upstream.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
a few small changes for consistency...
In SuperBuild/External_Gadgetron.cmake:
> @@ -58,7 +57,7 @@ if (MKL_FOUND)
endif ()
ExternalProject_Add(${proj}
${${proj}_EP_ARGS}
- GIT_REPOSITORY ${${proj}_URL}
+ GIT_REPOSITORY ${GADGETRON_URL}
I'd prefer to use ${${proj}_URL}. Obviously that means that in the version_config.cmake, we need to use the same capitals as here. It's best if we keep this consistent everywhere (too confusing otherwise).
In SuperBuild/External_HDF5.cmake:
> set(proj HDF5)
# Set dependency list
set(${proj}_DEPENDENCIES "")
# Include dependent projects if any
-ExternalProject_Include_Dependencies(${proj} DEPENDS_VAR ${proj}_DEPENDENCIES)
-
-# Set external name (same as internal for now)
can you clarify why this was changed? I struggled at some point with the difference between "external" and "internal" project name. I thought it'd be less confusing to just keep 1. This change doesn't seem to have anything to do with the versioning.
In SuperBuild/External_SIRF.cmake:
> @@ -62,7 +62,7 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr
ExternalProject_Add(${proj}
${${proj}_EP_ARGS}
GIT_REPOSITORY ${${proj}_URL}
- GIT_TAG release-0.9.0
+ GIT_TAG ${SIRF_TAG}
Use ${${proj}_TAG}, so same everywhere.
In SuperBuild/External_googletest.cmake:
> @@ -49,6 +49,7 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr
ExternalProject_Add(${proj}
${${proj}_EP_ARGS}
GIT_REPOSITORY ${${proj}_URL}
+ GIT_TAG ${googletest_TAG}
as before
Kris Thielemans
thanks. this looks nearly ready.
I think it'd be cleaner to actually just remove the lines that you commented out in the External* files. Otherwise we'll have to remove them later. what do you think?
And sorry for the git hash. Using the top of the CCP PETMR forks is fine.
By the way, I intend to "squash and merge" this to generate a single commit, so don't merge this into another branch. Or let me know if that'd be a problem.
Edoardo Pasca
@paskino commented on this pull request.
In SuperBuild/External_HDF5.cmake:
> set(proj HDF5)
# Set dependency list
set(${proj}_DEPENDENCIES "")
# Include dependent projects if any
-ExternalProject_Include_Dependencies(${proj} DEPENDS_VAR ${proj}_DEPENDENCIES)
-
-# Set external name (same as internal for now)
I am not sure why I changed it. I presume I tried to set it to the values it had before I did some changes. I'll put the lines I deleted back.
Edoardo Pasca
@paskino pushed 3 commits.
- b8deedb removed comments
- b8ad3b3 use consistent naming in URL/TAG variables
- 6598e95 Override changes
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
I seem to have found a few more spurious comments...
In SuperBuild.cmake:
> @@ -31,6 +31,8 @@ set(proj ${PRIMARY_PROJECT_NAME})
if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
set(CMAKE_INSTALL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/INSTALL" CACHE PATH "Prefix for path for installation" FORCE)
endif()
+#do it in any case
let's remove these comments.
In SuperBuild/External_FFTW3.cmake:
> @@ -39,21 +39,14 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr
set(FFTW_Configure_Script ${CMAKE_CURRENT_LIST_DIR}/External_FFTW_configure.cmake)
set(FFTW_Build_Script ${CMAKE_CURRENT_LIST_DIR}/External_FFTW_build.cmake)
- set(${proj}_URL http://www.fftw.org/fftw-3.3.5.tar.gz )
- set(${proj}_MD5 6cc08a3b9c7ee06fdd5b9eb02e06f569 )
+ #set(${proj}_URL http://www.fftw.org/fftw-3.3.5.tar.gz )
remove these comments
In SuperBuild/External_SWIG.cmake:
>
set(SWIG_Install_Dir ${SUPERBUILD_INSTALL_DIR})
set(SWIG_Configure_Script ${CMAKE_CURRENT_LIST_DIR}/External_SWIG_configure.cmake)
set(SWIG_Build_Script ${CMAKE_CURRENT_LIST_DIR}/External_SWIG_build.cmake)
- set(${proj}_URL http://prdownloads.sourceforge.net/swig/swig-3.0.12.tar.gz )
- set(${proj}_MD5 82133dfa7bba75ff9ad98a7046be687c )
+# set(${proj}_URL http://prdownloads.sourceforge.net/swig/swig-3.0.12.tar.gz )
remove these
In SuperBuild/External_googletest.cmake:
> @@ -49,6 +49,7 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr
ExternalProject_Add(${proj}
${${proj}_EP_ARGS}
GIT_REPOSITORY ${${proj}_URL}
+ GIT_TAG ${googletest_TAG}
${${proj}_TAG}
Edoardo Pasca
@paskino pushed 4 commits.
- f1492bf removed comments
- c07e802 Consistent use of ${proj}
- a9cf393 Use consistent ${proj} and remove comments
- a580371 Remove comments and fix FFTW3double_URL/TAG variables
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
I waited to rebuild and test on my machine before committing.
I've removed all the dangling comments and hopefully made it more consistent in the use of references to project URL/TAG.
Kris Thielemans
Merged #29.