From deae0c48e44753a97a39660b37ad0fbf025be55e Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Fri, 30 Sep 2022 13:56:53 -0700 Subject: develop: canonicalize dev paths and base relative paths on `env.path` (#30075) Allow environment variables and spack-specific path substitution variables (e.g. `$spack`) to be used in the paths associated with develop specs, while maintaining the ability to keep those paths relative to the environment rather than the working directory. --- lib/spack/spack/cmd/develop.py | 9 ++---- lib/spack/spack/concretize.py | 3 +- lib/spack/spack/environment/environment.py | 2 +- lib/spack/spack/solver/asp.py | 3 +- lib/spack/spack/test/cmd/develop.py | 44 ++++++++++++++++++++++++++++++ lib/spack/spack/util/path.py | 11 ++++++-- 6 files changed, 60 insertions(+), 12 deletions(-) diff --git a/lib/spack/spack/cmd/develop.py b/lib/spack/spack/cmd/develop.py index d95a50cb17..2ca11de322 100644 --- a/lib/spack/spack/cmd/develop.py +++ b/lib/spack/spack/cmd/develop.py @@ -9,6 +9,7 @@ import llnl.util.tty as tty import spack.cmd import spack.cmd.common.arguments as arguments +import spack.util.path from spack.error import SpackError description = "add a spec to an environment's dev-build information" @@ -52,7 +53,7 @@ def develop(parser, args): # download all dev specs for name, entry in env.dev_specs.items(): path = entry.get("path", name) - abspath = path if os.path.isabs(path) else os.path.join(env.path, path) + abspath = spack.util.path.canonicalize_path(path, default_wd=env.path) if os.path.exists(abspath): msg = "Skipping developer download of %s" % entry["spec"] @@ -79,11 +80,7 @@ def develop(parser, args): # default path is relative path to spec.name path = args.path or spec.name - - # get absolute path to check - abspath = path - if not os.path.isabs(abspath): - abspath = os.path.join(env.path, path) + abspath = spack.util.path.canonicalize_path(path, default_wd=env.path) # clone default: only if the path doesn't exist clone = args.clone diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py index 8174855de2..f5e43955ff 100644 --- a/lib/spack/spack/concretize.py +++ b/lib/spack/spack/concretize.py @@ -38,6 +38,7 @@ import spack.repo import spack.spec import spack.target import spack.tengine +import spack.util.path import spack.variant as vt from spack.config import config from spack.package_prefs import PackagePrefs, is_spec_buildable, spec_externals @@ -91,7 +92,7 @@ class Concretizer(object): if not dev_info: return False - path = os.path.normpath(os.path.join(env.path, dev_info["path"])) + path = spack.util.path.canonicalize_path(dev_info["path"], default_wd=env.path) if "dev_path" in spec.variants: assert spec.variants["dev_path"].value == path diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index a27977c956..c3d30bd42d 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -1217,7 +1217,7 @@ class Environment(object): if clone: # "steal" the source code via staging API - abspath = os.path.normpath(os.path.join(self.path, path)) + abspath = spack.util.path.canonicalize_path(path, default_wd=self.path) # Stage, at the moment, requires a concrete Spec, since it needs the # dag_hash for the stage dir name. Below though we ask for a stage diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 51782cf670..af55b757be 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -47,6 +47,7 @@ import spack.platforms import spack.repo import spack.spec import spack.store +import spack.util.path import spack.util.timer import spack.variant import spack.version @@ -2286,7 +2287,7 @@ def _develop_specs_from_env(spec, env): if not dev_info: return - path = os.path.normpath(os.path.join(env.path, dev_info["path"])) + path = spack.util.path.canonicalize_path(dev_info["path"], default_wd=env.path) if "dev_path" in spec.variants: error_msg = ( diff --git a/lib/spack/spack/test/cmd/develop.py b/lib/spack/spack/test/cmd/develop.py index 2c2faaf467..4a4de9465a 100644 --- a/lib/spack/spack/test/cmd/develop.py +++ b/lib/spack/spack/test/cmd/develop.py @@ -102,3 +102,47 @@ class TestDevelop(object): develop("mpich@2.0") self.check_develop(e, spack.spec.Spec("mpich@2.0")) assert len(e.dev_specs) == 1 + + def test_develop_canonicalize_path(self, monkeypatch, config): + env("create", "test") + with ev.read("test") as e: + path = "../$user" + abspath = spack.util.path.canonicalize_path(path, e.path) + + def check_path(stage, dest): + assert dest == abspath + + monkeypatch.setattr(spack.stage.Stage, "steal_source", check_path) + + develop("-p", path, "mpich@1.0") + self.check_develop(e, spack.spec.Spec("mpich@1.0"), path) + + # Check modifications actually worked + assert spack.spec.Spec("mpich@1.0").concretized().satisfies("dev_path=%s" % abspath) + + def test_develop_canonicalize_path_no_args(self, monkeypatch, config): + env("create", "test") + with ev.read("test") as e: + path = "$user" + abspath = spack.util.path.canonicalize_path(path, e.path) + + def check_path(stage, dest): + assert dest == abspath + + monkeypatch.setattr(spack.stage.Stage, "steal_source", check_path) + + # Defensive check to ensure canonicalization failures don't pollute FS + assert abspath.startswith(e.path) + + # Create path to allow develop to modify env + fs.mkdirp(abspath) + develop("--no-clone", "-p", path, "mpich@1.0") + + # Remove path to ensure develop with no args runs staging code + os.rmdir(abspath) + + develop() + self.check_develop(e, spack.spec.Spec("mpich@1.0"), path) + + # Check modifications actually worked + assert spack.spec.Spec("mpich@1.0").concretized().satisfies("dev_path=%s" % abspath) diff --git a/lib/spack/spack/util/path.py b/lib/spack/spack/util/path.py index fe45541321..e5a5e61632 100644 --- a/lib/spack/spack/util/path.py +++ b/lib/spack/spack/util/path.py @@ -314,9 +314,13 @@ def add_padding(path, length): return os.path.join(path, padding) -def canonicalize_path(path): +def canonicalize_path(path, default_wd=None): """Same as substitute_path_variables, but also take absolute path. + If the string is a yaml object with file annotations, make absolute paths + relative to that file's directory. + Otherwise, use ``default_wd`` if specified, otherwise ``os.getcwd()`` + Arguments: path (str): path being converted as needed @@ -335,8 +339,9 @@ def canonicalize_path(path): if filename: path = os.path.join(filename, path) else: - path = os.path.abspath(path) - tty.debug("Using current working directory as base for abspath") + base = default_wd or os.getcwd() + path = os.path.join(base, path) + tty.debug("Using working directory %s as base for abspath" % base) return os.path.normpath(path) -- cgit v1.2.3-70-g09d2