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

Rose mpi launch #2067

Merged
merged 2 commits into from Apr 19, 2017
Merged

Rose mpi launch #2067

merged 2 commits into from Apr 19, 2017

Conversation

steoxley
Copy link
Contributor

@matthewrmshin I have tested this in my own space and with:

rose test-battery t/rose-mpi-launch

and also demonstrated that without the fix the modified test fails. I decided to use file size (ulimit -f) rather than stack size as that is the default value that -H or -S reports. In practice -a is the option we would normally use, but that produces many lines of output and would have been harder to create a control.

@scwhitehouse
Copy link
Contributor

I'm unsure what the motivation for this change is, can someone explain why the change is necessary?

@steoxley
Copy link
Contributor Author

Apologies, most of the initial work and reasoning was communicated by email. The particular use case for this option, as described in the header of the script, is:

ROSE_LAUNCHER_ULIMIT_OPTS="-a -s unlimited -d unlimited -a"

This is meant to run the following commands:

ulimit -a
ulimit -s unlimited
ulimit -d unlimited
ulimit -a

i.e. show the current settings, change various settings, show the new settings.

Unfortunately the code as it stands results in:

ulimit -a $OPTARG

for ulimit arguments without a value (HSa). Since $OPTARG is not defined in these cases, the set -u in lib/bash/rose_init brings down the script. I believe set -u is a relatively new addition to this script and why this problem has only recently been identified when a user tried to use this capability.

Copy link
Contributor

@scwhitehouse scwhitehouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks reasonable to me!

@matthewrmshin matthewrmshin merged commit ce63ef7 into metomi:master Apr 19, 2017
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.

None yet

3 participants