From 195341113e7da85611553d1ea9bd907ccabeff64 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Mon, 15 Mar 2021 21:38:35 +0100 Subject: 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. --- lib/spack/spack/cmd/env.py | 16 +++++-- lib/spack/spack/concretize.py | 4 +- lib/spack/spack/environment.py | 43 +++++++++++++++--- lib/spack/spack/solver/asp.py | 3 +- lib/spack/spack/test/cmd/dev_build.py | 6 +-- lib/spack/spack/test/cmd/env.py | 85 ++++++++++++++++++++++++++++++++++- share/spack/spack-completion.bash | 2 +- 7 files changed, 139 insertions(+), 20 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' diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash index 20bcf1e660..a0fea36e21 100755 --- a/share/spack/spack-completion.bash +++ b/share/spack/spack-completion.bash @@ -793,7 +793,7 @@ _spack_env_deactivate() { _spack_env_create() { if $list_options then - SPACK_COMPREPLY="-h --help -d --dir --without-view --with-view" + SPACK_COMPREPLY="-h --help -d --dir --keep-relative --without-view --with-view" else _environments fi -- cgit v1.2.3-70-g09d2