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.

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)