environment.py: clean up broken views on failure (#29643)

When view creation fails for some reason, remove it, so that the next
time around it can start from scratch.

2 files changed, 46 insertions, 13 deletions

diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 4feecfab53..ea1dc85d4a 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -524,22 +524,30 @@ class ViewDescriptor(object):
             tty.msg("Updating view at {0}".format(self.root))
 
         view = self.view(new=new_root)
-        fs.mkdirp(new_root)
-        view.add_specs(*specs, with_dependencies=False)
 
-        # create symlink from tmpname to new_root
         root_dirname = os.path.dirname(self.root)
         tmp_symlink_name = os.path.join(root_dirname, '._view_link')
-        if os.path.exists(tmp_symlink_name):
-            os.unlink(tmp_symlink_name)
-        symlink(new_root, tmp_symlink_name)
-
-        # mv symlink atomically over root symlink to old_root
-        if os.path.exists(self.root) and not os.path.islink(self.root):
-            msg = "Cannot create view: "
-            msg += "file already exists and is not a link: %s" % self.root
-            raise SpackEnvironmentViewError(msg)
-        rename(tmp_symlink_name, self.root)
+
+        # Create a new view
+        try:
+            fs.mkdirp(new_root)
+            view.add_specs(*specs, with_dependencies=False)
+
+            # create symlink from tmp_symlink_name to new_root
+            if os.path.exists(tmp_symlink_name):
+                os.unlink(tmp_symlink_name)
+            symlink(new_root, tmp_symlink_name)
+
+            # mv symlink atomically over root symlink to old_root
+            rename(tmp_symlink_name, self.root)
+        except Exception as e:
+            # Clean up new view and temporary symlink on any failure.
+            try:
+                shutil.rmtree(new_root, ignore_errors=True)
+                os.unlink(tmp_symlink_name)
+            except (IOError, OSError):
+                pass
+            raise e
 
         # Remove the old root when it's in the same folder as the new root. This guards
         # against removal of an arbitrary path when the original symlink in self.root
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index cf2c947bdc..7315a9d7f1 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -2746,3 +2746,28 @@ def test_env_view_fail_if_symlink_points_elsewhere(tmpdir, install_mockery, mock
         add('libelf')
         install('--fake')
     assert os.path.isdir(non_view_dir)
+
+
+def test_failed_view_cleanup(tmpdir, mock_stage, mock_fetch, install_mockery):
+    """Tests whether Spack cleans up after itself when a view fails to create"""
+    view = str(tmpdir.join('view'))
+    with ev.create('env', with_view=view):
+        add('libelf')
+        install('--fake')
+
+    # Save the current view directory.
+    resolved_view = os.path.realpath(view)
+    all_views = os.path.dirname(resolved_view)
+    views_before = os.listdir(all_views)
+
+    # Add a spec that results in MergeConflictError's when creating a view
+    with ev.read('env'):
+        add('libelf cflags=-O3')
+        with pytest.raises(llnl.util.link_tree.MergeConflictError):
+            install('--fake')
+
+    # Make sure there is no broken view in the views directory, and the current
+    # view is the original view from before the failed regenerate attempt.
+    views_after = os.listdir(all_views)
+    assert views_before == views_after
+    assert os.path.samefile(resolved_view, view)