fix and test issue with copying by reference vs value

lib/spack/spack/environment/environment.py

@@ -1345,28 +1345,38 @@ def concretize(self, force=False, tests=False):
             List of specs that have been concretized. Each entry is a tuple of
             the user spec and the corresponding concretized spec.
         """
-        if force:
-            # Clear previously concretized specs
-            self.concretized_user_specs = []
-            self.concretized_order = []
-            self.specs_by_hash = {}
+        old_concretized_user_specs = self.concretized_user_specs[:]
+        old_concretized_order = self.concretized_order[:]
+        old_specs_by_hash = self.specs_by_hash.copy()
 
-        # Remove concrete specs that no longer correlate to a user spec
-        for spec in set(self.concretized_user_specs) - set(self.user_specs):
-            self.deconcretize(spec)
+        try:
+            if force:
+                # Clear previously concretized specs
+                self.concretized_user_specs = []
+                self.concretized_order = []
+                self.specs_by_hash = {}
 
-        # Pick the right concretization strategy
-        if self.unify == "when_possible":
-            return self._concretize_together_where_possible(tests=tests)
+            # Remove concrete specs that no longer correlate to a user spec
+            for spec in set(self.concretized_user_specs) - set(self.user_specs):
+                self.deconcretize(spec)
 
-        if self.unify is True:
-            return self._concretize_together(tests=tests)
+            # Pick the right concretization strategy
+            if self.unify == "when_possible":
+                return self._concretize_together_where_possible(tests=tests)
 
-        if self.unify is False:
-            return self._concretize_separately(tests=tests)
+            if self.unify is True:
+                return self._concretize_together(tests=tests)
 
-        msg = "concretization strategy not implemented [{0}]"
-        raise SpackEnvironmentError(msg.format(self.unify))
+            if self.unify is False:
+                return self._concretize_separately(tests=tests)
+
+            msg = "concretization strategy not implemented [{0}]"
+            raise SpackEnvironmentError(msg.format(self.unify))
+        except Exception:
+            self.concretized_user_specs = old_concretized_user_specs
+            self.concretized_order = old_concretized_order
+            self.specs_by_hash = old_specs_by_hash
+            raise
 
     def deconcretize(self, spec):
         # spec has to be a root of the environment

lib/spack/spack/test/cmd/env.py

@@ -17,6 +17,7 @@
 import llnl.util.link_tree
 
 import spack.cmd.env
+import spack.concretize
 import spack.config
 import spack.environment as ev
 import spack.environment.environment
@@ -25,6 +26,7 @@
 import spack.modules
 import spack.paths
 import spack.repo
+import spack.solver.asp
 import spack.util.spack_json as sjson
 from spack.cmd.env import _env_create
 from spack.main import SpackCommand, SpackCommandError
@@ -2759,6 +2761,51 @@ def test_virtual_spec_concretize_together(tmpdir):
     assert any(s.package.provides("mpi") for _, s in e.concretized_specs())
 
 
+@pytest.mark.parametrize(
+    "unify,method_to_fail",
+    [
+        (True, (spack.concretize, "concretize_specs_together")),
+        ("when_possible", (spack.solver.asp.Solver, "solve_in_rounds")),
+        # An earlier failure so that we test the case where the internal state
+        # has been changed, but the pointer to the internal variables has not change.
+        # This effectively tests that we are properly copying by value not by
+        # reference for the transactional concretization
+        (True, (spack.environment.Environment, "_get_specs_to_concretize")),
+    ],
+)
+def test_concretize_transactional(unify, method_to_fail, monkeypatch):
+    e = ev.create("test")
+    e.unify = unify
+
+    e.add("mpi")
+    e.add("zlib")
+    e.concretize()
+
+    # remove one spec and add another to ensure we test with changes before
+    # and after the environment is cleared during concretization
+    e.remove("zlib")
+    e.add("libelf")
+
+    def fail(*args, **kwargs):
+        raise Exception("Test failures")
+
+    location, method = method_to_fail
+    monkeypatch.setattr(location, method, fail)
+
+    first_user_specs = e.concretized_user_specs[:]
+    first_order = e.concretized_order[:]
+    first_hash_dict = e.specs_by_hash.copy()
+
+    try:
+        e.concretize()
+    except Exception:
+        pass
+
+    assert e.concretized_user_specs == first_user_specs
+    assert e.concretized_order == first_order
+    assert e.specs_by_hash == first_hash_dict
+
+
 def test_query_develop_specs():
     """Test whether a spec is develop'ed or not"""
     env("create", "test")