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 @@ __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 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 @@ def regenerate(self, concretized_root_specs): 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")