Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

env files need to set SIRF_PATH to where data is installed #722

Open
KrisThielemans opened this issue Jun 9, 2022 · 6 comments
Open

env files need to set SIRF_PATH to where data is installed #722

KrisThielemans opened this issue Jun 9, 2022 · 6 comments
Assignees
Milestone

Comments

@KrisThielemans
Copy link
Member

SIRF_PATH=@SIRF_SOURCE_DIR@

but this means we need to have access to the source files.

Should instead be something like ${CMAKE_INSTALL_PREFIX}/share/SIRF-3.2/. see https://github.com/SyneRBI/SIRF-SuperBuild/pull/718/files#r882778578

@KrisThielemans KrisThielemans added this to the v3.3 milestone Jun 9, 2022
@KrisThielemans
Copy link
Member Author

SIRF finds location for install here

@KrisThielemans KrisThielemans modified the milestones: v3.3, v3.4 Jun 9, 2022
@KrisThielemans
Copy link
Member Author

KrisThielemans commented Jun 9, 2022

Ideally we should get rid of the use of SIRF_PATH at all. Related is SyneRBI/SIRF#904 and SyneRBI/SIRF#919

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Jan 16, 2023

as we apparently haven't fixed this yet, we need to keep the sources (in #101).

@paskino, best to postpone this to 3.5 I think

@KrisThielemans
Copy link
Member Author

Pending SyneRBI/SIRF#1201, so postponing

@KrisThielemans
Copy link
Member Author

Likely we can now just remove SIRF_PATH completely.

@KrisThielemans
Copy link
Member Author

SIRF tests still use the SIRF_PATH env variable sadly, see SyneRBI/SIRF#1235, so we cannot remove SIRF_PATH yet. However, there are a few things that probably should be resolved now.

Before going into that, note that it is currently set as

SIRF_PATH=@SIRF_SOURCE_DIR@

Also, I believe that in the new docker env, we don't have the source anymore, so SIRF_PATH will point to a non-existent path I suppose.

optionally fix now

I found a few places where SIRF_PATH is used to find the location of the gadgetron executable, but they should all use SIRF_INSTALL_PATH instead

SB_PATH=$(dirname "$(dirname "$SIRF_PATH")")
echo "start gadgetron"
pushd "${SB_PATH}"
test -x ./INSTALL/bin/gadgetron && ./INSTALL/bin/gadgetron >& ~/gadgetron.log&

$SIRF_PATH/../../INSTALL/bin/gadgetron >& ~/gadgetron.log&

pushd $(dirname "$(dirname "$SIRF_PATH")")
# start gadgetron
[ -f ./INSTALL/bin/gadgetron ] && ./INSTALL/bin/gadgetron >& gadgetron.log&

Note that these 2 VM scripts should use another way to find the location of the sources after we remove SIRF_PATH

if [ -z "$SIRF_PATH" -o ! -d $SIRF_PATH -o ! -d $SIRF_PATH/../STIR ]; then
echo "Directories not found. Run update_VM.sh"

if [ -z "$SIRF_PATH" -o ! -d $SIRF_PATH -o ! -d $SIRF_PATH/../STIR ]; then
echo "Directories not found. Run update_VM.sh"

fix later

I found 2 locations in the doc referring to $SIRF_PATH which will need to be adapted

cd $SIRF_PATH

long term problem?

By the way, as the docker images don't contain the build files either, I guess we don't run any SIRF tests in them (which isn't great). Is that correct, @casperdcl ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants