New command, `spack change`, to change existing env specs (#31995)

If you have an environment like

```
$ cat spack.yaml
spack:
  specs: [openmpi@4.1.0+cuda]
```

this PR provides a new command `spack change` that you can use to adjust environment specs from the command line:

```
$ spack change openmpi~cuda
$ cat spack.yaml
spack:
  specs: [openmpi@4.1.0~cuda]
```

in other words, this allows you to tweak the details of environment specs from the command line.

Notes:

* This is only allowed for environments that do not define matrices
  * This is possible but not anticipated to be needed immediately
  * If this were done, it should probably only be done for "named"/not-anonymous specs (i.e. we can change `openmpi+cuda` but not spec like `+cuda` or `@4.0.1~cuda`)

7 files changed, 263 insertions, 3 deletions

diff --git a/lib/spack/spack/cmd/change.py b/lib/spack/spack/cmd/change.py
new file mode 100644
index 0000000000..17d71f33c5
--- /dev/null
+++ b/lib/spack/spack/cmd/change.py
@@ -0,0 +1,51 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import spack.cmd
+import spack.cmd.common.arguments as arguments
+
+description = "change an existing spec in an environment"
+section = "environments"
+level = "long"
+
+
+def setup_parser(subparser):
+    subparser.add_argument(
+        "-l",
+        "--list-name",
+        dest="list_name",
+        default="specs",
+        help="name of the list to remove specs from",
+    )
+    subparser.add_argument(
+        "--match-spec",
+        dest="match_spec",
+        help="if name is ambiguous, supply a spec to match",
+    )
+    subparser.add_argument(
+        "-a",
+        "--all",
+        action="store_true",
+        help="change all matching specs (allow changing more than one spec)",
+    )
+    arguments.add_common_arguments(subparser, ["specs"])
+
+
+def change(parser, args):
+    env = spack.cmd.require_active_env(cmd_name="change")
+
+    with env.write_transaction():
+        if args.match_spec:
+            match_spec = spack.spec.Spec(args.match_spec)
+        else:
+            match_spec = None
+        for spec in spack.cmd.parse_specs(args.specs):
+            env.change_existing_spec(
+                spec,
+                list_name=args.list_name,
+                match_spec=match_spec,
+                allow_changing_multiple_specs=args.all,
+            )
+        env.write()
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 3df8e4f1df..a27977c956 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -1073,6 +1073,58 @@ class Environment(object):
 
         return bool(not existing)
 
+    def change_existing_spec(
+        self,
+        change_spec,
+        list_name=user_speclist_name,
+        match_spec=None,
+        allow_changing_multiple_specs=False,
+    ):
+        """
+        Find the spec identified by `match_spec` and change it to `change_spec`.
+
+        Arguments:
+            change_spec (spack.spec.Spec): defines the spec properties that
+                need to be changed. This will not change attributes of the
+                matched spec unless they conflict with `change_spec`.
+            list_name (str): identifies the spec list in the environment that
+                should be modified
+            match_spec (spack.spec.Spec): if set, this identifies the spec
+                that should be changed. If not set, it is assumed we are
+                looking for a spec with the same name as `change_spec`.
+        """
+        if not (change_spec.name or (match_spec and match_spec.name)):
+            raise ValueError(
+                "Must specify a spec name to identify a single spec"
+                " in the environment that will be changed"
+            )
+        match_spec = match_spec or Spec(change_spec.name)
+
+        list_to_change = self.spec_lists[list_name]
+        if list_to_change.is_matrix:
+            raise SpackEnvironmentError(
+                "Cannot directly change specs in matrices:"
+                " specify a named list that is not a matrix"
+            )
+
+        matches = list(x for x in list_to_change if x.satisfies(match_spec))
+        if len(matches) == 0:
+            raise ValueError(
+                "There are no specs named {0} in {1}".format(match_spec.name, list_name)
+            )
+        elif len(matches) > 1 and not allow_changing_multiple_specs:
+            raise ValueError("{0} matches multiple specs".format(str(match_spec)))
+
+        new_speclist = SpecList(list_name)
+        for i, spec in enumerate(list_to_change):
+            if spec.satisfies(match_spec):
+                new_speclist.add(Spec.override(spec, change_spec))
+            else:
+                new_speclist.add(spec)
+
+        self.spec_lists[list_name] = new_speclist
+        self.update_stale_references()
+
     def remove(self, query_spec, list_name=user_speclist_name, force=False):
         """Remove specs from an environment that match a query_spec"""
         query_spec = Spec(query_spec)
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 8a463c2f78..b7ff676fe2 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -284,6 +284,22 @@ class ArchSpec(object):
 
         self.platform, self.os, self.target = platform_tuple
 
+    @staticmethod
+    def override(init_spec, change_spec):
+        if init_spec:
+            new_spec = init_spec.copy()
+        else:
+            new_spec = ArchSpec()
+        if change_spec.platform:
+            new_spec.platform = change_spec.platform
+            # TODO: if the platform is changed to something that is incompatible
+            # with the current os, we should implicitly remove it
+        if change_spec.os:
+            new_spec.os = change_spec.os
+        if change_spec.target:
+            new_spec.target = change_spec.target
+        return new_spec
+
     def _autospec(self, spec_like):
         if isinstance(spec_like, ArchSpec):
             return spec_like
@@ -2243,6 +2259,33 @@ class Spec(object):
             yield dep_name, dep_hash, list(deptypes), hash_type
 
     @staticmethod
+    def override(init_spec, change_spec):
+        # TODO: this doesn't account for the case where the changed spec
+        # (and the user spec) have dependencies
+        new_spec = init_spec.copy()
+        package_cls = spack.repo.path.get_pkg_class(new_spec.name)
+        if change_spec.versions and not change_spec.versions == spack.version.ver(":"):
+            new_spec.versions = change_spec.versions
+        for variant, value in change_spec.variants.items():
+            if variant in package_cls.variants:
+                if variant in new_spec.variants:
+                    new_spec.variants.substitute(value)
+                else:
+                    new_spec.variants[variant] = value
+            else:
+                raise ValueError("{0} is not a variant of {1}".format(variant, new_spec.name))
+        if change_spec.compiler:
+            new_spec.compiler = change_spec.compiler
+        if change_spec.compiler_flags:
+            for flagname, flagvals in change_spec.compiler_flags.items():
+                new_spec.compiler_flags[flagname] = flagvals
+        if change_spec.architecture:
+            new_spec.architecture = ArchSpec.override(
+                new_spec.architecture, change_spec.architecture
+            )
+        return new_spec
+
+    @staticmethod
     def from_literal(spec_dict, normal=True):
         """Builds a Spec from a dictionary containing the spec literal.
 
diff --git a/lib/spack/spack/spec_list.py b/lib/spack/spack/spec_list.py
index aac099eb4d..ffbf2bfaf9 100644
--- a/lib/spack/spack/spec_list.py
+++ b/lib/spack/spack/spec_list.py
@@ -35,6 +35,13 @@ class SpecList(object):
         self._specs = None
 
     @property
+    def is_matrix(self):
+        for item in self.specs_as_yaml_list:
+            if isinstance(item, dict):
+                return True
+        return False
+
+    @property
     def specs_as_yaml_list(self):
         if self._expanded_list is None:
             self._expanded_list = self._expand_references(self.yaml_list)
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index d849931dea..e465060392 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -43,6 +43,7 @@ pytestmark = [
 env = SpackCommand("env")
 install = SpackCommand("install")
 add = SpackCommand("add")
+change = SpackCommand("change")
 remove = SpackCommand("remove")
 concretize = SpackCommand("concretize")
 stage = SpackCommand("stage")
@@ -71,6 +72,35 @@ def test_add():
     assert Spec("mpileaks") in e.user_specs
 
 
+def test_change_match_spec():
+    env("create", "test")
+
+    e = ev.read("test")
+    with e:
+        add("mpileaks@2.1")
+        add("mpileaks@2.2")
+
+        change("--match-spec", "mpileaks@2.2", "mpileaks@2.3")
+
+    assert not any(x.satisfies("mpileaks@2.2") for x in e.user_specs)
+    assert any(x.satisfies("mpileaks@2.3") for x in e.user_specs)
+
+
+def test_change_multiple_matches():
+    env("create", "test")
+
+    e = ev.read("test")
+    with e:
+        add("mpileaks@2.1")
+        add("mpileaks@2.2")
+        add("libelf@0.8.12%clang")
+
+        change("--match-spec", "mpileaks", "-a", "mpileaks%gcc")
+
+    assert all(x.satisfies("%gcc") for x in e.user_specs if x.name == "mpileaks")
+    assert any(x.satisfies("%clang") for x in e.user_specs if x.name == "libelf")
+
+
 def test_env_add_virtual():
     env("create", "test")
 
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index c1b36eb5dd..60056e397a 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -4,16 +4,19 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 """Test environment internals without CLI"""
+import sys
 
 import pytest
+from six import StringIO
 
 import spack.environment as ev
 import spack.spec
 
-
-@pytest.mark.skipif(
-    str(spack.platforms.host()) == "windows", reason="Not supported on Windows (yet)"
+pytestmark = pytest.mark.skipif(
+    sys.platform == "win32", reason="Envs are not supported on windows"
 )
+
+
 def test_hash_change_no_rehash_concrete(tmpdir, mock_packages, config):
     # create an environment
     env_path = tmpdir.mkdir("env_dir").strpath
@@ -43,6 +46,66 @@ def test_hash_change_no_rehash_concrete(tmpdir, mock_packages, config):
     assert read_in.specs_by_hash[read_in.concretized_order[0]]._hash == new_hash
 
 
+def test_env_change_spec(tmpdir, mock_packages, config):
+    env_path = tmpdir.mkdir("env_dir").strpath
+    env = ev.Environment(env_path)
+    env.write()
+
+    spec = spack.spec.Spec("mpileaks@2.1~shared+debug")
+    env.add(spec)
+    env.write()
+
+    change_spec = spack.spec.Spec("mpileaks@2.2")
+    env.change_existing_spec(change_spec)
+    (spec,) = env.added_specs()
+    assert spec == spack.spec.Spec("mpileaks@2.2~shared+debug")
+
+    change_spec = spack.spec.Spec("mpileaks~debug")
+    env.change_existing_spec(change_spec)
+    (spec,) = env.added_specs()
+    assert spec == spack.spec.Spec("mpileaks@2.2~shared~debug")
+
+
+_test_matrix_yaml = """\
+env:
+  definitions:
+  - compilers: ["%gcc", "%clang"]
+  - desired_specs: ["mpileaks@2.1"]
+  specs:
+  - matrix:
+    - [$compilers]
+    - [$desired_specs]
+"""
+
+
+def test_env_change_spec_in_definition(tmpdir, mock_packages, config, mutable_mock_env_path):
+    initial_yaml = StringIO(_test_matrix_yaml)
+    e = ev.create("test", initial_yaml)
+    e.concretize()
+    e.write()
+
+    assert any(x.satisfies("mpileaks@2.1%gcc") for x in e.user_specs)
+
+    e.change_existing_spec(spack.spec.Spec("mpileaks@2.2"), list_name="desired_specs")
+    e.write()
+
+    assert any(x.satisfies("mpileaks@2.2%gcc") for x in e.user_specs)
+    assert not any(x.satisfies("mpileaks@2.1%gcc") for x in e.user_specs)
+
+
+def test_env_change_spec_in_matrix_raises_error(
+    tmpdir, mock_packages, config, mutable_mock_env_path
+):
+    initial_yaml = StringIO(_test_matrix_yaml)
+    e = ev.create("test", initial_yaml)
+    e.concretize()
+    e.write()
+
+    with pytest.raises(spack.environment.SpackEnvironmentError) as error:
+        e.change_existing_spec(spack.spec.Spec("mpileaks@2.2"))
+    assert "Cannot directly change specs in matrices" in str(error)
+
+
 def test_activate_should_require_an_env():
     with pytest.raises(TypeError):
         ev.activate(env="name")
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index d417adaa18..b543eae9a8 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -1111,6 +1111,20 @@ class TestSpecSematics(object):
         with pytest.raises(spack.spec.SpliceError, match="will not provide the same virtuals."):
             spec.splice(dep, transitive)
 
+    def test_spec_override(self):
+        init_spec = Spec("a foo=baz foobar=baz cflags=-O3 cxxflags=-O1")
+        change_spec = Spec("a foo=fee cflags=-O2")
+        new_spec = Spec.override(init_spec, change_spec)
+        new_spec.concretize()
+        assert "foo=fee" in new_spec
+        # This check fails without concretizing: apparently if both specs are
+        # abstract, then the spec will always be considered to satisfy
+        # 'variant=value' (regardless of whether it in fact does).
+        assert "foo=baz" not in new_spec
+        assert "foobar=baz" in new_spec
+        assert new_spec.compiler_flags["cflags"] == ["-O2"]
+        assert new_spec.compiler_flags["cxxflags"] == ["-O1"]
+
 
 @pytest.mark.regression("3887")
 @pytest.mark.parametrize("spec_str", ["git", "hdf5", "py-flake8"])