From f00e4112872a6df244e9a437e1773c9734112c08 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Thu, 17 Nov 2022 13:21:27 +0100 Subject: relocate.py: small refactor for file_is_relocatable (#33967) --- lib/spack/spack/binary_distribution.py | 4 ++-- lib/spack/spack/relocate.py | 37 +++++++++++++--------------------- lib/spack/spack/test/packaging.py | 4 ++-- lib/spack/spack/test/relocate.py | 25 ++++++++++++----------- 4 files changed, 31 insertions(+), 39 deletions(-) diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index 4fbf6e5ec3..b5ec57687d 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/lib/spack/spack/binary_distribution.py @@ -1634,7 +1634,7 @@ def make_package_relative(workdir, spec, allow_root): if "elf" in platform.binary_formats: relocate.make_elf_binaries_relative(cur_path_names, orig_path_names, old_layout_root) - relocate.raise_if_not_relocatable(cur_path_names, allow_root) + allow_root or relocate.ensure_binaries_are_relocatable(cur_path_names) orig_path_names = list() cur_path_names = list() for linkname in buildinfo.get("relocate_links", []): @@ -1652,7 +1652,7 @@ def check_package_relocatable(workdir, spec, allow_root): cur_path_names = list() for filename in buildinfo["relocate_binaries"]: cur_path_names.append(os.path.join(workdir, filename)) - relocate.raise_if_not_relocatable(cur_path_names, allow_root) + allow_root or relocate.ensure_binaries_are_relocatable(cur_path_names) def dedupe_hardlinks_if_necessary(root, buildinfo): diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py index 7ac19574ce..97812bdb5c 100644 --- a/lib/spack/spack/relocate.py +++ b/lib/spack/spack/relocate.py @@ -19,9 +19,11 @@ from llnl.util.lang import memoized from llnl.util.symlink import symlink import spack.bootstrap +import spack.paths import spack.platforms import spack.repo import spack.spec +import spack.store import spack.util.elf as elf import spack.util.executable as executable @@ -772,19 +774,17 @@ def make_elf_binaries_relative(new_binaries, orig_binaries, orig_layout_root): _set_elf_rpaths(new_binary, new_rpaths) -def raise_if_not_relocatable(binaries, allow_root): +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 - allow_root (bool): whether root dir is allowed or not in a binary Raises: InstallRootStringError: if the file is not relocatable """ for binary in binaries: - if not (allow_root or file_is_relocatable(binary)): - raise InstallRootStringError(binary, spack.store.layout.root) + ensure_binary_is_relocatable(binary) def warn_if_link_cant_be_relocated(link, target): @@ -933,30 +933,27 @@ def is_relocatable(spec): # 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")] - abs_files = [os.path.join(root, f) for f in files] - if not all(file_is_relocatable(f) for f in abs_files if is_binary(f)): - # If any of the file is not relocatable, the entire - # package is not relocatable + 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 file_is_relocatable(filename, paths_to_relocate=None): - """Returns True if the filename passed as argument is relocatable. +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 - Returns: - True or false - Raises: - + InstallRootStringError: if the binary contains an absolute path ValueError: if the filename does not exist or the path is not absolute """ - default_paths_to_relocate = [spack.store.layout.root, spack.paths.prefix] - paths_to_relocate = paths_to_relocate or default_paths_to_relocate + 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)) @@ -987,13 +984,7 @@ def file_is_relocatable(filename, paths_to_relocate=None): for path_to_relocate in paths_to_relocate: if any(path_to_relocate in x for x in set_of_strings): - # One binary has the root folder not in the RPATH, - # meaning that this spec is not relocatable - msg = 'Found "{0}" in {1} strings' - tty.debug(msg.format(path_to_relocate, filename), level=2) - return False - - return True + raise InstallRootStringError(filename, path_to_relocate) def is_binary(filename): diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py index f889cd5384..ac92e85dad 100644 --- a/lib/spack/spack/test/packaging.py +++ b/lib/spack/spack/test/packaging.py @@ -28,7 +28,7 @@ import spack.util.gpg from spack.fetch_strategy import FetchStrategyComposite, URLFetchStrategy from spack.paths import mock_gpg_keys_path from spack.relocate import ( - file_is_relocatable, + ensure_binary_is_relocatable, macho_find_paths, macho_make_paths_normal, macho_make_paths_relative, @@ -206,7 +206,7 @@ def test_unsafe_relocate_text(tmpdir): with open(filename, "r") as script: for line in script: assert new_dir in line - assert file_is_relocatable(os.path.realpath(filename)) + ensure_binary_is_relocatable(os.path.realpath(filename)) # Remove cached binary specs since we deleted the mirror bindist._cached_specs = set() diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py index b07f8402df..f3471efdc0 100644 --- a/lib/spack/spack/test/relocate.py +++ b/lib/spack/spack/test/relocate.py @@ -158,14 +158,21 @@ def copy_binary(prefix_like): @pytest.mark.requires_executables("/usr/bin/gcc", "patchelf", "strings", "file") @skip_unless_linux -def test_file_is_relocatable(source_file, is_relocatable): +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) - assert spack.relocate.file_is_relocatable(executable) is is_relocatable + + try: + spack.relocate.ensure_binary_is_relocatable(executable) + relocatable = True + except spack.relocate.InstallRootStringError: + relocatable = False + + assert relocatable == is_relocatable @pytest.mark.requires_executables("patchelf", "strings", "file") @@ -173,14 +180,14 @@ def test_file_is_relocatable(source_file, is_relocatable): def test_patchelf_is_relocatable(): patchelf = os.path.realpath(spack.relocate._patchelf()) assert llnl.util.filesystem.is_exe(patchelf) - assert spack.relocate.file_is_relocatable(patchelf) + spack.relocate.ensure_binary_is_relocatable(patchelf) @skip_unless_linux -def test_file_is_relocatable_errors(tmpdir): +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.file_is_relocatable("/usr/bin/does_not_exist") + 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 @@ -189,7 +196,7 @@ def test_file_is_relocatable_errors(tmpdir): with llnl.util.filesystem.working_dir(str(tmpdir)): with pytest.raises(ValueError) as exc_info: - spack.relocate.file_is_relocatable("delete.me") + spack.relocate.ensure_binary_is_relocatable("delete.me") assert "is not an absolute path" in str(exc_info.value) @@ -340,12 +347,6 @@ 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) -def test_raise_if_not_relocatable(monkeypatch): - monkeypatch.setattr(spack.relocate, "file_is_relocatable", lambda x: False) - with pytest.raises(spack.relocate.InstallRootStringError): - spack.relocate.raise_if_not_relocatable(["an_executable"], allow_root=False) - - @pytest.mark.requires_executables("patchelf", "strings", "file", "gcc") @skip_unless_linux def test_relocate_text_bin(binary_with_rpaths, copy_binary, prefix_tmpdir): -- cgit v1.2.3-70-g09d2