From 5b23c5dcc070e11828e4e1ff773c4a68e2d7386c Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Tue, 18 Jul 2023 18:45:14 +0200 Subject: 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 | 12 --- lib/spack/spack/ci.py | 4 +- lib/spack/spack/cmd/buildcache.py | 18 ++--- lib/spack/spack/relocate.py | 90 ---------------------- lib/spack/spack/test/bindist.py | 14 ++-- lib/spack/spack/test/cmd/buildcache.py | 14 ++-- lib/spack/spack/test/cmd/ci.py | 4 +- lib/spack/spack/test/cmd/install.py | 4 +- lib/spack/spack/test/cmd/mirror.py | 4 +- .../spack/test/data/templates/non_relocatable.c | 5 -- lib/spack/spack/test/data/templates/relocatable.c | 5 -- lib/spack/spack/test/packaging.py | 6 +- lib/spack/spack/test/relocate.py | 72 ++--------------- lib/spack/spack/test/rewiring.py | 2 +- 14 files changed, 36 insertions(+), 218 deletions(-) delete mode 100644 lib/spack/spack/test/data/templates/non_relocatable.c delete mode 100644 lib/spack/spack/test/data/templates/relocatable.c (limited to 'lib') diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index 693a864a6b..407ee4d029 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/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 diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py index fd0f5fe20f..3498e8d805 100644 --- a/lib/spack/spack/ci.py +++ b/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( - input_spec, push_url, bindist.PushOptions(force=True, allow_root=True, unsigned=unsigned) - ) + return bindist.push(input_spec, push_url, bindist.PushOptions(force=True, unsigned=unsigned)) def push_mirror_contents(input_spec: spack.spec.Spec, mirror_url, sign_binaries): diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py index 0a59ee9eaf..416fb88a80 100644 --- a/lib/spack/spack/cmd/buildcache.py +++ b/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) - specs = spack.store.find(constraints, multiple=True) - - # Cycle over the specs that match - for spec in specs: - print("Relocatable nodes") - print("--------------------------------") - print(spec.tree(status_fn=spack.relocate.is_relocatable)) + tty.warn( + "`spack buildcache preview` is deprecated since `spack buildcache push --allow-root` is " + "now the default. This command will be removed in Spack 0.22" + ) def check_fn(args): diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py index 30cdb4bd06..6c29ea69b3 100644 --- a/lib/spack/spack/relocate.py +++ b/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 diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py index d791e19bd2..cf2eb932ce 100644 --- a/lib/spack/spack/test/bindist.py +++ b/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) diff --git a/lib/spack/spack/test/cmd/buildcache.py b/lib/spack/spack/test/cmd/buildcache.py index 85765c96ac..8cd8db7431 100644 --- a/lib/spack/spack/test/cmd/buildcache.py +++ b/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( - platform.system().lower() != "linux", reason="implementation for MacOS still missing" -) -@pytest.mark.db -def test_buildcache_preview_just_runs(database): +def test_buildcache_preview_just_runs(): + # TODO: remove in Spack 0.21 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 test_buildcache_sync( # 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 diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py index 818bf1850e..1ef493e422 100644 --- a/lib/spack/spack/test/cmd/ci.py +++ b/lib/spack/spack/test/cmd/ci.py @@ -895,7 +895,7 @@ spack: ) 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 @@ spack: 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") diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py index e9503d80d8..5c162a30e2 100644 --- a/lib/spack/spack/test/cmd/install.py +++ b/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 test_install_use_buildcache( # 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") diff --git a/lib/spack/spack/test/cmd/mirror.py b/lib/spack/spack/test/cmd/mirror.py index 1ae38d5d98..758044f61f 100644 --- a/lib/spack/spack/test/cmd/mirror.py +++ b/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 diff --git a/lib/spack/spack/test/data/templates/non_relocatable.c b/lib/spack/spack/test/data/templates/non_relocatable.c deleted file mode 100644 index c2e3724c2c..0000000000 --- a/lib/spack/spack/test/data/templates/non_relocatable.c +++ /dev/null @@ -1,5 +0,0 @@ -#include - -int main(){ - printf("Hello World from {{ prefix }} !"); -} diff --git a/lib/spack/spack/test/data/templates/relocatable.c b/lib/spack/spack/test/data/templates/relocatable.c deleted file mode 100644 index f3e4959692..0000000000 --- a/lib/spack/spack/test/data/templates/relocatable.c +++ /dev/null @@ -1,5 +0,0 @@ -#include - -int main(){ - printf("Hello World!"); -} diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py index b9bc51ac42..c47a0d6c46 100644 --- a/lib/spack/spack/test/packaging.py +++ b/lib/spack/spack/test/packaging.py @@ -29,7 +29,6 @@ import spack.util.url as url_util 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() diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py index a0553d4422..9d600aaa69 100644 --- a/lib/spack/spack/test/relocate.py +++ b/lib/spack/spack/test/relocate.py @@ -10,8 +10,6 @@ import sys 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_binary(prefix_like): 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( diff --git a/lib/spack/spack/test/rewiring.py b/lib/spack/spack/test/rewiring.py index 3aed08a720..033318f15e 100644 --- a/lib/spack/spack/test/rewiring.py +++ b/lib/spack/spack/test/rewiring.py @@ -14,7 +14,7 @@ import spack.store 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: -- cgit v1.2.3-60-g2f50