buildcache push: make --allow-root the default and deprecate the option (#38878)

Without --allow-root spack cannot push binaries that contain paths in
binaries. This flag is almost always needed, so there is no point of
requiring users to spell it out. 

Even without --allow-root, rpaths would still have to be patched, so the 
flag is not there to guarantee binaries are not modified on install.

This commit makes --allow-root the default, and drops the code 
required for it. It also deprecates `spack buildcache preview`, since 
the command made sense only with --allow-root.

As a side effect, Spack no longer depends on binutils for relocation

lib/spack/spack/binary_distribution.py

@@ -1218,9 +1218,6 @@ class PushOptions(NamedTuple):
     #: Overwrite existing tarball/metadata files in buildcache
     force: bool = False
 
-    #: Allow absolute paths to package prefixes when creating a tarball
-    allow_root: bool = False
-
     #: Regenerated indices after pushing
     regenerate_index: bool = False
 
@@ -1293,9 +1290,6 @@ def _build_tarball_in_stage_dir(spec: Spec, out_url: str, stage_dir: str, option
     # create info for later relocation and create tar
     buildinfo = get_buildinfo_dict(spec)
 
-    if not options.allow_root:
-        ensure_package_relocatable(buildinfo, binaries_dir)
-
     _do_create_tarball(tarfile_path, binaries_dir, pkg_dir, buildinfo)
 
     # get the sha256 checksum of the tarball
@@ -1574,12 +1568,6 @@ def download_tarball(spec, unsigned=False, mirrors_for_spec=None):
     return None
 
 
-def ensure_package_relocatable(buildinfo, binaries_dir):
-    """Check if package binaries are relocatable."""
-    binaries = [os.path.join(binaries_dir, f) for f in buildinfo["relocate_binaries"]]
-    relocate.ensure_binaries_are_relocatable(binaries)
-
-
 def dedupe_hardlinks_if_necessary(root, buildinfo):
     """Updates a buildinfo dict for old archives that did
     not dedupe hardlinks. De-duping hardlinks is necessary

lib/spack/spack/ci.py

@@ -1419,9 +1419,7 @@ def _push_mirror_contents(input_spec, sign_binaries, mirror_url):
     unsigned = not sign_binaries
     tty.debug("Creating buildcache ({0})".format("unsigned" if unsigned else "signed"))
     push_url = spack.mirror.Mirror.from_url(mirror_url).push_url
-    return bindist.push(
+    return bindist.push(input_spec, push_url, bindist.PushOptions(force=True, unsigned=unsigned))
-        input_spec, push_url, bindist.PushOptions(force=True, allow_root=True, unsigned=unsigned)
-    )
 
 
 def push_mirror_contents(input_spec: spack.spec.Spec, mirror_url, sign_binaries):

lib/spack/spack/cmd/buildcache.py

@@ -307,6 +307,11 @@ def push_fn(args):
     """create a binary package and push it to a mirror"""
     mirror = arguments.mirror_name_or_url(args.mirror)
 
+    if args.allow_root:
+        tty.warn(
+            "The flag `--allow-root` is the default in Spack 0.21, will be removed in Spack 0.22"
+        )
+
     url = mirror.push_url
 
     specs = bindist.specs_to_be_packaged(
@@ -336,7 +341,6 @@ def push_fn(args):
                 bindist.PushOptions(
                     force=args.force,
                     unsigned=args.unsigned,
-                    allow_root=args.allow_root,
                     key=args.key,
                     regenerate_index=args.update_index,
                 ),
@@ -410,14 +414,10 @@ def keys_fn(args):
 
 def preview_fn(args):
     """analyze an installed spec and reports whether executables and libraries are relocatable"""
-    constraints = spack.cmd.parse_specs(args.specs)
+    tty.warn(
-    specs = spack.store.find(constraints, multiple=True)
+        "`spack buildcache preview` is deprecated since `spack buildcache push --allow-root` is "
-
+        "now the default. This command will be removed in Spack 0.22"
-    # Cycle over the specs that match
+    )
-    for spec in specs:
-        print("Relocatable nodes")
-        print("--------------------------------")
-        print(spec.tree(status_fn=spack.relocate.is_relocatable))
 
 
 def check_fn(args):

lib/spack/spack/relocate.py

@@ -599,19 +599,6 @@ def make_elf_binaries_relative(new_binaries, orig_binaries, orig_layout_root):
             _set_elf_rpaths(new_binary, new_rpaths)
 
 
-def ensure_binaries_are_relocatable(binaries):
-    """Raise an error if any binary in the list is not relocatable.
-
-    Args:
-        binaries (list): list of binaries to check
-
-    Raises:
-        InstallRootStringError: if the file is not relocatable
-    """
-    for binary in binaries:
-        ensure_binary_is_relocatable(binary)
-
-
 def warn_if_link_cant_be_relocated(link, target):
     if not os.path.isabs(target):
         return
@@ -663,83 +650,6 @@ def relocate_text_bin(binaries, prefixes):
     return BinaryFilePrefixReplacer.from_strings_or_bytes(prefixes).apply(binaries)
 
 
-def is_relocatable(spec):
-    """Returns True if an installed spec is relocatable.
-
-    Args:
-        spec (spack.spec.Spec): spec to be analyzed
-
-    Returns:
-        True if the binaries of an installed spec
-        are relocatable and False otherwise.
-
-    Raises:
-        ValueError: if the spec is not installed
-    """
-    if not spec.installed:
-        raise ValueError("spec is not installed [{0}]".format(str(spec)))
-
-    if spec.external or spec.virtual:
-        tty.warn("external or virtual package %s is not relocatable" % spec.name)
-        return False
-
-    # Explore the installation prefix of the spec
-    for root, dirs, files in os.walk(spec.prefix, topdown=True):
-        dirs[:] = [d for d in dirs if d not in (".spack", "man")]
-        try:
-            abs_paths = (os.path.join(root, f) for f in files)
-            ensure_binaries_are_relocatable(filter(is_binary, abs_paths))
-        except InstallRootStringError:
-            return False
-
-    return True
-
-
-def ensure_binary_is_relocatable(filename, paths_to_relocate=None):
-    """Raises if any given or default absolute path is found in the
-    binary (apart from rpaths / load commands).
-
-    Args:
-        filename: absolute path of the file to be analyzed
-
-    Raises:
-        InstallRootStringError: if the binary contains an absolute path
-        ValueError: if the filename does not exist or the path is not absolute
-    """
-    paths_to_relocate = paths_to_relocate or [spack.store.layout.root, spack.paths.prefix]
-
-    if not os.path.exists(filename):
-        raise ValueError("{0} does not exist".format(filename))
-
-    if not os.path.isabs(filename):
-        raise ValueError("{0} is not an absolute path".format(filename))
-
-    strings = executable.Executable("strings")
-
-    # Remove the RPATHS from the strings in the executable
-    set_of_strings = set(strings(filename, output=str).split())
-
-    m_type, m_subtype = fs.mime_type(filename)
-    if m_type == "application":
-        tty.debug("{0},{1}".format(m_type, m_subtype), level=2)
-
-    if not is_macos:
-        if m_subtype == "x-executable" or m_subtype == "x-sharedlib":
-            rpaths = ":".join(_elf_rpaths_for(filename))
-            set_of_strings.discard(rpaths)
-    else:
-        if m_subtype == "x-mach-binary":
-            rpaths, deps, idpath = macholib_get_paths(filename)
-            set_of_strings.discard(set(rpaths))
-            set_of_strings.discard(set(deps))
-            if idpath is not None:
-                set_of_strings.discard(idpath)
-
-    for path_to_relocate in paths_to_relocate:
-        if any(path_to_relocate in x for x in set_of_strings):
-            raise InstallRootStringError(filename, path_to_relocate)
-
-
 def is_binary(filename):
     """Returns true if a file is binary, False otherwise
 

lib/spack/spack/test/bindist.py

@@ -176,7 +176,7 @@ def install_dir_non_default_layout(tmpdir):
         spack.store.layout = real_layout
 
 
-args = ["strings", "file"]
+args = ["file"]
 if sys.platform == "darwin":
     args.extend(["/usr/bin/clang++", "install_name_tool"])
 else:
@@ -201,12 +201,14 @@ def test_default_rpaths_create_install_default_layout(mirror_dir):
     install_cmd("--no-cache", sy_spec.name)
 
     # Create a buildache
-    buildcache_cmd("push", "-au", mirror_dir, cspec.name, sy_spec.name)
+    buildcache_cmd("push", "-u", mirror_dir, cspec.name, sy_spec.name)
+
     # Test force overwrite create buildcache (-f option)
-    buildcache_cmd("push", "-auf", mirror_dir, cspec.name)
+    buildcache_cmd("push", "-uf", mirror_dir, cspec.name)
 
     # Create mirror index
     buildcache_cmd("update-index", mirror_dir)
+
     # List the buildcaches in the mirror
     buildcache_cmd("list", "-alv")
 
@@ -376,7 +378,7 @@ def test_spec_needs_rebuild(monkeypatch, tmpdir):
     install_cmd(s.name)
 
     # Put installed package in the buildcache
-    buildcache_cmd("push", "-u", "-a", mirror_dir.strpath, s.name)
+    buildcache_cmd("push", "-u", mirror_dir.strpath, s.name)
 
     rebuild = bindist.needs_rebuild(s, mirror_url)
 
@@ -405,7 +407,7 @@ def test_generate_index_missing(monkeypatch, tmpdir, mutable_config):
     install_cmd("--no-cache", s.name)
 
     # Create a buildcache and update index
-    buildcache_cmd("push", "-ua", mirror_dir.strpath, s.name)
+    buildcache_cmd("push", "-u", mirror_dir.strpath, s.name)
     buildcache_cmd("update-index", mirror_dir.strpath)
 
     # Check package and dependency in buildcache
@@ -491,7 +493,7 @@ def test_update_sbang(tmpdir, test_mirror):
     install_cmd("--no-cache", old_spec.name)
 
     # Create a buildcache with the installed spec.
-    buildcache_cmd("push", "-u", "-a", mirror_dir, old_spec_hash_str)
+    buildcache_cmd("push", "-u", mirror_dir, old_spec_hash_str)
 
     # Need to force an update of the buildcache index
     buildcache_cmd("update-index", mirror_dir)

lib/spack/spack/test/cmd/buildcache.py

@@ -5,7 +5,6 @@
 
 import errno
 import os
-import platform
 import shutil
 import sys
 
@@ -49,11 +48,8 @@ def mock_get_specs_multiarch(database, monkeypatch):
     monkeypatch.setattr(spack.binary_distribution, "update_cache_and_get_specs", lambda: specs)
 
 
-@pytest.mark.skipif(
+def test_buildcache_preview_just_runs():
-    platform.system().lower() != "linux", reason="implementation for MacOS still missing"
+    # TODO: remove in Spack 0.21
-)
-@pytest.mark.db
-def test_buildcache_preview_just_runs(database):
     buildcache("preview", "mpileaks")
 
 
@@ -159,7 +155,7 @@ def test_update_key_index(
     # Put installed package in the buildcache, which, because we're signing
     # it, should result in the public key getting pushed to the buildcache
     # as well.
-    buildcache("push", "-a", mirror_dir.strpath, s.name)
+    buildcache("push", mirror_dir.strpath, s.name)
 
     # Now make sure that when we pass the "--keys" argument to update-index
     # it causes the index to get update.
@@ -213,13 +209,13 @@ def verify_mirror_contents():
     # Install a package and put it in the buildcache
     s = Spec(out_env_pkg).concretized()
     install(s.name)
-    buildcache("push", "-u", "-f", "-a", src_mirror_url, s.name)
+    buildcache("push", "-u", "-f", src_mirror_url, s.name)
 
     env("create", "test")
     with ev.read("test"):
         add(in_env_pkg)
         install()
-        buildcache("push", "-u", "-f", "-a", src_mirror_url, in_env_pkg)
+        buildcache("push", "-u", "-f", src_mirror_url, in_env_pkg)
 
         # Now run the spack buildcache sync command with all the various options
         # for specifying mirrors

lib/spack/spack/test/cmd/ci.py

@@ -895,7 +895,7 @@ def test_ci_nothing_to_rebuild(
     )
 
     install_cmd("archive-files")
-    buildcache_cmd("push", "-a", "-f", "-u", mirror_url, "archive-files")
+    buildcache_cmd("push", "-f", "-u", mirror_url, "archive-files")
 
     filename = str(tmpdir.join("spack.yaml"))
     with open(filename, "w") as f:
@@ -1458,7 +1458,7 @@ def test_ci_rebuild_index(
                 ypfd.write(spec_json)
 
             install_cmd("--add", "--keep-stage", "-f", json_path)
-            buildcache_cmd("push", "-u", "-a", "-f", mirror_url, "callpath")
+            buildcache_cmd("push", "-u", "-f", mirror_url, "callpath")
             ci_cmd("rebuild-index")
 
             buildcache_path = os.path.join(mirror_dir.strpath, "build_cache")

lib/spack/spack/test/cmd/install.py

@@ -966,7 +966,7 @@ def test_compiler_bootstrap_from_binary_mirror(
     install("gcc@=10.2.0")
 
     # Put installed compiler in the buildcache
-    buildcache("push", "-u", "-a", "-f", mirror_dir.strpath, "gcc@10.2.0")
+    buildcache("push", "-u", "-f", mirror_dir.strpath, "gcc@10.2.0")
 
     # Now uninstall the compiler
     uninstall("-y", "gcc@10.2.0")
@@ -1138,7 +1138,7 @@ def install_use_buildcache(opt):
 
     # Populate the buildcache
     install(package_name)
-    buildcache("push", "-u", "-a", "-f", mirror_dir.strpath, package_name, dependency_name)
+    buildcache("push", "-u", "-f", mirror_dir.strpath, package_name, dependency_name)
 
     # Uninstall the all of the packages for clean slate
     uninstall("-y", "-a")

lib/spack/spack/test/cmd/mirror.py

@@ -235,7 +235,7 @@ def test_mirror_destroy(
 
     # Put a binary package in a buildcache
     install("--no-cache", spec_name)
-    buildcache("push", "-u", "-a", "-f", mirror_dir.strpath, spec_name)
+    buildcache("push", "-u", "-f", mirror_dir.strpath, spec_name)
 
     contents = os.listdir(mirror_dir.strpath)
     assert "build_cache" in contents
@@ -245,7 +245,7 @@ def test_mirror_destroy(
 
     assert not os.path.exists(mirror_dir.strpath)
 
-    buildcache("push", "-u", "-a", "-f", mirror_dir.strpath, spec_name)
+    buildcache("push", "-u", "-f", mirror_dir.strpath, spec_name)
 
     contents = os.listdir(mirror_dir.strpath)
     assert "build_cache" in contents

lib/spack/spack/test/data/templates/non_relocatable.c

@@ -1,5 +0,0 @@
-#include <stdio.h>
-
-int main(){
-    printf("Hello World from {{ prefix }} !");
-}

lib/spack/spack/test/data/templates/relocatable.c

@@ -1,5 +0,0 @@
-#include <stdio.h>
-
-int main(){
-    printf("Hello World!");
-}

lib/spack/spack/test/packaging.py

@@ -29,7 +29,6 @@
 from spack.fetch_strategy import FetchStrategyComposite, URLFetchStrategy
 from spack.paths import mock_gpg_keys_path
 from spack.relocate import (
-    ensure_binary_is_relocatable,
     macho_find_paths,
     macho_make_paths_normal,
     macho_make_paths_relative,
@@ -73,7 +72,7 @@ def test_buildcache(mock_archive, tmp_path, monkeypatch, mutable_config):
         parser = argparse.ArgumentParser()
         buildcache.setup_parser(parser)
 
-        create_args = ["create", "-a", "-f", mirror_path, pkghash]
+        create_args = ["create", "-f", "--rebuild-index", mirror_path, pkghash]
         # Create a private key to sign package with if gpg2 available
         spack.util.gpg.create(
             name="test key 1",
@@ -82,8 +81,6 @@ def test_buildcache(mock_archive, tmp_path, monkeypatch, mutable_config):
             comment="Spack test key",
         )
 
-        create_args.insert(create_args.index("-a"), "--rebuild-index")
-
         args = parser.parse_args(create_args)
         buildcache.buildcache(parser, args)
         # trigger overwrite warning
@@ -141,7 +138,6 @@ def test_relocate_text(tmp_path):
     dummy_txt = tmp_path / "dummy.txt"
     dummy_txt.write_text(original_dir)
 
-    ensure_binary_is_relocatable(os.path.realpath(dummy_txt))
     relocate_text([str(dummy_txt)], {original_dir: relocation_dir})
     text = dummy_txt.read_text()
 

lib/spack/spack/test/relocate.py

@@ -10,8 +10,6 @@
 
 import pytest
 
-import llnl.util.filesystem
-
 import spack.concretize
 import spack.paths
 import spack.platforms
@@ -49,30 +47,6 @@ def text_in_bin(text, binary):
         return True
 
 
-@pytest.fixture(params=[True, False])
-def is_relocatable(request):
-    return request.param
-
-
-@pytest.fixture()
-def source_file(tmpdir, is_relocatable):
-    """Returns the path to a source file of a relocatable executable."""
-    if is_relocatable:
-        template_src = os.path.join(spack.paths.test_path, "data", "templates", "relocatable.c")
-        src = tmpdir.join("relocatable.c")
-        shutil.copy(template_src, str(src))
-    else:
-        template_dirs = (os.path.join(spack.paths.test_path, "data", "templates"),)
-        env = spack.tengine.make_environment(template_dirs)
-        template = env.get_template("non_relocatable.c")
-        text = template.render({"prefix": spack.store.layout.root})
-
-        src = tmpdir.join("non_relocatable.c")
-        src.write(text)
-
-    return src
-
-
 @pytest.fixture()
 def mock_patchelf(tmpdir, mock_executable):
     def _factory(output):
@@ -154,42 +128,6 @@ def _copy_somewhere(orig_binary):
     return _copy_somewhere
 
 
-@pytest.mark.requires_executables("/usr/bin/gcc", "patchelf", "strings", "file")
-@skip_unless_linux
-def test_ensure_binary_is_relocatable(source_file, is_relocatable):
-    compiler = spack.util.executable.Executable("/usr/bin/gcc")
-    executable = str(source_file).replace(".c", ".x")
-    compiler_env = {"PATH": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}
-    compiler(str(source_file), "-o", executable, env=compiler_env)
-
-    assert spack.relocate.is_binary(executable)
-
-    try:
-        spack.relocate.ensure_binary_is_relocatable(executable)
-        relocatable = True
-    except spack.relocate.InstallRootStringError:
-        relocatable = False
-
-    assert relocatable == is_relocatable
-
-
-@skip_unless_linux
-def test_ensure_binary_is_relocatable_errors(tmpdir):
-    # The file passed in as argument must exist...
-    with pytest.raises(ValueError) as exc_info:
-        spack.relocate.ensure_binary_is_relocatable("/usr/bin/does_not_exist")
-    assert "does not exist" in str(exc_info.value)
-
-    # ...and the argument must be an absolute path to it
-    file = tmpdir.join("delete.me")
-    file.write("foo")
-
-    with llnl.util.filesystem.working_dir(str(tmpdir)):
-        with pytest.raises(ValueError) as exc_info:
-            spack.relocate.ensure_binary_is_relocatable("delete.me")
-        assert "is not an absolute path" in str(exc_info.value)
-
-
 @pytest.mark.parametrize(
     "start_path,path_root,paths,expected",
     [
@@ -233,7 +171,7 @@ def test_normalize_relative_paths(start_path, relative_paths, expected):
     assert normalized == expected
 
 
-@pytest.mark.requires_executables("patchelf", "strings", "file", "gcc")
+@pytest.mark.requires_executables("patchelf", "file", "gcc")
 @skip_unless_linux
 def test_relocate_text_bin(binary_with_rpaths, prefix_like):
     prefix = "/usr/" + prefix_like
@@ -250,7 +188,7 @@ def test_relocate_text_bin(binary_with_rpaths, prefix_like):
     assert "%s/lib:%s/lib64" % (new_prefix, new_prefix) in rpaths_for(executable)
 
 
-@pytest.mark.requires_executables("patchelf", "strings", "file", "gcc")
+@pytest.mark.requires_executables("patchelf", "file", "gcc")
 @skip_unless_linux
 def test_relocate_elf_binaries_absolute_paths(binary_with_rpaths, copy_binary, prefix_tmpdir):
     # Create an executable, set some RPATHs, copy it to another location
@@ -272,7 +210,7 @@ def test_relocate_elf_binaries_absolute_paths(binary_with_rpaths, copy_binary, p
     assert "/foo/lib:/usr/lib64" in rpaths_for(new_binary)
 
 
-@pytest.mark.requires_executables("patchelf", "strings", "file", "gcc")
+@pytest.mark.requires_executables("patchelf", "file", "gcc")
 @skip_unless_linux
 def test_relocate_elf_binaries_relative_paths(binary_with_rpaths, copy_binary):
     # Create an executable, set some RPATHs, copy it to another location
@@ -293,7 +231,7 @@ def test_relocate_elf_binaries_relative_paths(binary_with_rpaths, copy_binary):
     assert "/foo/lib:/foo/lib64:/opt/local/lib" in rpaths_for(new_binary)
 
 
-@pytest.mark.requires_executables("patchelf", "strings", "file", "gcc")
+@pytest.mark.requires_executables("patchelf", "file", "gcc")
 @skip_unless_linux
 def test_make_elf_binaries_relative(binary_with_rpaths, copy_binary, prefix_tmpdir):
     orig_binary = binary_with_rpaths(
@@ -313,7 +251,7 @@ def test_make_elf_binaries_relative(binary_with_rpaths, copy_binary, prefix_tmpd
     assert "$ORIGIN/lib:$ORIGIN/lib64:/opt/local/lib" in rpaths_for(new_binary)
 
 
-@pytest.mark.requires_executables("patchelf", "strings", "file", "gcc")
+@pytest.mark.requires_executables("patchelf", "file", "gcc")
 @skip_unless_linux
 def test_relocate_text_bin_with_message(binary_with_rpaths, copy_binary, prefix_tmpdir):
     orig_binary = binary_with_rpaths(

lib/spack/spack/test/rewiring.py

@@ -14,7 +14,7 @@
 from spack.spec import Spec
 from spack.test.relocate import text_in_bin
 
-args = ["strings", "file"]
+args = ["file"]
 if sys.platform == "darwin":
     args.extend(["/usr/bin/clang++", "install_name_tool"])
 else:

var/spack/repos/builtin/packages/spack/package.py

@@ -71,8 +71,8 @@ class Spack(Package):
     depends_on("lmod@7.5.12:", type="run", when="@0.18:")
 
     # Buildcache
-    # We really just need the 'strings' from binutils
+    # We really just need the 'strings' from binutils for older versions of spack
-    depends_on("binutils", type="run")
+    depends_on("binutils", type="run", when="@:0.20")
     depends_on("gnupg", type="run")
     depends_on("patchelf", type="run", when="platform=linux")
     depends_on("patchelf", type="run", when="platform=cray")