view: fix issue with non-contributing specs (#34661)

Specs that did not contribute any files to an env view caused a problem
where zip(specs, files grouped by prefix) got "out of sync", causing the
wrong merge map to be passed to a package's `add_files_to_view`, which
specifically caused an issue where *sometimes* bin/python ended up as a
symlink instead of a copy.

One such example is kokkos + kokkos-nvcc-wrapper, as the latter package
only provides the file bin/nvcc_wrapper, which is also added to view by
kokkos, causing kokkos-nvcc-wrapper to contribute 0 files.

The test feels a bit contrived, but it captures the problem... pkg a is
added first and has 0 files to contribute, pkg b adds a single file, and
we check if pkg b receives a merge map (and a does not).

lib/spack/llnl/util/link_tree.py

@@ -75,7 +75,7 @@ def __init__(self, ignore=None):
         # so that we have a fast lookup and can run mkdir in order.
         self.directories = OrderedDict()
 
-        # Files to link. Maps dst_rel to (src_rel, src_root)
+        # Files to link. Maps dst_rel to (src_root, src_rel)
         self.files = OrderedDict()
 
     def before_visit_dir(self, root, rel_path, depth):

lib/spack/spack/filesystem_view.py

@@ -686,22 +686,34 @@ def skip_list(file):
         for dst in visitor.directories:
             os.mkdir(os.path.join(self._root, dst))
 
-        # Then group the files to be linked by spec...
+        # Link the files using a "merge map": full src => full dst
-        # For compatibility, we have to create a merge_map dict mapping
+        merge_map_per_prefix = self._source_merge_visitor_to_merge_map(visitor)
-        # full_src => full_dst
+        for spec in specs:
-        files_per_spec = itertools.groupby(visitor.files.items(), key=lambda item: item[1][0])
+            merge_map = merge_map_per_prefix.get(spec.package.view_source(), None)
-
+            if not merge_map:
-        for (spec, (src_root, rel_paths)) in zip(specs, files_per_spec):
+                # Not every spec may have files to contribute.
-            merge_map = dict()
+                continue
-            for dst_rel, (_, src_rel) in rel_paths:
-                full_src = os.path.join(src_root, src_rel)
-                full_dst = os.path.join(self._root, dst_rel)
-                merge_map[full_src] = full_dst
             spec.package.add_files_to_view(self, merge_map, skip_if_exists=False)
 
         # Finally create the metadata dirs.
         self.link_metadata(specs)
 
+    def _source_merge_visitor_to_merge_map(self, visitor: SourceMergeVisitor):
+        # For compatibility with add_files_to_view, we have to create a
+        # merge_map of the form join(src_root, src_rel) => join(dst_root, dst_rel),
+        # but our visitor.files format is dst_rel => (src_root, src_rel).
+        # We exploit that visitor.files is an ordered dict, and files per source
+        # prefix are contiguous.
+        source_root = lambda item: item[1][0]
+        per_source = itertools.groupby(visitor.files.items(), key=source_root)
+        return {
+            src_root: {
+                os.path.join(src_root, src_rel): os.path.join(self._root, dst_rel)
+                for dst_rel, (_, src_rel) in group
+            }
+            for src_root, group in per_source
+        }
+
     def link_metadata(self, specs):
         metadata_visitor = SourceMergeVisitor()
 

lib/spack/spack/test/views.py

@@ -3,12 +3,13 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import os
 import sys
 
 import pytest
 
 from spack.directory_layout import DirectoryLayout
-from spack.filesystem_view import YamlFilesystemView
+from spack.filesystem_view import SimpleFilesystemView, YamlFilesystemView
 from spack.spec import Spec
 
 
@@ -23,3 +24,45 @@ def test_remove_extensions_ordered(install_mockery, mock_fetch, tmpdir):
 
     e1 = e2["extension1"]
     view.remove_specs(e1, e2)
+
+
+@pytest.mark.regression("32456")
+def test_view_with_spec_not_contributing_files(mock_packages, tmpdir):
+    view_dir = os.path.join(str(tmpdir), "view")
+    os.mkdir(view_dir)
+
+    layout = DirectoryLayout(view_dir)
+    view = SimpleFilesystemView(view_dir, layout)
+
+    a = Spec("a")
+    b = Spec("b")
+    a.prefix = os.path.join(tmpdir, "a")
+    b.prefix = os.path.join(tmpdir, "b")
+    a._mark_concrete()
+    b._mark_concrete()
+
+    # Create directory structure for a and b, and view
+    os.makedirs(a.prefix.subdir)
+    os.makedirs(b.prefix.subdir)
+    os.makedirs(os.path.join(a.prefix, ".spack"))
+    os.makedirs(os.path.join(b.prefix, ".spack"))
+
+    # Add files to b's prefix, but not to a's
+    with open(b.prefix.file, "w") as f:
+        f.write("file 1")
+
+    with open(b.prefix.subdir.file, "w") as f:
+        f.write("file 2")
+
+    # In previous versions of Spack we incorrectly called add_files_to_view
+    # with b's merge map. It shouldn't be called at all, since a has no
+    # files to add to the view.
+    def pkg_a_add_files_to_view(view, merge_map, skip_if_exists=True):
+        assert False, "There shouldn't be files to add"
+
+    a.package.add_files_to_view = pkg_a_add_files_to_view
+
+    # Create view and see if files are linked.
+    view.add_specs(a, b)
+    assert os.path.lexists(os.path.join(view_dir, "file"))
+    assert os.path.lexists(os.path.join(view_dir, "subdir", "file"))