environment: improve spack remove matching (#39390)

search for equivalent specs, not for equal strings when selecting a spec to remove.

3 files changed, 95 insertions, 5 deletions

diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index ea4ac49a3a..8eb7edab40 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -2664,6 +2664,26 @@ class EnvironmentManifestFile(collections.abc.Mapping):
         self.yaml_content = with_defaults_added
         self.changed = False
 
+    def _all_matches(self, user_spec: str) -> List[str]:
+        """Maps the input string to the first equivalent user spec in the manifest,
+        and returns it.
+
+        Args:
+            user_spec: user spec to be found
+
+        Raises:
+            ValueError: if no equivalent match is found
+        """
+        result = []
+        for yaml_spec_str in self.pristine_configuration["specs"]:
+            if Spec(yaml_spec_str) == Spec(user_spec):
+                result.append(yaml_spec_str)
+
+        if not result:
+            raise ValueError(f"cannot find a spec equivalent to {user_spec}")
+
+        return result
+
     def add_user_spec(self, user_spec: str) -> None:
         """Appends the user spec passed as input to the list of root specs.
 
@@ -2684,8 +2704,9 @@ class EnvironmentManifestFile(collections.abc.Mapping):
             SpackEnvironmentError: when the user spec is not in the list
         """
         try:
-            self.pristine_configuration["specs"].remove(user_spec)
-            self.configuration["specs"].remove(user_spec)
+            for key in self._all_matches(user_spec):
+                self.pristine_configuration["specs"].remove(key)
+                self.configuration["specs"].remove(key)
         except ValueError as e:
             msg = f"cannot remove {user_spec} from {self}, no such spec exists"
             raise SpackEnvironmentError(msg) from e
diff --git a/lib/spack/spack/spec_list.py b/lib/spack/spack/spec_list.py
index 6cad15e14e..3f60d57249 100644
--- a/lib/spack/spack/spec_list.py
+++ b/lib/spack/spack/spec_list.py
@@ -97,8 +97,10 @@ class SpecList:
             msg += "Either %s is not in %s or %s is " % (spec, self.name, spec)
             msg += "expanded from a matrix and cannot be removed directly."
             raise SpecListError(msg)
-        assert len(remove) == 1
-        self.yaml_list.remove(remove[0])
+
+        # Remove may contain more than one string representation of the same spec
+        for item in remove:
+            self.yaml_list.remove(item)
 
         # invalidate cache variables when we change the list
         self._expanded_list = None
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index 4396557f8a..e88af08761 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -13,7 +13,11 @@ import llnl.util.filesystem as fs
 
 import spack.environment as ev
 import spack.spec
-from spack.environment.environment import SpackEnvironmentViewError, _error_on_nonempty_view_dir
+from spack.environment.environment import (
+    EnvironmentManifestFile,
+    SpackEnvironmentViewError,
+    _error_on_nonempty_view_dir,
+)
 
 pytestmark = pytest.mark.not_on_windows("Envs are not supported on windows")
 
@@ -623,3 +627,66 @@ def test_requires_on_virtual_and_potential_providers(
         assert mpileaks.satisfies("^mpich2")
         assert mpileaks["mpi"].satisfies("mpich2")
         assert not mpileaks.satisfies(f"^{possible_mpi_spec}")
+
+
+@pytest.mark.regression("39387")
+@pytest.mark.parametrize(
+    "spec_str", ["mpileaks +opt", "mpileaks  +opt   ~shared", "mpileaks  ~shared   +opt"]
+)
+def test_manifest_file_removal_works_if_spec_is_not_normalized(tmp_path, spec_str):
+    """Tests that we can remove a spec from a manifest file even if its string
+    representation is not normalized.
+    """
+    manifest = tmp_path / "spack.yaml"
+    manifest.write_text(
+        f"""\
+spack:
+  specs:
+  - {spec_str}
+"""
+    )
+    s = spack.spec.Spec(spec_str)
+    spack_yaml = EnvironmentManifestFile(tmp_path)
+    # Doing a round trip str -> Spec -> str normalizes the representation
+    spack_yaml.remove_user_spec(str(s))
+    spack_yaml.flush()
+
+    assert spec_str not in manifest.read_text()
+
+
+@pytest.mark.regression("39387")
+@pytest.mark.parametrize(
+    "duplicate_specs,expected_number",
+    [
+        # Swap variants, versions, etc. add spaces
+        (["foo +bar ~baz", "foo ~baz    +bar"], 3),
+        (["foo @1.0 ~baz %gcc", "foo ~baz @1.0%gcc"], 3),
+        # Item 1 and 3 are exactly the same
+        (["zlib +shared", "zlib      +shared", "zlib +shared"], 4),
+    ],
+)
+def test_removing_spec_from_manifest_with_exact_duplicates(
+    duplicate_specs, expected_number, tmp_path
+):
+    """Tests that we can remove exact duplicates from a manifest file.
+
+    Note that we can't get in a state with duplicates using only CLI, but this might happen
+    on user edited spack.yaml files.
+    """
+    manifest = tmp_path / "spack.yaml"
+    manifest.write_text(
+        f"""\
+    spack:
+      specs: [{", ".join(duplicate_specs)} , "zlib"]
+    """
+    )
+
+    with ev.Environment(tmp_path) as env:
+        assert len(env.user_specs) == expected_number
+        env.remove(duplicate_specs[0])
+        env.write()
+
+    assert "+shared" not in manifest.read_text()
+    assert "zlib" in manifest.read_text()
+    with ev.Environment(tmp_path) as env:
+        assert len(env.user_specs) == 1