view remove: directly check whether specs own files before removing from view (#16955)

Bugfix for hardlinks and copies 

3 files changed, 72 insertions, 5 deletions

diff --git a/lib/spack/spack/filesystem_view.py b/lib/spack/spack/filesystem_view.py
index f4d77a694b..0f21e4d975 100644
--- a/lib/spack/spack/filesystem_view.py
+++ b/lib/spack/spack/filesystem_view.py
@@ -3,7 +3,6 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-import filecmp
 import functools as ft
 import os
 import re
@@ -18,6 +17,7 @@ from llnl.util.filesystem import (
     mkdirp, remove_dead_links, remove_empty_directories)
 
 import spack.util.spack_yaml as s_yaml
+import spack.util.spack_json as s_json
 
 import spack.spec
 import spack.store
@@ -411,10 +411,34 @@ class YamlFilesystemView(FilesystemView):
         if not os.path.lexists(dest):
             tty.warn("Tried to remove %s which does not exist" % dest)
             return
-        # remove if dest is a hardlink/symlink to src; this will only
-        # be false if two packages are merged into a prefix and have a
-        # conflicting file
-        if filecmp.cmp(src, dest, shallow=True):
+
+        def needs_file(spec, file):
+            # convert the file we want to remove to a source in this spec
+            projection = self.get_projection_for_spec(spec)
+            relative_path = os.path.relpath(file, projection)
+            test_path = os.path.join(spec.prefix, relative_path)
+
+            # check if this spec owns a file of that name (through the
+            # manifest in the metadata dir, which we have in the view).
+            manifest_file = os.path.join(self.get_path_meta_folder(spec),
+                                         spack.store.layout.manifest_file_name)
+            try:
+                with open(manifest_file, 'r') as f:
+                    manifest = s_json.load(f)
+            except (OSError, IOError):
+                # if we can't load it, assume it doesn't know about the file.
+                manifest = {}
+            return test_path in manifest
+
+        # remove if dest is not owned by any other package in the view
+        # This will only be false if two packages are merged into a prefix
+        # and have a conflicting file
+
+        # check all specs for whether they own the file. That include the spec
+        # we are currently removing, as we remove files before unlinking the
+        # metadata directory.
+        if len([s for s in self.get_all_specs()
+                if needs_file(s, dest)]) <= 1:
             os.remove(dest)
 
     def check_added(self, spec):
diff --git a/lib/spack/spack/test/cmd/view.py b/lib/spack/spack/test/cmd/view.py
index d908248a19..4ff1592035 100644
--- a/lib/spack/spack/test/cmd/view.py
+++ b/lib/spack/spack/test/cmd/view.py
@@ -40,6 +40,21 @@ def test_view_link_type(
     assert os.path.islink(package_prefix) == is_link_cmd
 
 
+@pytest.mark.parametrize('add_cmd', ['hardlink', 'symlink', 'hard', 'add',
+                                     'copy', 'relocate'])
+def test_view_link_type_remove(
+        tmpdir, mock_packages, mock_archive, mock_fetch, config,
+        install_mockery, add_cmd):
+    install('needs-relocation')
+    viewpath = str(tmpdir.mkdir('view_{0}'.format(add_cmd)))
+    view(add_cmd, viewpath, 'needs-relocation')
+    bindir = os.path.join(viewpath, 'bin')
+    assert os.path.exists(bindir)
+
+    view('remove', viewpath, 'needs-relocation')
+    assert not os.path.exists(bindir)
+
+
 @pytest.mark.parametrize('cmd', ['hardlink', 'symlink', 'hard', 'add',
                                  'copy', 'relocate'])
 def test_view_projections(
diff --git a/var/spack/repos/builtin.mock/packages/needs-relocation/package.py b/var/spack/repos/builtin.mock/packages/needs-relocation/package.py
new file mode 100644
index 0000000000..681a8a53ed
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/needs-relocation/package.py
@@ -0,0 +1,28 @@
+# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack import *
+
+
+def check(condition, msg):
+    """Raise an install error if condition is False."""
+    if not condition:
+        raise InstallError(msg)
+
+
+class NeedsRelocation(Package):
+    """A dumy package that encodes its prefix."""
+    homepage  = 'https://www.cmake.org'
+    url       = 'https://cmake.org/files/v3.4/cmake-3.4.3.tar.gz'
+
+    version('0.0.0', '12345678qwertyuiasdfghjkzxcvbnm0')
+
+    def install(self, spec, prefix):
+        mkdirp(prefix.bin)
+
+        exe = join_path(prefix.bin, 'exe')
+        with open(exe, 'w') as f:
+            f.write(prefix)
+        set_executable(exe)