Make mirrors use spack environment expansions#9027
Conversation
lib/spack/spack/util/path.py
Outdated
|
|
||
|
|
||
| def canonicalize_path(path): | ||
| def canonicalize_path(path, make_abs=True): |
There was a problem hiding this comment.
instead of adding an optional argument, factor out substitute_variables() (which does everything but the absolute path) and make canonicalize_path() call that.
lib/spack/spack/stage.py
Outdated
| # urljoin() will strip everything past the final '/' in | ||
| # the root, so we add a '/' if it is not present. | ||
| mirror_roots = [root if root.endswith('/') else root + '/' | ||
| mirror_roots = [canonicalize_path(root, make_abs=False) |
There was a problem hiding this comment.
Adding make_abs=False just in this spot isn't particularly readable. also, why call it twice like this? DRY... use a variable.
There was a problem hiding this comment.
More importantly, if this is a relative path, what's it relative to, and is that documented somewhere?
This is coming from the mirror config, and the working directory for fetchers is the stage path. But the stage could be anywhere (the stage location is user configurable), so a relative path from there is not reliable. If it's relative to anything, it should probably be relative to the config file it came from, or to the spack installation, or something predictable that a user would know about. It seems like to me that this will get really confusing for users.
There was a problem hiding this comment.
It's not a relative path, it's a url and when we try to turn urls into absolute paths we get garbage.
file:///path/to/mirror becomes /path/to/spack/file:///path/to/mirror.
There was a problem hiding this comment.
The refactoring you suggested should take care of the legibility issue, though.
|
@tgamblin comments addressed. |
tgamblin
changed the title
tgamblin
force-pushed
the
bugfix/mirrors_use_path_expansions
branch
3 times, most recently
from
180f5cb to
d743028
Compare
tgamblin
force-pushed
the
bugfix/mirrors_use_path_expansions
branch
from
d743028 to
70729c0
Compare
- ensure that `$spack` and other variables are substituted into mirror paths
tgamblin
force-pushed
the
bugfix/mirrors_use_path_expansions
branch
from
70729c0 to
e39aa79
Compare
…upsream_develop * commit 'f7026a058b63f5a3109691e2c3871ee77c08f756': (1881 commits) Version 19.8.1 of PLASMA (spack#12299) new package: py-exodus (spack#12291) ncurses: fix pic and opt flags (spack#12272) pumi: new version 2.2.1 (spack#12282) tests: explain and test dependency flattening routines (spack#11993) graphviz package: add MacOS fixes and quartz support (spack#11128) Overhaul numpy package (spack#12170) mirrors: mirror config should use spack variable expansions (spack#9027) stacks: fix reference handling in env.write() (spack#12096) fltk: fix about variable types (spack#12292) Avoid sending empty reports to codecov (spack#12293) Packages/musl (spack#12288) c-blosc package: Add -std=gnu99 flag for gcc (spack#11959) Move new packages from tutorial to builtin (spack#12289) Balay/amrex 19.08 (spack#12287) openPMD-api: pre-load depend libs (spack#12279) Add version 19.8.0 of PLASMA (spack#12275) Add version 2.5.1 of MAGMA released today (spack#12274) ginkgo: add maintainers (spack#12273) new package: py-backports-tempfile (spack#12261) ... # Conflicts: # .travis.yml # var/spack/repos/builtin/packages/moab/package.py # var/spack/repos/builtin/packages/mofem-cephas/package.py # var/spack/repos/builtin/packages/mofem-fracture-module/package.py # var/spack/repos/builtin/packages/mofem-users-modules/package.py # var/spack/repos/builtin/packages/petsc/package.py
- ensure that `$spack` and other variables are substituted into mirror paths
3 participants
Previously, it was not possible to use environment variables or spack config variables in mirror paths. With this PR you can.
E.g.
mirrors.yaml