Uses CTest to run the tests that are found in the SIRF/src/x*/p*/tests directories.
Creates the .sirfrc file which can be sourced to set up all the environmental variables needed to run SIRF.
https://github.com/CCPPETMR/SIRF-SuperBuild/pull/32
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
The build doesn't work because of these errors that are unrelated to the commits:
The command "pip${PYVER} install --upgrade pip setuptools wheel" failed and exited with 127 during .
The command "pip${PYVER} install --upgrade pip setuptools wheel" failed and exited with 2 during .
the python2 version fails because of user permissions. seems we need
pip${PYVER} install --user --upgrade pip setuptools wheel
The python3 version fails with
$ pip${PYVER} install --upgrade pip setuptools wheel
pyenv: pip3: command not found
The `pip3' command exists in these Python versions:
3.5
3.5.3
Possibly we need
PYENV_VERSION=3.5
or something, although that's a bit horrible as it depends on Travis.
see e.g.
https://github.com/praekeltfoundation/travis-pyenv/blob/develop/.travis.yml
Hopefully #33 fixes this issue
@paskino, please merge with master to get @casperdcl's fix to .travis.yml
@paskino pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
It now builds correctly.
@casperdcl commented on this pull request.
would prefer append to .bashrc
rather than creation of .sirfrc
, but whatever...
In SuperBuild.cmake:
> @@ -92,3 +92,31 @@ set(SIRF_SRC_PATH ${CMAKE_CURRENT_LIST_DIR}/SIRF) set(CCPPETMR_INSTALL ${SUPERBUILD_INSTALL_DIR}) configure_file(env_ccppetmr.sh.in ${CCPPETMR_INSTALL}/bin/env_ccppetmr.sh) configure_file(env_ccppetmr.csh.in ${CCPPETMR_INSTALL}/bin/env_ccppetmr.csh) + +# Creates sirfrc and appends to the bashrc + +if (EXISTS $ENV{HOME}/.sirfrc) + # copy new version of the env veriables to the file + configure_file(env_ccppetmr.sh.in $ENV{HOME}/.sirfrc) +else() + # create .sirfrc + configure_file(env_ccppetmr.sh.in $ENV{HOME}/.sirfrc) + # append .sirfrc to .bashrc +# file(APPEND $ENV{HOME}/.bashrc "#Environment variables for SIRF +#source ~/.sirfrc") +endif()
if-else
statement not currently used
In SuperBuild.cmake:
> + +# Creates sirfrc and appends to the bashrc + +if (EXISTS $ENV{HOME}/.sirfrc) + # copy new version of the env veriables to the file + configure_file(env_ccppetmr.sh.in $ENV{HOME}/.sirfrc) +else() + # create .sirfrc + configure_file(env_ccppetmr.sh.in $ENV{HOME}/.sirfrc) + # append .sirfrc to .bashrc +# file(APPEND $ENV{HOME}/.bashrc "#Environment variables for SIRF +#source ~/.sirfrc") +endif() +message("To set up all the environmental variables for SIRF, just type: source ~/.sirfrc") + +configure_file(env_ccppetmr.csh.in ${CCPPETMR_INSTALL}/bin/env_ccppetmr.csh)
surely we also need:
configure_file(env_ccppetmr.sh.in ${CCPPETMR_INSTALL}/bin/env_ccppetmr.sh)
@KrisThielemans requested changes on this pull request.
This has a few problems I believe.
.sirfrc will now use sh syntax, which means that it cannot be sourced by csh. I think this means we should not have .sirfrc anymore, but just rely on the env_ccppetmr.sh.* files and tell the user to source the appropriate one.
we cannot really have bash scripts for the test as that will fail on systems without bash, and in particular windows. As far as I can see, the bash scripts are there only to give a nicer message when gadgetron isn't running. That is something for SIRF itself really (I know @evgueni-ovtchinnikov struggles with this as it can also time-out, but fine).
So, the test command should just call ${PYTHON_EXECUTABLE} as far as I can see.
@casperdcl, we don't want to append to .bashrc, as discussed elsewhere. it wouldn't work for anyone who deson't run bash anyway.
I was referring to appending . env_ccppetmr.sh
to .bashrc
, and source env_ccppetmr.csh
for .cshrc
etc
ok. we can instruct the user to do that. let's not do it as part of our superbuild.
I think the tests should be added in SIRF, not SIRF-Superbuild.
@paskino pushed 7 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
I agree that the test should be also on SIRF.
Now ${PYTHON_EXECUTABLE} is used as suggested by @KrisThielemans
I’ve managed to use TEST_AFTER_INSTALL for the external project SIRF in order to use the tests of SIRF rather than in to the SuperBuild as @KrisThielemans suggested.
However, it does not do what we want for a number of reasons:
I guess that what we want is that the SIRF tests pass. We’ve agreed –I believe— that the user should be responsible to run Gadgetron. So the only viable way I see is that we keep the tests both in the Superbuild and in SIRF. (Actually the Superbuild tests just link to the test files in SIRF.)
So the ‘make test’ phase IMO must happen in the SuperBuild in 3 steps :
Do you see any other options?
From what I've read, it seems that if you trigger a process (e.g. gadgetron
) from CMake via execute_process
, then it will wait (or timeout) for a response. This isn't useful to us as we want to keep gadgetron
alive until we've run the test. However, maybe we could build a small application/script that forks and detaches gadgetron
and returns a status value, then run the tests and kills gadgetron
. In principle on Windows you could (in C++) mimic the behaviour of fork()
with CreateProcess()
. Any thoughts? Too much work?
Seems too much effort. @paskino suggested order
seems fine (with additional step of source the env files). In some sense, it's a more complete test: does the Gadgetron config work. It also prevents trouble on Windows where we cannot build Gadgetron anyway.
If we are happy with the 3 steps solution, we could merge the PR.
somewhat confused now. Once ctest
is enabled for SIRF, do we need this here? If we use TEST_COMMAND
(as opposed to TEST_AFTER_INSTALL
, wouldn't we be able to follow the suggested workflow, while not having to duplicate the test here?
IMO the tests should be both in the SuperBuild and in SIRF. Imagine you are a user, you build the superbuild and then make test
. Imagine you are a developer, you change something in SIRF and then you make test
.
yes, agreed. but we cannot have duplicate code. So, we have them in SIRF, and somehow tell the superbuild do to "make test" in SIRF when you do "make test" in the SuperBuild. (same for gadgetron etc of course). I thought TEST_COMMAND
might do the trick.
Alternative option might be to add a target to the SuperBuild CMake called test_SIRF
and let that do its stuff.
@paskino pushed 10 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@KrisThielemans approved this pull request.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
perfect.
can you then add something in the README.md and also modify .travis.yml to use this as opposed to the current manual lines.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@paskino pushed 3 commits.
The build fails with this log message
-- Installing: /home/travis/build/CCPPETMR/SIRF-SuperBuild/INSTALL/share/gadgetron/chroot/nvidia-copy.sh
[ 97%] �[34m�[1mCompleted 'Gadgetron'�[0m
[ 97%] Built target Gadgetron
make: *** [all] Error 2
travis_time:end:283247ae:start=1506335531288183999,finish=1506336568000020844,duration=1036711836845
�[0K
�[31;1mThe command "make -j 6" failed and exited with 2 during .�[0m
Your build has been stopped.
Sincerely I don't see what failed.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Probably related to line 84 of .travis.yml:
- make -j 6
My guess is this consumes too many resources are stops. I'd suggest amending the line to just make
and see if this fixes the problem.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
might then time-out, but worth a try.
you can also just rerun the job to see if it goes through 2nd time.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
normally I use -j2 it speeds up download time and it's not too resource hungry.
Currently building single thread.
@paskino pushed 1 commit.
—
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.
thanks! A few more...
In .travis.yml:
> @@ -81,15 +81,14 @@ install: - #pip install --upgrade pip setuptools wheel - pip install --only-binary=numpy,scipy,matplotlib numpy scipy matplotlib - ./cmake-*/bin/cmake $BUILD_FLAGS $EXTRA_BUILD_FLAGS . - - make -j 6 + - make - mv INSTALL/share/gadgetron/config/gadgetron.xml.example INSTALL/share/gadgetron/config/gadgetron.xml - source $PWD/INSTALL/bin/env_ccppetmr.sh - #env script: - python ./SIRF/src/xSTIR/pSTIR/tests/test1.py
should this not be deleted?
In .travis.yml:
> - mv INSTALL/share/gadgetron/config/gadgetron.xml.example INSTALL/share/gadgetron/config/gadgetron.xml - source $PWD/INSTALL/bin/env_ccppetmr.sh - #env script: - python ./SIRF/src/xSTIR/pSTIR/tests/test1.py - ./INSTALL/bin/gadgetron >& gadgetron.log& - - python ./SIRF/src/xGadgetron/pGadgetron/tests/fully_sampled.py - - python ./SIRF/src/xGadgetron/pGadgetron/tests/undersampled.py + - ctest
I like your -verbose
option. let's add it here.
In README.md:
> +1/1 Test #1: SIRF_TESTS ....................... Passed 9.70 sec + +100% tests passed, 0 tests failed out of 1 + +Total Test time (real) = 9.70 sec +``` + +The user may also run the SIRF tests independently of the SuperBuild. Just enter the SIRF build directory and launch ctest: + +```bash +cd SIRF-prefix/src/SIRF-build +ctest +``` +## Build of specific versions + +The SuperBuild allows the user to change the versions of the projects it's building. This is done at the configuration stage that happens when you issue the `cmake` command in the root of the Superbuild.
split line.
Note that cmake
doesn't have to be run in the root. It's cleaner to do out-of-source builds
git clone ...
mkdir build
cd build
cmake ../SIRF-SuperBuild
so, adjust text here (I'd just remove the "in the root of the SuperBuild"), and probably add a note on the instructions elsewhere
In README.md:
> +100% tests passed, 0 tests failed out of 1 + +Total Test time (real) = 9.70 sec +``` + +The user may also run the SIRF tests independently of the SuperBuild. Just enter the SIRF build directory and launch ctest: + +```bash +cd SIRF-prefix/src/SIRF-build +ctest +``` +## Build of specific versions + +The SuperBuild allows the user to change the versions of the projects it's building. This is done at the configuration stage that happens when you issue the `cmake` command in the root of the Superbuild. + +There is a `DEVEL_BUILD` tag that allows to build the upstream/master versions of all packages
insert something like "By default, the SuperBuild will build the latest stable release of SIRF and associated versions of the dependencies."
Also, can we somehow say that the table contains hashes etc for release 0.9.0?
In README.md:
> +|`SIRF_TAG` | `v0.9.0` | `master` | +|`STIR_URL` | https://github.com/CCPPETMR/STIR | https://github.com/UCL/STIR | +|`STIR_TAG` | `8bf37d9d7fdde7cb3a98a6f848d93827dbd98a18` | `master` | +|`Gadgetron_URL` | https://github.com/CCPPETMR/gadgetron |https://github.com/gadgetron/gadgetron | +|`Gadgetron_TAG` | `f03829ef45e57466829e6ec46da7a7cf61db1c8a` | `master` | +|`ISMRMRD_URL` | https://github.com/CCPPETMR/ismrmrd | https://github.com/ismrmrd/ismrmrd | +|`ISMRMRD_TAG` | `35012c6c8000616546c2d6b1757eba0c5b21b2d4` | `master` | + +To use the `DEVEL_BUILD` option one may (on the terminal) + +```bash + +cmake . -DDEVEL_BUILD=ON +``` + +Additionally one may want to use only a specific version of a package. This is achieved by adding the right tag to the command line (see the table above for available tags):
split line and add something like. "Note that the CMake options in the table are Advanced Options. When running the CMake GUI (or ccmake
) they will therefore only be visible when you toggle those on."
In SuperBuild/External_SIRF.cmake:
> @@ -82,6 +82,7 @@ if(NOT ( DEFINED "USE_SYSTEM_${externalProjName}" AND "${USE_SYSTEM_${externalPr INSTALL_DIR ${SIRF_Install_Dir} DEPENDS ${${proj}_DEPENDENCIES} + TEST_AFTER_INSTALL 0
delete?
> @@ -11,3 +11,6 @@ setenv LD_LIBRARY_PATH @CCPPETMR_INSTALL@/lib:$LD_LIBRARY_PATH setenv DYLD_FALLBACK_LIBRARY_PATH @CCPPETMR_INSTALL@/lib:$DYLD_FALLBACK_LIBRARY_PATH setenv PYTHONPATH @CCPPETMR_INSTALL@/python:$PYTHONPATH set path=( $path @CCPPETMR_INSTALL@/bin ) + +# set nano as editor
we cannot do this here. we should only do SIRF stuff, not our preferences. (This is for the VM only)
> @@ -18,3 +18,6 @@ PYTHONPATH=@CCPPETMR_INSTALL@/python:$PYTHONPATH export PYTHONPATH PATH=$PATH:@CCPPETMR_INSTALL@/bin +
as above
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
@paskino commented on this pull request.
In README.md:
> +1/1 Test #1: SIRF_TESTS ....................... Passed 9.70 sec + +100% tests passed, 0 tests failed out of 1 + +Total Test time (real) = 9.70 sec +``` + +The user may also run the SIRF tests independently of the SuperBuild. Just enter the SIRF build directory and launch ctest: + +```bash +cd SIRF-prefix/src/SIRF-build +ctest +``` +## Build of specific versions + +The SuperBuild allows the user to change the versions of the projects it's building. This is done at the configuration stage that happens when you issue the `cmake` command in the root of the Superbuild.
but issuing make
must be done in the SIRF-SuperBuild directory, or not?
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
@KrisThielemans commented on this pull request.
In README.md:
> +1/1 Test #1: SIRF_TESTS ....................... Passed 9.70 sec + +100% tests passed, 0 tests failed out of 1 + +Total Test time (real) = 9.70 sec +``` + +The user may also run the SIRF tests independently of the SuperBuild. Just enter the SIRF build directory and launch ctest: + +```bash +cd SIRF-prefix/src/SIRF-build +ctest +``` +## Build of specific versions + +The SuperBuild allows the user to change the versions of the projects it's building. This is done at the configuration stage that happens when you issue the `cmake` command in the root of the Superbuild.
no. make
is always where cmake
created the build files (i.e. the Makefiles in this case)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
hi. presumably failing because of sourceforge outage now. But in any case, I think there's a few comments for the README outstanding?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
@paskino pushed 1 commit.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
sourceforge still down. I did merge SIRF ctest so this will actually get tested once it's up again. But there are a few comments for the README outstanding :-;
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
3 jobs ok. The 3rd them failed in the ctest
step.
- python: 3.5
env: EXTRA_BUILD_FLAGS="-DUSE_SYSTEM_HDF5=ON -DUSE_SYSTEM_FFTW3=ON" CC=gcc-5 CXX=g++-5
- python: 2.7
env: EXTRA_BUILD_FLAGS="-DUSE_SYSTEM_SWIG=ON -DUSE_SYSTEM_HDF5=ON -DUSE_SYSTEM_FFTW3=ON" CC=gcc-5 CXX=g++-5
- python: 3.5
env: EXTRA_BUILD_FLAGS="-DUSE_SYSTEM_HDF5=OFF -DUSE_SYSTEM_FFTW3=OFF -DDEVEL_BUILD=ON" CC=gcc-5 CXX=g++-5
- python: 2.7
env: EXTRA_BUILD_FLAGS="-DUSE_SYSTEM_SWIG=OFF -DUSE_SYSTEM_HDF5=OFF -DUSE_SYSTEM_FFTW3=OFF -DDEVEL_BUILD=ON" CC=gcc-5 CXX=g++-5
Here's the log. https://travis-ci.org/CCPPETMR/SIRF-SuperBuild/jobs/280751611/config
Build succeeded but all 3 tests failed. I don't think it did anything at all with Gadgetron, so it's probably a python config thing. No idea why.
This brings us to getting useful output from the ctest when run on Travis. Possibly there's something useful in Testing/Temporary/LastTest.log. That might sit in this case inside the SIRF-build. Anyone any ideas on how to get that out from Travis (or typed out to the screen if the test failed)?
Yes, let's add a cat LastTest.log
@paskino pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
The reason of it failing is
ImportError: No module named 'numpy'
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
@casperdcl , I guess this has to do Python environments and pip not being the same env as python. can you help?
It also says
Command: "/usr/bin/python3.4" "test1.py"
Which is wrong as it should be:
Command: "/home/travis/virtualenv/python3.5.3/bin/python" "test1.py"
as this is the python environment installed and activated by travis
Should the environment variables be sourced?
It seems that in the install section there is source $PWD/INSTALL/bin/env_ccppetmr.sh
, but there's no one in the script section.
as far as I understand, if you source once, it holds then for the rest of the travis script (the "install" and "script" etc sections are (I think) only used to give you a nice log). In any case, this is not relevant to this problem: we don't set anything python specific in the env_ccppetmr.*sh
scripts (and gadgetron etc would have failed to run).
I think @casperdcl is correct that CMake's
include(FindPythonInterp)
tries to use its own logic and not necessarily honours the virtualenv. There's some reports in the internet about this I believe (but I haven't researched this). it's apparently especially a problem on MacOSX (which we are currently already working around). see e.g. pybind/pybind11#99. sigh.
It'd need some investigation of the cmake module to see if we can do something like
'''
cmake -DPYTHON_EXECUTABLE:FILEPATH=${PYTHON_EXECUTABLE}`
If so, we'd have to pass this frm the Superbuild to SIRF.
This is not an issue with this branch though. I therefore suggest that @paskino fixes the remaining README issues and that we merge.
a 2014 thread with a suggestion
https://public.kitware.com/pipermail/cmake/2014-August/058368.html
rather amazing problem, but might be worth checking if it isn't fixed a more recent CMake.
Merged #32.