environment.py: ensure view dir does not exist (#29641)

4 files changed, 101 insertions, 0 deletions

diff --git a/lib/spack/spack/environment/__init__.py b/lib/spack/spack/environment/__init__.py
index 4b475cab08..b710979013 100644
--- a/lib/spack/spack/environment/__init__.py
+++ b/lib/spack/spack/environment/__init__.py
@@ -5,6 +5,7 @@
 from .environment import (
     Environment,
     SpackEnvironmentError,
+    SpackEnvironmentViewError,
     activate,
     active,
     active_environment,
@@ -33,6 +34,7 @@ from .environment import (
 __all__ = [
     'Environment',
     'SpackEnvironmentError',
+    'SpackEnvironmentViewError',
     'activate',
     'active',
     'active_environment',
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 20f6060265..72df9cbfe3 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -8,6 +8,7 @@ import copy
 import os
 import re
 import shutil
+import stat
 import sys
 import time
 
@@ -336,6 +337,29 @@ def _spec_needs_overwrite(spec, changed_dev_specs):
         return True
 
 
+def _error_on_nonempty_view_dir(new_root):
+    """Defensively error when the target view path already exists and is not an
+    empty directory. This usually happens when the view symlink was removed, but
+    not the directory it points to. In those cases, it's better to just error when
+    the new view dir is non-empty, since it indicates the user removed part but not
+    all of the view, and it likely in an inconsistent state."""
+    # Check if the target path lexists
+    try:
+        st = os.lstat(new_root)
+    except (IOError, OSError):
+        return
+
+    # Empty directories are fine
+    if stat.S_ISDIR(st.st_mode) and len(os.listdir(new_root)) == 0:
+        return
+
+    # Anything else is an error
+    raise SpackEnvironmentViewError(
+        "Failed to generate environment view, because the target {} already "
+        "exists or is not empty. To update the view, remove this path, and run "
+        "`spack env view regenerate`".format(new_root))
+
+
 class ViewDescriptor(object):
     def __init__(self, base_path, root, projections={}, select=[], exclude=[],
                  link=default_view_link, link_type='symlink'):
@@ -519,6 +543,8 @@ class ViewDescriptor(object):
             tty.debug("View at %s does not need regeneration." % self.root)
             return
 
+        _error_on_nonempty_view_dir(new_root)
+
         # construct view at new_root
         if specs:
             tty.msg("Updating view at {0}".format(self.root))
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 7853cb16f2..7f05e22545 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import glob
 import os
+import shutil
 import sys
 from argparse import Namespace
 
@@ -2805,3 +2806,40 @@ def test_failed_view_cleanup(tmpdir, mock_stage, mock_fetch, install_mockery):
     views_after = os.listdir(all_views)
     assert views_before == views_after
     assert os.path.samefile(resolved_view, view)
+
+
+def test_environment_view_target_already_exists(
+    tmpdir, mock_stage, mock_fetch, install_mockery
+):
+    """When creating a new view, Spack should check whether
+    the new view dir already exists. If so, it should not be
+    removed or modified."""
+
+    # Create a new environment
+    view = str(tmpdir.join('view'))
+    env('create', '--with-view={0}'.format(view), 'test')
+    with ev.read('test'):
+        add('libelf')
+        install('--fake')
+
+    # Empty the underlying view
+    real_view = os.path.realpath(view)
+    assert os.listdir(real_view)  # make sure it had *some* contents
+    shutil.rmtree(real_view)
+
+    # Replace it with something new.
+    os.mkdir(real_view)
+    fs.touch(os.path.join(real_view, 'file'))
+
+    # Remove the symlink so Spack can't know about the "previous root"
+    os.unlink(view)
+
+    # Regenerate the view, which should realize it can't write into the same dir.
+    msg = 'Failed to generate environment view'
+    with ev.read('test'):
+        with pytest.raises(ev.SpackEnvironmentViewError, match=msg):
+            env('view', 'regenerate')
+
+    # Make sure the dir was left untouched.
+    assert not os.path.lexists(view)
+    assert os.listdir(real_view) == ['file']
diff --git a/lib/spack/spack/test/environment.py b/lib/spack/spack/test/environment.py
index 2040fb90ea..36bd87bc7e 100644
--- a/lib/spack/spack/test/environment.py
+++ b/lib/spack/spack/test/environment.py
@@ -3,9 +3,16 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import os
 import pickle
 
+import pytest
+
 from spack.environment import Environment
+from spack.environment.environment import (
+    SpackEnvironmentViewError,
+    _error_on_nonempty_view_dir,
+)
 
 
 def test_environment_pickle(tmpdir):
@@ -13,3 +20,31 @@ def test_environment_pickle(tmpdir):
     obj = pickle.dumps(env1)
     env2 = pickle.loads(obj)
     assert isinstance(env2, Environment)
+
+
+def test_error_on_nonempty_view_dir(tmpdir):
+    """Error when the target is not an empty dir"""
+    with tmpdir.as_cwd():
+        os.mkdir("empty_dir")
+        os.mkdir("nonempty_dir")
+        with open(os.path.join("nonempty_dir", "file"), "wb"):
+            pass
+        os.symlink("empty_dir", "symlinked_empty_dir")
+        os.symlink("does_not_exist", "broken_link")
+        os.symlink("broken_link", "file")
+
+        # This is OK.
+        _error_on_nonempty_view_dir("empty_dir")
+
+        # This is not OK.
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("nonempty_dir")
+
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("symlinked_empty_dir")
+
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("broken_link")
+
+        with pytest.raises(SpackEnvironmentViewError):
+            _error_on_nonempty_view_dir("file")