relocate.py: small refactor for file_is_relocatable (#33967)

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):

lib/spack/spack/relocate.py

@@ -19,9 +19,11 @@
 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):

lib/spack/spack/test/packaging.py

@@ -28,7 +28,7 @@
 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()
 

lib/spack/spack/test/relocate.py

@@ -158,14 +158,21 @@ def _copy_somewhere(orig_binary):
 
 @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):