I've added a file version_config.cmake with the versions of all the packages we build.
https://github.com/CCPPETMR/SIRF-SuperBuild/pull/29
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
@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 option
s (mark_as_advanced
). Not 100% if the DEVEL_BUILD should set STIR et al to master as well.
@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.
@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)
@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!
> set(STIR_TAG stir_rel_3_00) + +## Gadgetron set(GADGETRON_TAG v3.8.2)
to resume upstream should be:
@paskino pushed 6 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
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
See my comments on #30. If necessary, set STIR_DIR
but you must have changed something that breaks this.
@KrisThielemans commented on this pull request.
yes
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)
@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?
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...
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.
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.
git hashes are in my previous comments for version_config.cmake.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
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.
@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
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.
@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.
@paskino pushed 3 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@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}
@paskino pushed 4 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
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.
Merged #29.