From 1b4de7813a64f81c65256b98894aba7af86976d7 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 24 Mar 2020 09:02:54 +0100 Subject: [PATCH] spack.relocate: add unit test (#15610) * relocate: removed import from statements * relocate: renamed *Exception to *Error This aims at consistency in naming with both the standard library (ValueError, AttributeError, etc.) and other errors in 'spack.error'. Improved existing docstrings * relocate: simplified search function by un-nesting conditionals The search function that searches for patchelf has been refactored to remove deeply nested conditionals. Extended docstring. * relocate: removed a condition specific to unit tests * relocate: added test for _patchelf --- lib/spack/spack/relocate.py | 138 +++++++++++++++++-------------- lib/spack/spack/test/relocate.py | 59 ++++++++++++- 2 files changed, 130 insertions(+), 67 deletions(-) diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py index c7d442a427..9f8669f3d4 100644 --- a/lib/spack/spack/relocate.py +++ b/lib/spack/spack/relocate.py @@ -2,55 +2,63 @@ # Spack Project Developers. See the top-level COPYRIGHT file for details. # # SPDX-License-Identifier: (Apache-2.0 OR MIT) - - import os +import platform import re import shutil -import platform -import spack.repo -import spack.cmd + import llnl.util.lang -from spack.util.executable import Executable, ProcessError import llnl.util.tty as tty -from macholib.MachO import MachO -from spack.spec import Spec +import macholib.MachO import macholib.mach_o +import spack.cmd +import spack.repo +import spack.spec +import spack.util.executable as executable -class InstallRootStringException(spack.error.SpackError): - """ - Raised when the relocated binary still has the install root string. - """ - +class InstallRootStringError(spack.error.SpackError): def __init__(self, file_path, root_path): - super(InstallRootStringException, self).__init__( + """Signal that the relocated binary still has the original + Spack's store root string + + Args: + file_path (str): path of the binary + root_path (str): original Spack's store root string + """ + super(InstallRootStringError, self).__init__( "\n %s \ncontains string\n %s \n" "after replacing it in rpaths.\n" "Package should not be relocated.\n Use -a to override." % (file_path, root_path)) -class BinaryStringReplacementException(spack.error.SpackError): - """ - Raised when the size of the file changes after binary path substitution. - """ - +class BinaryStringReplacementError(spack.error.SpackError): def __init__(self, file_path, old_len, new_len): - super(BinaryStringReplacementException, self).__init__( + """The size of the file changed after binary path substitution + + Args: + file_path (str): file with changing size + old_len (str): original length of the file + new_len (str): length of the file after substitution + """ + super(BinaryStringReplacementError, self).__init__( "Doing a binary string replacement in %s failed.\n" "The size of the file changed from %s to %s\n" "when it should have remanined the same." % (file_path, old_len, new_len)) -class BinaryTextReplaceException(spack.error.SpackError): - """ - Raised when the new install path is shorter than the old install path - so binary text replacement cannot occur. - """ - +class BinaryTextReplaceError(spack.error.SpackError): def __init__(self, old_path, new_path): + """Raised when the new install path is longer than the + old one, so binary text replacement cannot occur. + + Args: + old_path (str): original path to be substituted + new_path (str): candidate path for substitution + """ + msg = "New path longer than old path: binary text" msg += " replacement not possible." err_msg = "The new path %s" % new_path @@ -58,33 +66,35 @@ def __init__(self, old_path, new_path): err_msg += "Text replacement in binaries will not work.\n" err_msg += "Create buildcache from an install path " err_msg += "longer than new path." - super(BinaryTextReplaceException, self).__init__(msg, err_msg) + super(BinaryTextReplaceError, self).__init__(msg, err_msg) -def get_patchelf(): +def _patchelf(): + """Return the full path to the patchelf binary, if available, else None. + + Search first the current PATH for patchelf. If not found, try to look + if the default patchelf spec is installed and if not install it. + + Return None on Darwin or if patchelf cannot be found. """ - Returns the full patchelf binary path if available in $PATH. - Builds and installs spack patchelf package on linux platforms - using the first concretized spec if it is not installed and - returns the full patchelf binary path. - """ - # as we may need patchelf, find out where it is + # Check if patchelf is already in the PATH patchelf = spack.util.executable.which('patchelf') if patchelf is not None: return patchelf.path - patchelf_spec = Spec('patchelf').concretized() - patchelf = patchelf_spec.package - if patchelf.installed: - patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf") - return patchelf_executable - else: - if (str(spack.architecture.platform()) == 'test' or - str(spack.architecture.platform()) == 'darwin'): - return None - else: - patchelf.do_install() - patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf") - return patchelf_executable + + # Check if patchelf spec is installed + spec = spack.spec.Spec('patchelf').concretized() + exe_path = os.path.join(spec.prefix.bin, "patchelf") + if spec.package.installed and os.path.exists(exe_path): + return exe_path + + # Skip darwin + if str(spack.architecture.platform()) == 'darwin': + return None + + # Install the spec and return its path + spec.package.do_install() + return exe_path if os.path.exists(exe_path) else None def get_existing_elf_rpaths(path_name): @@ -95,17 +105,17 @@ def get_existing_elf_rpaths(path_name): # if we're relocating patchelf itself, use it - if path_name[-13:] == "/bin/patchelf": - patchelf = Executable(path_name) + if path_name.endswith("/bin/patchelf"): + patchelf = executable.Executable(path_name) else: - patchelf = Executable(get_patchelf()) + patchelf = executable.Executable(_patchelf()) rpaths = list() try: output = patchelf('--print-rpath', '%s' % path_name, output=str, error=str) rpaths = output.rstrip('\n').split(':') - except ProcessError as e: + except executable.ProcessError as e: msg = 'patchelf --print-rpath %s produced an error %s' % (path_name, e) tty.warn(msg) return rpaths @@ -266,7 +276,7 @@ def modify_macho_object(cur_path, rpaths, deps, idpath, # avoid error message for libgcc_s if 'libgcc_' in cur_path: return - install_name_tool = Executable('install_name_tool') + install_name_tool = executable.Executable('install_name_tool') if idpath: new_idpath = paths_to_paths.get(idpath, None) @@ -295,7 +305,7 @@ def modify_object_macholib(cur_path, paths_to_paths): dictionary mapping paths in old install layout to new install layout """ - dll = MachO(cur_path) + dll = macholib.MachO.MachO(cur_path) changedict = paths_to_paths @@ -324,7 +334,7 @@ def macholib_get_paths(cur_path): Get rpaths, dependencies and id of mach-o objects using python macholib package """ - dll = MachO(cur_path) + dll = macholib.MachO.MachO(cur_path) ident = None rpaths = list() @@ -359,14 +369,14 @@ def modify_elf_object(path_name, new_rpaths): if path_name[-13:] == "/bin/patchelf": shutil.copy(path_name, bak_path) - patchelf = Executable(bak_path) + patchelf = executable.Executable(bak_path) else: - patchelf = Executable(get_patchelf()) + patchelf = executable.Executable(_patchelf()) try: patchelf('--force-rpath', '--set-rpath', '%s' % new_joined, '%s' % path_name, output=str, error=str) - except ProcessError as e: + except executable.ProcessError as e: msg = 'patchelf --force-rpath --set-rpath %s failed with error %s' % ( path_name, e) tty.warn(msg) @@ -443,7 +453,7 @@ def replace(match): return ndata = pat.sub(replace, data) if not len(ndata) == original_data_len: - raise BinaryStringReplacementException( + raise BinaryStringReplacementError( path_name, original_data_len, len(ndata)) f.write(ndata) f.truncate() @@ -468,7 +478,7 @@ def replace(match): new_dir.encode('utf-8')) + b'\0' * padding if len(new_dir) > len(old_dir): - raise BinaryTextReplaceException(old_dir, new_dir) + raise BinaryTextReplaceError(old_dir, new_dir) with open(path_name, 'rb+') as f: data = f.read() @@ -479,7 +489,7 @@ def replace(match): return ndata = pat.sub(replace, data) if not len(ndata) == original_data_len: - raise BinaryStringReplacementException( + raise BinaryStringReplacementError( path_name, original_data_len, len(ndata)) f.write(ndata) f.truncate() @@ -656,7 +666,7 @@ def check_files_relocatable(cur_path_names, allow_root): for cur_path in cur_path_names: if (not allow_root and not file_is_relocatable(cur_path)): - raise InstallRootStringException( + raise InstallRootStringError( cur_path, spack.store.layout.root) @@ -725,7 +735,7 @@ def relocate_text_bin(path_names, old_layout_root, new_layout_root, replace_prefix_bin(path_name, old_spack_prefix, new_spack_prefix) else: if len(path_names) > 0: - raise BinaryTextReplaceException( + raise BinaryTextReplaceError( old_install_prefix, new_install_prefix) @@ -789,7 +799,7 @@ def file_is_relocatable(file, paths_to_relocate=None): if not os.path.isabs(file): raise ValueError('{0} is not an absolute path'.format(file)) - strings = Executable('strings') + strings = executable.Executable('strings') # Remove the RPATHS from the strings in the executable set_of_strings = set(strings(file, output=str).split()) @@ -851,7 +861,7 @@ def mime_type(file): Returns: Tuple containing the MIME type and subtype """ - file_cmd = Executable('file') + file_cmd = executable.Executable('file') output = file_cmd('-b', '-h', '--mime-type', file, output=str, error=str) tty.debug('[MIME_TYPE] {0} -> {1}'.format(file, output.strip())) # In corner cases the output does not contain a subtype prefixed with a / diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py index 113bdcf66a..0a9e9f7f0a 100644 --- a/lib/spack/spack/test/relocate.py +++ b/lib/spack/spack/test/relocate.py @@ -3,15 +3,18 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import collections import os.path import platform import shutil -import pytest - import llnl.util.filesystem +import pytest +import spack.architecture +import spack.concretize import spack.paths import spack.relocate +import spack.spec import spack.store import spack.tengine import spack.util.executable @@ -45,6 +48,47 @@ def source_file(tmpdir, is_relocatable): return src +@pytest.fixture(params=['which_found', 'installed', 'to_be_installed']) +def expected_patchelf_path(request, mutable_database, monkeypatch): + """Prepare the stage to tests different cases that can occur + when searching for patchelf. + """ + case = request.param + + # Mock the which function + which_fn = { + 'which_found': lambda x: collections.namedtuple( + '_', ['path'] + )('/usr/bin/patchelf') + } + monkeypatch.setattr( + spack.util.executable, 'which', + which_fn.setdefault(case, lambda x: None) + ) + if case == 'which_found': + return '/usr/bin/patchelf' + + # TODO: Mock a case for Darwin architecture + + spec = spack.spec.Spec('patchelf') + spec.concretize() + + patchelf_cls = type(spec.package) + do_install = patchelf_cls.do_install + expected_path = os.path.join(spec.prefix.bin, 'patchelf') + + def do_install_mock(self, **kwargs): + do_install(self, fake=True) + with open(expected_path): + pass + + monkeypatch.setattr(patchelf_cls, 'do_install', do_install_mock) + if case == 'installed': + spec.package.do_install() + + return expected_path + + @pytest.mark.requires_executables( '/usr/bin/gcc', 'patchelf', 'strings', 'file' ) @@ -64,7 +108,7 @@ def test_file_is_relocatable(source_file, is_relocatable): 'patchelf', 'strings', 'file' ) def test_patchelf_is_relocatable(): - patchelf = spack.relocate.get_patchelf() + patchelf = spack.relocate._patchelf() assert llnl.util.filesystem.is_exe(patchelf) assert spack.relocate.file_is_relocatable(patchelf) @@ -87,3 +131,12 @@ def test_file_is_relocatable_errors(tmpdir): with pytest.raises(ValueError) as exc_info: spack.relocate.file_is_relocatable('delete.me') assert 'is not an absolute path' in str(exc_info.value) + + +@pytest.mark.skipif( + platform.system().lower() != 'linux', + reason='implementation for MacOS still missing' +) +def test_search_patchelf(expected_patchelf_path): + current = spack.relocate._patchelf() + assert current == expected_patchelf_path