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

meminfo bug: AA no longer supports non-bash shells #191

Closed
jooh opened this Issue Feb 12, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@jooh
Copy link
Collaborator

jooh commented Feb 12, 2019

I have been getting a few user complaints like the below:

Error in aas_shell (line 45)

    aas_log([],stopforerrors,sprintf('***WITH ENVIRONMENT VARIABLES\n%s',wenv));

 

Error in meminfo (line 103)

                [junk, out] = aas_shell('ulimit -m',true); out = deblank(out);

 

Error in aas_convertseries_fromstream (line 80)

    memLimit = meminfo; memLimit = memLimit.ResFree;

 

Error in aamod_convert_structural (line 28)

        [aap, convertedfns, dcmhdr] = aas_convertseries_fromstream(aap, subjInd, inStream);

This happens because ulimit is sh-family, not csh, so it's not available on our standard CBU shells. The meminfo function appears to have been refactored under the assumption that this function is always available.

A possible workaround for now is to set aap.directory_conventions.linuxshell = 'bash'. But reading the code I can see that there are ulimit calls that use system directly rather than the aas_shell wrapper in the ismac block. So I suspect this workaround wouldn't work if you are weird enough to use CSH on a mac...

At a higher level, the complexity we introduce by supporting different options for linuxshell, fslshell, freesurfershell AND also using the odd direct system call should not be underestimated. It might be useful to consider simplifying this by a) simply using bash throughout in AA, b) using whatever the user's $SHELL is set to, and making a best effort to run all our tests twice in the future: once after starting matlab from CSH, and once after starting matlab from Bash.

@jooh

This comment has been minimized.

Copy link
Collaborator Author

jooh commented Feb 12, 2019

Or so I thought. In fact, linuxshell appears to be a dead parameter. As far as I can tell it is never used anywhere. aas_shell does some very weird stuff with the cache to work out what shell to use. Does anyone understand how this code works? Shouldn't it be using linuxshell instead?

@tiborauer

This comment has been minimized.

Copy link
Member

tiborauer commented Feb 12, 2019

I agree that the current approach is convoluted..

I like the idea of using the user's $SHELL. I think that is what aas_shell tries to do. It search for the shell for the first time, then put it into the cache, so it does not have to search next time.

I am not sure, however, whether system depends on user's $SHELL or not.

@jooh

This comment has been minimized.

Copy link
Collaborator Author

jooh commented Feb 12, 2019

Ok. I think a bigger refactor might be in order at some point but for now we just need to avoid assuming that ulimit is available.

@jooh jooh closed this in 0bf9a9f Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.