Skip to content

Conversation

@giordano
Copy link
Contributor

@giordano giordano commented Aug 11, 2021

This removes the need to set the SPACK_ROOT environment variable. Fixes #2097.

I'm not sure how to test this though as it requires a working Spack to generate
the script.

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@giordano
Copy link
Contributor Author

I guess the obvious question is: why don't you do

cmds = ['. $(spack location --spack-root)/share/spack/setup-env.sh']

? The answer is because I'm not sure that'd work on my system: the problem I was seeing in #2097 is that SPACK_ROOT was set in the login node, but not on the compute one, despite the fact it is in my .bashrc. Likewise, spack would be in the PATH on the login node but not on the compute one. However I'll need to do some more investigation about whether using $(spack location --spack-root) would work. I think that not requiring to set the variable SPACK_ROOT would be good anyways though.

@vkarak vkarak changed the title Automatically detect location of Spack [feat] Automatically detect location of Spack Aug 11, 2021
@giordano giordano force-pushed the mg/spack-root branch 2 times, most recently from 4d60abe to 7eecfc7 Compare August 13, 2021 13:17
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2021

Codecov Report

Merging #2123 (d735799) into master (91d5797) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2123   +/-   ##
=======================================
  Coverage   85.97%   85.97%           
=======================================
  Files          53       53           
  Lines        9362     9362           
=======================================
  Hits         8049     8049           
  Misses       1313     1313           
Impacted Files Coverage Δ
reframe/core/buildsystems.py 96.20% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91d5797...d735799. Read the comment docs.

@giordano
Copy link
Contributor Author

Ok, I finally figured out what was my trouble in #2097: the job scripts had

#!/bin/bash

as shebang, so my init file wasn't loaded. The solution was to use the RFM_USE_LOGIN_SHELL environment variable. With this, SPACK_ROOT is now set, and spack is in the path.

This pull request replaces $SPACK_ROOT with spack location --spack-root, since we require spack to be in the PATH anyways, but Spack doesn't require SPACK_ROOT to be set

This removes the need to set the `SPACK_ROOT` environment variable.
@teojgo
Copy link
Contributor

teojgo commented Aug 13, 2021

Ok to test

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Lgtm. I'm not sure about whether we need the quotes or not.

@vkarak vkarak merged commit 152d697 into reframe-hpc:master Aug 19, 2021
@giordano giordano deleted the mg/spack-root branch August 19, 2021 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatic handling of SPACK_ROOT?

5 participants