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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

octomike
Copy link

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')

In an MCR environment directly reading a file from / fails, work around
that.
@gllmflndn
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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).

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

Successfully merging this pull request may close these issues.

2 participants