bids-apps / SPM

BIDS App containing an instance of the SPM software.
https://hub.docker.com/r/bids/spm/tags
Apache License 2.0
14 stars 12 forks source link

Fix: don't use fileread() directly in / #21

Open octomike opened 4 years ago

octomike commented 4 years ago

Matlab/MCR is doing a weird thing (there should be a name for it) when using fileread() on a file directly in /, which results in the -v/--version switch to output garbage on the terminal.

Minimal example:

docker run --entrypoint /bin/bash -it --rm bids/spm:v0.0.20 -c "echo -e \"fileread('/version')\nquit\" > test.m ; /opt/spm12/run_spm12.sh /opt/mcr/v97 script test"
------------------------------------------
Setting up environment variables
---
LD_LIBRARY_PATH is .:/opt/mcr/v97/runtime/glnxa64:/opt/mcr/v97/bin/glnxa64:/opt/mcr/v97/sys/os/glnxa64:/opt/mcr/v97/sys/opengl/lib/glnxa64
SPM12, version 7771 (standalone)
MATLAB, version 9.7.0.1319299 (R2019b) Update 5
 ___  ____  __  __                                            
/ __)(  _ \(  \/  )                                           
\__ \ )___/ )    (   Statistical Parametric Mapping           
(___/(__)  (_/\/\_)  SPM12 - https://www.fil.ion.ucl.ac.uk/spm/

ans =

    'V1MCC4000MEC1000MCR1000x  ���V=�W���i�{��H �k�����G������w������;B�+��]v����
[...]

However, adding a second / resulting in //version, everything works out again:

docker run --entrypoint /bin/bash -it --rm bids/spm:v0.0.20 -c "echo -e \"fileread('//version')\nquit\" > test.m ; /opt/spm12/run_spm12.sh /opt/mcr/v97 script test"
------------------------------------------
Setting up environment variables
---
LD_LIBRARY_PATH is .:/opt/mcr/v97/runtime/glnxa64:/opt/mcr/v97/bin/glnxa64:/opt/mcr/v97/sys/os/glnxa64:/opt/mcr/v97/sys/opengl/lib/glnxa64
SPM12, version 7771 (standalone)
MATLAB, version 9.7.0.1319299 (R2019b) Update 5
 ___  ____  __  __                                            
/ __)(  _ \(  \/  )                                           
\__ \ )___/ )    (   Statistical Parametric Mapping           
(___/(__)  (_/\/\_)  SPM12 - https://www.fil.ion.ucl.ac.uk/spm/

ans =

    'v0.0.20-2-gd739e10
     '

This trivial PR puts version in SPM_DIR, as I couldn't find words for a comment explaining fileread('//version')

gllmflndn commented 4 years ago

This is indeed very odd - it seems to return the (compiled) content of a file version.m stored elsewhere in the filesystem:

root:/# /opt/spm12/run_spm12.sh /opt/mcr/v97 eval "which('/version','-all')"
SPM12, version 7771 (standalone)
MATLAB, version 9.7.0.1319299 (R2019b) Update 5
 ___  ____  __  __                                            
/ __)(  _ \(  \/  )                                           
\__ \ )___/ )    (   Statistical Parametric Mapping           
(___/(__)  (_/\/\_)  SPM12 - https://www.fil.ion.ucl.ac.uk/spm/

/opt/mcr/v97/mcr/toolbox/matlab/codetools/@mtree/version.m  % mtree method

version is also a built-in MATLAB command. I would prefer to fix the root cause of the issue (as it used to work with previous versions of the MCR) and thought that /version was expected to be found in BIDS Apps but it doesn't seem to be the case in prominent apps: https://github.com/poldracklab/fmriprep/blob/8ea6db905d4b0a86ab65b3e8f8213e29f70e27c0/Dockerfile#L184-L186 so I am OK to use /opt/spm12/VERSION. Two further comments:

octomike commented 4 years ago

It would be preferable if spm_BIDS_App.m was also working outside of a docker container

Sure, I can catch this and amend the PR.

The content of VERSION should also probably be the SHA-1 checksum of the latest commit, i.e. relying on ARG as in:

I'm a bit puzzled here. When and where should the hash be written, exactly? Wouldn't this only work in the Dockerfile (assuming it is always build from within a git repo) or circleCI. Otherwise, how would you persist the hash in the repo?

gllmflndn commented 4 years ago

Thanks! If I'm not mistaken, there wouldn't be any VERSION file in this repository but an environment variable would be passed on at build time (docker build ... --build-arg VERSION=XXX). We would then need to be robust in spm_BIDS_App.m if VERSION file does not exist (not in a Docker environment) or is empty (the variable was not defined during build).