From 68b711c1ad7157930154fc37f4a912aaf325fbfb Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Thu, 16 Feb 2023 19:36:22 +0100 Subject: [PATCH] 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 | 2 +- lib/spack/spack/filesystem_view.py | 34 ++++++++++++++-------- lib/spack/spack/test/views.py | 45 +++++++++++++++++++++++++++++- 3 files changed, 68 insertions(+), 13 deletions(-) diff --git a/lib/spack/llnl/util/link_tree.py b/lib/spack/llnl/util/link_tree.py index f1ca5ebe34..52268bc3f8 100644 --- a/lib/spack/llnl/util/link_tree.py +++ b/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): diff --git a/lib/spack/spack/filesystem_view.py b/lib/spack/spack/filesystem_view.py index 41ef8c93a8..058a952216 100644 --- a/lib/spack/spack/filesystem_view.py +++ b/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... - # For compatibility, we have to create a merge_map dict mapping - # full_src => full_dst - files_per_spec = itertools.groupby(visitor.files.items(), key=lambda item: item[1][0]) - - for (spec, (src_root, rel_paths)) in zip(specs, files_per_spec): - merge_map = dict() - 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 + # Link the files using a "merge map": full src => full dst + merge_map_per_prefix = self._source_merge_visitor_to_merge_map(visitor) + for spec in specs: + merge_map = merge_map_per_prefix.get(spec.package.view_source(), None) + if not merge_map: + # Not every spec may have files to contribute. + continue 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() diff --git a/lib/spack/spack/test/views.py b/lib/spack/spack/test/views.py index 43eb56e31e..77dd86d411 100644 --- a/lib/spack/spack/test/views.py +++ b/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"))