Expand relative dev paths in environment files (#22045)

* Rewrite relative dev_spec paths internally to absolute paths in case of relocation of the environment file

* Test relative paths for dev_path in environments

* Add a --keep-relative flag to spack env create

This ensures that relative paths of develop paths are not expanded to
absolute paths when initializing the environment in a different location
from the spack.yaml init file.

6 files changed, 138 insertions, 19 deletions

diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py
index 224b74bdeb..11b422a9bd 100644
--- a/lib/spack/spack/cmd/env.py
+++ b/lib/spack/spack/cmd/env.py
@@ -157,6 +157,10 @@ def env_create_setup_parser(subparser):
     subparser.add_argument(
         '-d', '--dir', action='store_true',
         help='create an environment in a specific directory')
+    subparser.add_argument(
+        '--keep-relative', action='store_true',
+        help='copy relative develop paths verbatim into the new environment'
+             ' when initializing from envfile')
     view_opts = subparser.add_mutually_exclusive_group()
     view_opts.add_argument(
         '--without-view', action='store_true',
@@ -184,13 +188,14 @@ def env_create(args):
     if args.envfile:
         with open(args.envfile) as f:
             _env_create(args.create_env, f, args.dir,
-                        with_view=with_view)
+                        with_view=with_view, keep_relative=args.keep_relative)
     else:
         _env_create(args.create_env, None, args.dir,
                     with_view=with_view)
 
 
-def _env_create(name_or_path, init_file=None, dir=False, with_view=None):
+def _env_create(name_or_path, init_file=None, dir=False, with_view=None,
+                keep_relative=False):
     """Create a new environment, with an optional yaml description.
 
     Arguments:
@@ -199,15 +204,18 @@ def _env_create(name_or_path, init_file=None, dir=False, with_view=None):
             spack.yaml or spack.lock
         dir (bool): if True, create an environment in a directory instead
             of a named environment
+        keep_relative (bool): if True, develop paths are copied verbatim into
+            the new environment file, otherwise they may be made absolute if the
+            new environment is in a different location
     """
     if dir:
-        env = ev.Environment(name_or_path, init_file, with_view)
+        env = ev.Environment(name_or_path, init_file, with_view, keep_relative)
         env.write()
         tty.msg("Created environment in %s" % env.path)
         tty.msg("You can activate this environment with:")
         tty.msg("  spack env activate %s" % env.path)
     else:
-        env = ev.create(name_or_path, init_file, with_view)
+        env = ev.create(name_or_path, init_file, with_view, keep_relative)
         env.write()
         tty.msg("Created environment '%s' in %s" % (name_or_path, env.path))
         tty.msg("You can activate this environment with:")
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 9228c66181..fbd9a7908b 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -73,9 +73,7 @@ class Concretizer(object):
         if not dev_info:
             return False
 
-        path = dev_info['path']
-        path = path if os.path.isabs(path) else os.path.join(
-            env.path, path)
+        path = os.path.normpath(os.path.join(env.path, dev_info['path']))
 
         if 'dev_path' in spec.variants:
             assert spec.variants['dev_path'].value == path
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index fe1eb67459..9e3ad68124 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -132,7 +132,7 @@ def activate(
     if use_env_repo:
         spack.repo.path.put_first(_active_environment.repo)
 
-    tty.debug("Using environmennt '%s'" % _active_environment.name)
+    tty.debug("Using environment '%s'" % _active_environment.name)
 
     # Construct the commands to run
     cmds = ''
@@ -393,12 +393,12 @@ def read(name):
     return Environment(root(name))
 
 
-def create(name, init_file=None, with_view=None):
+def create(name, init_file=None, with_view=None, keep_relative=False):
     """Create a named environment in Spack."""
     validate_env_name(name)
     if exists(name):
         raise SpackEnvironmentError("'%s': environment already exists" % name)
-    return Environment(root(name), init_file, with_view)
+    return Environment(root(name), init_file, with_view, keep_relative)
 
 
 def config_dict(yaml_data):
@@ -587,7 +587,7 @@ class ViewDescriptor(object):
 
 
 class Environment(object):
-    def __init__(self, path, init_file=None, with_view=None):
+    def __init__(self, path, init_file=None, with_view=None, keep_relative=False):
         """Create a new environment.
 
         The environment can be optionally initialized with either a
@@ -600,6 +600,10 @@ class Environment(object):
             with_view (str or bool): whether a view should be maintained for
                 the environment. If the value is a string, it specifies the
                 path to the view.
+            keep_relative (bool): if True, develop paths are copied verbatim
+                into the new environment file, otherwise they are made absolute
+                when the environment path is different from init_file's
+                directory.
         """
         self.path = os.path.abspath(path)
 
@@ -621,6 +625,13 @@ class Environment(object):
                     self._set_user_specs_from_lockfile()
                 else:
                     self._read_manifest(f, raw_yaml=default_manifest_yaml)
+
+                # Rewrite relative develop paths when initializing a new
+                # environment in a different location from the spack.yaml file.
+                if not keep_relative and hasattr(f, 'name') and \
+                   f.name.endswith('.yaml'):
+                    init_file_dir = os.path.abspath(os.path.dirname(f.name))
+                    self._rewrite_relative_paths_on_relocation(init_file_dir)
         else:
             with lk.ReadTransaction(self.txlock):
                 self._read()
@@ -637,6 +648,27 @@ class Environment(object):
         # If with_view is None, then defer to the view settings determined by
         # the manifest file
 
+    def _rewrite_relative_paths_on_relocation(self, init_file_dir):
+        """When initializing the environment from a manifest file and we plan
+           to store the environment in a different directory, we have to rewrite
+           relative paths to absolute ones."""
+        if init_file_dir == self.path:
+            return
+
+        for name, entry in self.dev_specs.items():
+            dev_path = entry['path']
+            expanded_path = os.path.normpath(os.path.join(
+                init_file_dir, entry['path']))
+
+            # Skip if the expanded path is the same (e.g. when absolute)
+            if dev_path == expanded_path:
+                continue
+
+            tty.debug("Expanding develop path for {0} to {1}".format(
+                name, expanded_path))
+
+            self.dev_specs[name]['path'] = expanded_path
+
     def _re_read(self):
         """Reinitialize the environment object if it has been written (this
            may not be true if the environment was just created in this running
@@ -1044,8 +1076,7 @@ class Environment(object):
 
         if clone:
             # "steal" the source code via staging API
-            abspath = path if os.path.isabs(path) else os.path.join(
-                self.path, path)
+            abspath = os.path.normpath(os.path.join(self.path, path))
 
             stage = spec.package.stage
             stage.steal_source(abspath)
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 01536f267f..e61ebc74fd 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1615,8 +1615,7 @@ def _develop_specs_from_env(spec, env):
     if not dev_info:
         return
 
-    path = dev_info['path']
-    path = path if os.path.isabs(path) else os.path.join(env.path, path)
+    path = os.path.normpath(os.path.join(env.path, dev_info['path']))
 
     if 'dev_path' in spec.variants:
         assert spec.variants['dev_path'].value == path
diff --git a/lib/spack/spack/test/cmd/dev_build.py b/lib/spack/spack/test/cmd/dev_build.py
index 03f4ea5d12..11f4cbff82 100644
--- a/lib/spack/spack/test/cmd/dev_build.py
+++ b/lib/spack/spack/test/cmd/dev_build.py
@@ -202,7 +202,7 @@ env:
     dev-build-test-install:
       spec: dev-build-test-install@0.0.0
       path: %s
-""" % build_dir)
+""" % os.path.relpath(str(build_dir), start=str(envdir)))
 
         env('create', 'test', './spack.yaml')
         with ev.read('test'):
@@ -328,7 +328,7 @@ env:
     dev-build-test-install:
       spec: dev-build-test-install@0.0.0
       path: %s
-""" % build_dir)
+""" % os.path.relpath(str(build_dir), start=str(envdir)))
 
         env('create', 'test', './spack.yaml')
         with ev.read('test'):
@@ -343,7 +343,7 @@ env:
     assert dep_spec.package.filename in os.listdir(dep_spec.prefix)
     assert os.path.exists(spec.prefix)
 
-    # Ensure variants set properly
+    # Ensure variants set properly; ensure build_dir is absolute and normalized
     for dep in (dep_spec, spec['dev-build-test-install']):
         assert dep.satisfies('dev_path=%s' % build_dir)
     assert spec.satisfies('^dev_path=*')
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index e2df94cbfd..592d8f451c 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -2113,7 +2113,11 @@ def test_env_activate_default_view_root_unconditional(env_deactivate,
         viewdir = e.default_view.root
 
     out = env('activate', '--sh', 'test')
-    assert 'PATH=%s' % os.path.join(viewdir, 'bin') in out
+    viewdir_bin = os.path.join(viewdir, 'bin')
+
+    assert "export PATH={0}".format(viewdir_bin) in out or \
+           "export PATH='{0}".format(viewdir_bin) in out or \
+           'export PATH="{0}'.format(viewdir_bin) in out
 
 
 def test_concretize_user_specs_together():
@@ -2369,3 +2373,82 @@ spack:
             e.clear()
             e.write()
     assert os.path.exists(str(spack_lock))
+
+
+def _setup_develop_packages(tmpdir):
+    """Sets up a structure ./init_env/spack.yaml, ./build_folder, ./dest_env
+       where spack.yaml has a relative develop path to build_folder"""
+    init_env = tmpdir.join('init_env')
+    build_folder = tmpdir.join('build_folder')
+    dest_env = tmpdir.join('dest_env')
+
+    fs.mkdirp(str(init_env))
+    fs.mkdirp(str(build_folder))
+    fs.mkdirp(str(dest_env))
+
+    raw_yaml = """
+spack:
+  specs: ['mypkg1', 'mypkg2']
+  develop:
+    mypkg1:
+      path: ../build_folder
+      spec: mypkg@main
+    mypkg2:
+      path: /some/other/path
+      spec: mypkg@main
+"""
+    spack_yaml = init_env.join('spack.yaml')
+    spack_yaml.write(raw_yaml)
+
+    return init_env, build_folder, dest_env, spack_yaml
+
+
+def test_rewrite_rel_dev_path_new_dir(tmpdir):
+    """Relative develop paths should be rewritten for new environments in
+       a different directory from the original manifest file"""
+    _, build_folder, dest_env, spack_yaml = _setup_develop_packages(tmpdir)
+
+    env('create', '-d', str(dest_env), str(spack_yaml))
+    with ev.Environment(str(dest_env)) as e:
+        assert e.dev_specs['mypkg1']['path'] == str(build_folder)
+        assert e.dev_specs['mypkg2']['path'] == '/some/other/path'
+
+
+def test_rewrite_rel_dev_path_named_env(tmpdir):
+    """Relative develop paths should by default be rewritten for new named
+       environment"""
+    _, build_folder, _, spack_yaml = _setup_develop_packages(tmpdir)
+    env('create', 'named_env', str(spack_yaml))
+    with ev.read('named_env') as e:
+        assert e.dev_specs['mypkg1']['path'] == str(build_folder)
+        assert e.dev_specs['mypkg2']['path'] == '/some/other/path'
+
+
+def test_rewrite_rel_dev_path_original_dir(tmpdir):
+    """Relative devevelop paths should not be rewritten when initializing an
+       environment with root path set to the same directory"""
+    init_env, _, _, spack_yaml = _setup_develop_packages(tmpdir)
+    with ev.Environment(str(init_env), str(spack_yaml)) as e:
+        assert e.dev_specs['mypkg1']['path'] == '../build_folder'
+        assert e.dev_specs['mypkg2']['path'] == '/some/other/path'
+
+
+def test_rewrite_rel_dev_path_create_original_dir(tmpdir):
+    """Relative develop paths should not be rewritten when creating an
+       environment in the original directory"""
+    init_env, _, _, spack_yaml = _setup_develop_packages(tmpdir)
+    env('create', '-d', str(init_env), str(spack_yaml))
+    with ev.Environment(str(init_env)) as e:
+        assert e.dev_specs['mypkg1']['path'] == '../build_folder'
+        assert e.dev_specs['mypkg2']['path'] == '/some/other/path'
+
+
+def test_does_not_rewrite_rel_dev_path_when_keep_relative_is_set(tmpdir):
+    """Relative develop paths should not be rewritten when --keep-relative is
+       passed to create"""
+    _, _, _, spack_yaml = _setup_develop_packages(tmpdir)
+    env('create', '--keep-relative', 'named_env', str(spack_yaml))
+    with ev.read('named_env') as e:
+        print(e.dev_specs)
+        assert e.dev_specs['mypkg1']['path'] == '../build_folder'
+        assert e.dev_specs['mypkg2']['path'] == '/some/other/path'