-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
make SPACK_SHELL detection more robust #9712
Conversation
44c9906
to
e91c0f9
Compare
e91c0f9
to
9b3f23b
Compare
9b3f23b
to
fa92bcb
Compare
It looks like the script here is from a GPL project, so I'm a little worried about including it verbatim (though I agree it's very short). Looks like @xdelaruelle is the original author -- @xdelaruelle, are you ok with adding this snippet to Spack as Apache-2.0/MIT? |
@tgamblin Sure no problem |
fa92bcb
to
e9c68cc
Compare
I've been investigating the error this reports to solve and I can replicate it on a Cray machine but not on Linux/Mac. The behavior of From my experiments, replacing @carns Do you know of any command on Cray that replicates the behavior of
that being said, based on the above, I'm not sure if this is the best approach (i.e. based on my reading of |
I attached a test case that digs into this a little more carefully. You can download it, rename it to spack-shell-test.sh, and adjust the path to setup-env.sh to run it. It takes one command line argument to control whether it invokes setup-env.sh via "." "source" or simply running it with "/bin/bash". That third option isn't helpful because the caller won't inherit the spack variables in that case; I just included it for a sanity check. On a plain Ubuntu 18.10 laptop, I get the following results from running it each way, against origin/develop a76c50d:
If I prefix the test script with /bin/bash then I get this:
So on this machine (just a normal Linux box, not HPC or Cray), it doesn't matter whether you use . or source, but explicitly running the script with /bin/bash works around the problem. I still think it warrants a fix even for this kind of machine, though, because some schedulers and CI environments will directly execute scripts without an explicit shell command. This example shows that you could work around it by having your job script just relay to a second script using an explicit bash invocation, but it's confusing to have to do that. At any rate, I'm happy to adjust the commit message or PR description if we can make it more clear, but I don't believe that this is a Cray-specific problem. |
e9c68cc
to
9dcf023
Compare
FYI, I updated the source branch for this PR just now, but there is no change to the commit in question. I just needed to rebase to pick up unrelated recent commits from origin/develop. |
Thanks for checking that, I was mistaken about reproducing my results on Linux, I could only get that on Mac. I looked into it more and I was getting confused about how the executable name is set based on the presence of
So this is a more-robust approach for Linux as well and good to merge (and now that I understand why it works I can create a sensible commit message). |
Thanks! |
Great, thanks @scheibelp . I've confirmed that the current origin/develop branch of spack is working great within our regression jobs now. |
See discussions in #7475 and #8443. This patch attempts to make setup-env's shell detection work correctly when it is sourced within a shell script environment. The target use cases are continuous integration environments or job scheduler scripts.
Test case:
Without this PR, it produces the following output:
With this PR applied, it produces the following output: