From 13739e07837a580fd8c632e0dde35938ecc4908c Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Mon, 23 Jan 2023 19:03:54 +0100 Subject: environments: don't replace relative view path with absolute path on concretize/install (#34958) * environments: don't rewrite relative view path, expand path on cli ahead of time Currently if you have a spack.yaml that specifies a view by relative path, Spack expands it to an absolute path on `spack -e . install` and persists that to disk. This is rather annoying when you have a `spack.yaml` file inside a git repo, cause you want to use relative paths to make it relocatable, but you constantly have to undo the changes made to spack.yaml by Spack. So, as an alternative: 1. Always stick to paths as they are provided in spack.yaml, never replace them with a canonicalized version 2. Turn relative paths on the command line into absolute paths before storing to spack.yaml. This way you can do `spack env create --dir ./env --with-view ./view` and both `./env` and `./view` are resolved to the current working dir, as expected (not `./env/view`). This corresponds to the old behavior of `spack env create`. * create --with-view always takes a value --- lib/spack/spack/cmd/env.py | 6 +++++- lib/spack/spack/environment/environment.py | 7 ++++--- lib/spack/spack/test/cmd/env.py | 7 +++++++ lib/spack/spack/test/env.py | 32 ++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/cmd/env.py b/lib/spack/spack/cmd/env.py index 16e2838b67..fd8bf2de56 100644 --- a/lib/spack/spack/cmd/env.py +++ b/lib/spack/spack/cmd/env.py @@ -291,7 +291,11 @@ def env_create_setup_parser(subparser): def env_create(args): if args.with_view: - with_view = args.with_view + # Expand relative paths provided on the command line to the current working directory + # This way we interpret `spack env create --with-view ./view --dir ./env` as + # a view in $PWD/view, not $PWD/env/view. This is different from specifying a relative + # path in spack.yaml, which is resolved relative to the environment file. + with_view = os.path.abspath(args.with_view) elif args.without_view: with_view = False else: diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 79188c01d9..f81232feec 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -384,7 +384,8 @@ class ViewDescriptor(object): link_type="symlink", ): self.base = base_path - self.root = spack.util.path.canonicalize_path(root) + self.raw_root = root + self.root = spack.util.path.canonicalize_path(root, default_wd=base_path) self.projections = projections self.select = select self.exclude = exclude @@ -410,7 +411,7 @@ class ViewDescriptor(object): ) def to_dict(self): - ret = syaml.syaml_dict([("root", self.root)]) + ret = syaml.syaml_dict([("root", self.raw_root)]) if self.projections: # projections guaranteed to be ordered dict if true-ish # for python2.6, may be syaml or ruamel.yaml implementation @@ -2127,7 +2128,7 @@ class Environment(object): # Construct YAML representation of view default_name = default_view_name if self.views and len(self.views) == 1 and default_name in self.views: - path = self.default_view.root + path = self.default_view.raw_root if self.default_view == ViewDescriptor(self.path, self.view_path_default): view = True elif self.default_view == ViewDescriptor(self.path, path): diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index df1de8beeb..19b53c0573 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -3229,3 +3229,10 @@ def test_env_include_packages_url( cfg = spack.config.get("packages") assert "openmpi" in cfg["all"]["providers"]["mpi"] + + +def test_relative_view_path_on_command_line_is_made_absolute(tmpdir, config): + with fs.working_dir(str(tmpdir)): + env("create", "--with-view", "view", "--dir", "env") + environment = ev.Environment(os.path.join(".", "env")) + assert os.path.samefile("view", environment.default_view.root) diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py index aebad49253..83538e7d71 100644 --- a/lib/spack/spack/test/env.py +++ b/lib/spack/spack/test/env.py @@ -4,10 +4,13 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) """Test environment internals without CLI""" import io +import os import sys import pytest +import llnl.util.filesystem as fs + import spack.environment as ev import spack.spec @@ -111,3 +114,32 @@ def test_activate_should_require_an_env(): with pytest.raises(TypeError): ev.activate(env=None) + + +def test_user_view_path_is_not_canonicalized_in_yaml(tmpdir, config): + # When spack.yaml files are checked into version control, we + # don't want view: ./relative to get canonicalized on disk. + + # We create a view in /env_dir + env_path = tmpdir.mkdir("env_dir").strpath + + # And use a relative path to specify the view dir + view = os.path.join(".", "view") + + # Which should always resolve to the following independent of cwd. + absolute_view = os.path.join(env_path, "view") + + # Serialize environment with relative view path + with fs.working_dir(str(tmpdir)): + fst = ev.Environment(env_path, with_view=view) + fst.write() + + # The view link should be created + assert os.path.isdir(absolute_view) + + # Deserialize and check if the view path is still relative in yaml + # and also check that the getter is pointing to the right dir. + with fs.working_dir(str(tmpdir)): + snd = ev.Environment(env_path) + assert snd.yaml["spack"]["view"] == view + assert os.path.samefile(snd.default_view.root, absolute_view) -- cgit v1.2.3-70-g09d2