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

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 @@ class BinaryTextReplaceException(spack.error.SpackError):
         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():
-    """
-    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.
+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.
     """
-    # 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_prefix_bin(path_name, old_dir, new_dir):
             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_prefix_nullterm(path_name, old_dir, new_dir):
                                      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_prefix_nullterm(path_name, old_dir, new_dir):
             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