binary_distribution.py: stop relocation old /bin/bash sbang shebang (#48502)

lib/spack/spack/binary_distribution.py

@@ -2125,10 +2125,9 @@ def fetch_url_to_mirror(url):
 
 
 def dedupe_hardlinks_if_necessary(root, buildinfo):
-    """Updates a buildinfo dict for old archives that did
+    """Updates a buildinfo dict for old archives that did not dedupe hardlinks. De-duping hardlinks
-    not dedupe hardlinks. De-duping hardlinks is necessary
+    is necessary when relocating files in parallel and in-place. This means we must preserve inodes
-    when relocating files in parallel and in-place. This
+    when relocating."""
-    means we must preserve inodes when relocating."""
 
     # New archives don't need this.
     if buildinfo.get("hardlinks_deduped", False):
@@ -2157,69 +2156,46 @@ def dedupe_hardlinks_if_necessary(root, buildinfo):
         buildinfo[key] = new_list
 
 
-def relocate_package(spec):
+def relocate_package(spec: spack.spec.Spec) -> None:
-    """
+    """Relocate binaries and text files in the given spec prefix, based on its buildinfo file."""
-    Relocate the given package
+    buildinfo = read_buildinfo_file(spec.prefix)
-    """
-    workdir = str(spec.prefix)
-    buildinfo = read_buildinfo_file(workdir)
-    new_layout_root = str(spack.store.STORE.layout.root)
-    new_prefix = str(spec.prefix)
-    new_rel_prefix = str(os.path.relpath(new_prefix, new_layout_root))
-    new_spack_prefix = str(spack.paths.prefix)
-
-    old_sbang_install_path = None
-    if "sbang_install_path" in buildinfo:
-        old_sbang_install_path = str(buildinfo["sbang_install_path"])
     old_layout_root = str(buildinfo["buildpath"])
-    old_spack_prefix = str(buildinfo.get("spackprefix"))
-    old_rel_prefix = buildinfo.get("relative_prefix")
-    old_prefix = os.path.join(old_layout_root, old_rel_prefix)
 
-    # Warn about old style tarballs created with the now removed --rel flag.
+    # Warn about old style tarballs created with the --rel flag (removed in Spack v0.20)
     if buildinfo.get("relative_rpaths", False):
         tty.warn(
-            f"Tarball for {spec} uses relative rpaths, " "which can cause library loading issues."
+            f"Tarball for {spec} uses relative rpaths, which can cause library loading issues."
         )
 
-    # In the past prefix_to_hash was the default and externals were not dropped, so prefixes
+    # In Spack 0.19 and older prefix_to_hash was the default and externals were not dropped, so
-    # were not unique.
+    # prefixes were not unique.
     if "hash_to_prefix" in buildinfo:
         hash_to_old_prefix = buildinfo["hash_to_prefix"]
     elif "prefix_to_hash" in buildinfo:
-        hash_to_old_prefix = dict((v, k) for (k, v) in buildinfo["prefix_to_hash"].items())
+        hash_to_old_prefix = {v: k for (k, v) in buildinfo["prefix_to_hash"].items()}
     else:
-        hash_to_old_prefix = dict()
+        raise NewLayoutException(
+            "Package tarball was created from an install prefix with a different directory layout "
+            "and an older buildcache create implementation. It cannot be relocated."
+        )
 
-    if old_rel_prefix != new_rel_prefix and not hash_to_old_prefix:
+    prefix_to_prefix = {}
-        msg = "Package tarball was created from an install "
-        msg += "prefix with a different directory layout and an older "
-        msg += "buildcache create implementation. It cannot be relocated."
-        raise NewLayoutException(msg)
 
-    # Spurious replacements (e.g. sbang) will cause issues with binaries
+    if "sbang_install_path" in buildinfo:
-    # For example, the new sbang can be longer than the old one.
+        old_sbang_install_path = str(buildinfo["sbang_install_path"])
-    # Hence 2 dictionaries are maintained here.
+        prefix_to_prefix[old_sbang_install_path] = spack.hooks.sbang.sbang_install_path()
-    prefix_to_prefix_text = collections.OrderedDict()
-    prefix_to_prefix_bin = collections.OrderedDict()
 
-    if old_sbang_install_path:
+    # First match specific prefix paths. Possibly the *local* install prefix of some dependency is
-        install_path = spack.hooks.sbang.sbang_install_path()
+    # in an upstream, so we cannot assume the original spack store root can be mapped uniformly to
-        prefix_to_prefix_text[old_sbang_install_path] = install_path
+    # the new spack store root.
 
-    # First match specific prefix paths. Possibly the *local* install prefix
+    # If the spec is spliced, we need to handle the simultaneous mapping from the old install_tree
-    # of some dependency is in an upstream, so we cannot assume the original
+    # to the new install_tree and from the build_spec to the spliced spec. Because foo.build_spec
-    # spack store root can be mapped uniformly to the new spack store root.
+    # is foo for any non-spliced spec, we can simplify by checking for spliced-in nodes by checking
-    #
+    # for nodes not in the build_spec without any explicit check for whether the spec is spliced.
-    # If the spec is spliced, we need to handle the simultaneous mapping
+    # An analog in this algorithm is any spec that shares a name or provides the same virtuals in
-    # from the old install_tree to the new install_tree and from the build_spec
+    # the context of the relevant root spec. This ensures that the analog for a spec s is the spec
-    # to the spliced spec.
+    # that s replaced when we spliced.
-    # Because foo.build_spec is foo for any non-spliced spec, we can simplify
-    # by checking for spliced-in nodes by checking for nodes not in the build_spec
-    # without any explicit check for whether the spec is spliced.
-    # An analog in this algorithm is any spec that shares a name or provides the same virtuals
-    # in the context of the relevant root spec. This ensures that the analog for a spec s
-    # is the spec that s replaced when we spliced.
     relocation_specs = specs_to_relocate(spec)
     build_spec_ids = set(id(s) for s in spec.build_spec.traverse(deptype=dt.ALL & ~dt.BUILD))
     for s in relocation_specs:
@@ -2239,56 +2215,38 @@ def relocate_package(spec):
         lookup_dag_hash = analog.dag_hash()
         if lookup_dag_hash in hash_to_old_prefix:
             old_dep_prefix = hash_to_old_prefix[lookup_dag_hash]
-            prefix_to_prefix_bin[old_dep_prefix] = str(s.prefix)
+            prefix_to_prefix[old_dep_prefix] = str(s.prefix)
-            prefix_to_prefix_text[old_dep_prefix] = str(s.prefix)
 
     # Only then add the generic fallback of install prefix -> install prefix.
-    prefix_to_prefix_text[old_prefix] = new_prefix
+    prefix_to_prefix[old_layout_root] = str(spack.store.STORE.layout.root)
-    prefix_to_prefix_bin[old_prefix] = new_prefix
-    prefix_to_prefix_text[old_layout_root] = new_layout_root
-    prefix_to_prefix_bin[old_layout_root] = new_layout_root
 
-    # This is vestigial code for the *old* location of sbang. Previously,
+    # Delete identity mappings from prefix_to_prefix
-    # sbang was a bash script, and it lived in the spack prefix. It is
+    prefix_to_prefix = {k: v for k, v in prefix_to_prefix.items() if k != v}
-    # now a POSIX script that lives in the install prefix. Old packages
-    # will have the old sbang location in their shebangs.
-    orig_sbang = "#!/bin/bash {0}/bin/sbang".format(old_spack_prefix)
-    new_sbang = spack.hooks.sbang.sbang_shebang_line()
-    prefix_to_prefix_text[orig_sbang] = new_sbang
 
-    tty.debug("Relocating package from", "%s to %s." % (old_layout_root, new_layout_root))
+    # If there's nothing to relocate, we're done.
+    if not prefix_to_prefix:
+        return
+
+    for old, new in prefix_to_prefix.items():
+        tty.debug(f"Relocating: {old} => {new}.")
 
     # Old archives may have hardlinks repeated.
-    dedupe_hardlinks_if_necessary(workdir, buildinfo)
+    dedupe_hardlinks_if_necessary(spec.prefix, buildinfo)
 
     # Text files containing the prefix text
-    text_names = [os.path.join(workdir, f) for f in buildinfo["relocate_textfiles"]]
+    textfiles = [os.path.join(spec.prefix, f) for f in buildinfo["relocate_textfiles"]]
+    binaries = [os.path.join(spec.prefix, f) for f in buildinfo.get("relocate_binaries")]
+    links = [os.path.join(spec.prefix, f) for f in buildinfo.get("relocate_links", [])]
 
-    # If we are not installing back to the same install tree do the relocation
-    if old_prefix != new_prefix:
-        files_to_relocate = [
-            os.path.join(workdir, filename) for filename in buildinfo.get("relocate_binaries")
-        ]
-        # If the buildcache was not created with relativized rpaths
-        # do the relocation of path in binaries
     platform = spack.platforms.by_name(spec.platform)
     if "macho" in platform.binary_formats:
-            relocate.relocate_macho_binaries(files_to_relocate, prefix_to_prefix_bin)
+        relocate.relocate_macho_binaries(binaries, prefix_to_prefix)
     elif "elf" in platform.binary_formats:
-            # The new ELF dynamic section relocation logic only handles absolute to
+        relocate.relocate_elf_binaries(binaries, prefix_to_prefix)
-            # absolute relocation.
-            relocate.relocate_elf_binaries(files_to_relocate, prefix_to_prefix_bin)
 
-        # Relocate links to the new install prefix
+    relocate.relocate_links(links, prefix_to_prefix)
-        links = [os.path.join(workdir, f) for f in buildinfo.get("relocate_links", [])]
+    relocate.relocate_text(textfiles, prefix_to_prefix)
-        relocate.relocate_links(links, prefix_to_prefix_bin)
+    changed_files = relocate.relocate_text_bin(binaries, prefix_to_prefix)
-
-        # For all buildcaches
-        # relocate the install prefixes in text files including dependencies
-        relocate.relocate_text(text_names, prefix_to_prefix_text)
-
-        # relocate the install prefixes in binary files including dependencies
-        changed_files = relocate.relocate_text_bin(files_to_relocate, prefix_to_prefix_bin)
 
     # Add ad-hoc signatures to patched macho files when on macOS.
     if "macho" in platform.binary_formats and sys.platform == "darwin":
@@ -2300,12 +2258,6 @@ def relocate_package(spec):
             with fsys.edit_in_place_through_temporary_file(binary) as tmp_binary:
                 codesign("-fs-", tmp_binary)
 
-    # If we are installing back to the same location
-    # relocate the sbang location if the spack directory changed
-    else:
-        if old_spack_prefix != new_spack_prefix:
-            relocate.relocate_text(text_names, prefix_to_prefix_text)
-
 
 def _extract_inner_tarball(spec, filename, extract_to, signature_required: bool, remote_checksum):
     stagepath = os.path.dirname(filename)

lib/spack/spack/filesystem_view.py

@@ -35,7 +35,6 @@
 
 import spack.config
 import spack.directory_layout
-import spack.paths
 import spack.projections
 import spack.relocate
 import spack.schema.projections
@@ -44,7 +43,6 @@
 import spack.util.spack_json as s_json
 import spack.util.spack_yaml as s_yaml
 from spack.error import SpackError
-from spack.hooks import sbang
 
 __all__ = ["FilesystemView", "YamlFilesystemView"]
 
@@ -94,12 +92,6 @@ def view_copy(
         spack.relocate.relocate_text_bin(binaries=[dst], prefixes=prefix_to_projection)
     else:
         prefix_to_projection[spack.store.STORE.layout.root] = view._root
-
-        # This is vestigial code for the *old* location of sbang.
-        prefix_to_projection[f"#!/bin/bash {spack.paths.spack_root}/bin/sbang"] = (
-            sbang.sbang_shebang_line()
-        )
-
         spack.relocate.relocate_text(files=[dst], prefixes=prefix_to_projection)
 
     # The os module on Windows does not have a chown function.

lib/spack/spack/test/bindist.py

@@ -36,13 +36,13 @@
 import spack.oci.image
 import spack.paths
 import spack.spec
-import spack.stage
 import spack.store
 import spack.util.gpg
 import spack.util.spack_yaml as syaml
 import spack.util.url as url_util
 import spack.util.web as web_util
 from spack.binary_distribution import CannotListKeys, GenerateIndexError
+from spack.installer import PackageInstaller
 from spack.paths import test_path
 from spack.spec import Spec
 
@@ -492,74 +492,40 @@ def mock_list_url(url, recursive=False):
     assert f"Encountered problem listing packages at {url}" in capfd.readouterr().err
 
 
-@pytest.mark.usefixtures("mock_fetch", "install_mockery")
+def test_update_sbang(tmp_path, temporary_mirror, mock_fetch, install_mockery):
-def test_update_sbang(tmpdir, temporary_mirror):
+    """Test relocation of the sbang shebang line in a package script"""
-    """Test the creation and installation of buildcaches with default rpaths
+    s = Spec("old-sbang").concretized()
-    into the non-default directory layout scheme, triggering an update of the
+    PackageInstaller([s.package]).install()
-    sbang.
+    old_prefix, old_sbang_shebang = s.prefix, sbang.sbang_shebang_line()
+    old_contents = f"""\
+{old_sbang_shebang}
+#!/usr/bin/env python3
+
+{s.prefix.bin}
 """
-    spec_str = "old-sbang"
+    with open(os.path.join(s.prefix.bin, "script.sh"), encoding="utf-8") as f:
-    # Concretize a package with some old-fashioned sbang lines.
+        assert f.read() == old_contents
-    old_spec = Spec(spec_str).concretized()
-    old_spec_hash_str = "/{0}".format(old_spec.dag_hash())
-
-    # Need a fake mirror with *function* scope.
-    mirror_dir = temporary_mirror
-
-    # Assume all commands will concretize old_spec the same way.
-    install_cmd("--no-cache", old_spec.name)
 
     # Create a buildcache with the installed spec.
-    buildcache_cmd("push", "-u", mirror_dir, old_spec_hash_str)
+    buildcache_cmd("push", "--update-index", "--unsigned", temporary_mirror, f"/{s.dag_hash()}")
-
-    # Need to force an update of the buildcache index
-    buildcache_cmd("update-index", mirror_dir)
-
-    # Uninstall the original package.
-    uninstall_cmd("-y", old_spec_hash_str)
 
     # Switch the store to the new install tree locations
-    newtree_dir = tmpdir.join("newtree")
+    with spack.store.use_store(str(tmp_path)):
-    with spack.store.use_store(str(newtree_dir)):
+        s._prefix = None  # clear the cached old prefix
-        new_spec = Spec("old-sbang").concretized()
+        new_prefix, new_sbang_shebang = s.prefix, sbang.sbang_shebang_line()
-        assert new_spec.dag_hash() == old_spec.dag_hash()
+        assert old_prefix != new_prefix
+        assert old_sbang_shebang != new_sbang_shebang
+        PackageInstaller([s.package], cache_only=True, unsigned=True).install()
 
-        # Install package from buildcache
+        # Check that the sbang line refers to the new install tree
-        buildcache_cmd("install", "-u", "-f", new_spec.name)
+        new_contents = f"""\
+{sbang.sbang_shebang_line()}
+#!/usr/bin/env python3
 
-        # Continue blowing away caches
+{s.prefix.bin}
-        bindist.clear_spec_cache()
+"""
-        spack.stage.purge()
+        with open(os.path.join(s.prefix.bin, "script.sh"), encoding="utf-8") as f:
-
+            assert f.read() == new_contents
-        # test that the sbang was updated by the move
-        sbang_style_1_expected = """{0}
-#!/usr/bin/env python
-
-{1}
-""".format(
-            sbang.sbang_shebang_line(), new_spec.prefix.bin
-        )
-        sbang_style_2_expected = """{0}
-#!/usr/bin/env python
-
-{1}
-""".format(
-            sbang.sbang_shebang_line(), new_spec.prefix.bin
-        )
-
-        installed_script_style_1_path = new_spec.prefix.bin.join("sbang-style-1.sh")
-        assert (
-            sbang_style_1_expected
-            == open(str(installed_script_style_1_path), encoding="utf-8").read()
-        )
-
-        installed_script_style_2_path = new_spec.prefix.bin.join("sbang-style-2.sh")
-        assert (
-            sbang_style_2_expected
-            == open(str(installed_script_style_2_path), encoding="utf-8").read()
-        )
-
-        uninstall_cmd("-y", "/%s" % new_spec.dag_hash())
 
 
 @pytest.mark.skipif(

var/spack/repos/builtin.mock/packages/old-sbang/package.py

@@ -1,13 +1,14 @@
 # Copyright Spack Project Developers. See COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-import spack.paths
+import os
-import spack.store
+
+from spack.hooks.sbang import sbang_shebang_line
 from spack.package import *
 
 
 class OldSbang(Package):
-    """Toy package for testing the old sbang replacement problem"""
+    """Package for testing sbang relocation"""
 
     homepage = "https://www.example.com"
     url = "https://www.example.com/old-sbang.tar.gz"
@@ -16,23 +17,11 @@ class OldSbang(Package):
 
     def install(self, spec, prefix):
         mkdirp(prefix.bin)
+        contents = f"""\
+{sbang_shebang_line()}
+#!/usr/bin/env python3
 
-        sbang_style_1 = """#!/bin/bash {0}/bin/sbang
+{prefix.bin}
-#!/usr/bin/env python
+"""
-
+        with open(os.path.join(self.prefix.bin, "script.sh"), "w", encoding="utf-8") as f:
-{1}
+            f.write(contents)
-""".format(
-            spack.paths.prefix, prefix.bin
-        )
-        sbang_style_2 = """#!/bin/sh {0}/bin/sbang
-#!/usr/bin/env python
-
-{1}
-""".format(
-            spack.store.STORE.unpadded_root, prefix.bin
-        )
-        with open("%s/sbang-style-1.sh" % self.prefix.bin, "w", encoding="utf-8") as f:
-            f.write(sbang_style_1)
-
-        with open("%s/sbang-style-2.sh" % self.prefix.bin, "w", encoding="utf-8") as f:
-            f.write(sbang_style_2)