Bootstrap patchelf like GnuPG (#27532)

Remove a custom bootstrapping procedure to
use spack.bootstrap instead

Modifications:
* Reference count the bootstrap context manager
* Avoid SpackCommand to make the bootstrapping
  procedure more transparent
* Put back requirement on patchelf being in PATH for unit tests
* Add an e2e test to check bootstrapping patchelf

.github/workflows/bootstrap.yml

@@ -78,6 +78,34 @@ jobs:
           spack -d solve zlib
           tree ~/.spack/bootstrap/store/
 
+  ubuntu-clingo-binaries-and-patchelf:
+    runs-on: ubuntu-latest
+    container: "ubuntu:latest"
+    steps:
+      - name: Install dependencies
+        env:
+          DEBIAN_FRONTEND: noninteractive
+        run: |
+          apt-get update -y && apt-get upgrade -y
+          apt-get install -y \
+              bzip2 curl file g++ gcc gfortran git gnupg2 gzip \
+              make patch unzip xz-utils python3 python3-dev tree
+      - uses: actions/checkout@ec3a7ce113134d7a93b817d10a8272cb61118579 # @v2
+      - name: Setup repo and non-root user
+        run: |
+          git --version
+          git fetch --unshallow
+          . .github/workflows/setup_git.sh
+          useradd -m spack-test
+          chown -R spack-test .
+      - name: Bootstrap clingo
+        shell: runuser -u spack-test -- bash {0}
+        run: |
+          source share/spack/setup-env.sh
+          spack -d solve zlib
+          tree ~/.spack/bootstrap/store/
+
+
   opensuse-clingo-sources:
     runs-on: ubuntu-latest
     container: "opensuse/leap:latest"

lib/spack/spack/bootstrap.py

@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 from __future__ import print_function
 
+import argparse
 import contextlib
 import fnmatch
 import functools
@@ -25,7 +26,6 @@
 import spack.config
 import spack.detection
 import spack.environment
-import spack.main
 import spack.modules
 import spack.paths
 import spack.platforms
@@ -36,9 +36,6 @@
 import spack.util.executable
 import spack.util.path
 
-#: "spack buildcache" command, initialized lazily
-_buildcache_cmd = None
-
 #: Map a bootstrapper type to the corresponding class
 _bootstrap_methods = {}
 
@@ -258,10 +255,10 @@ def _read_metadata(self, package_name):
         return data
 
     def _install_by_hash(self, pkg_hash, pkg_sha256, index, bincache_platform):
-        global _buildcache_cmd
+        # TODO: The local import is due to a circular import error. The
-
+        # TODO: correct fix for this is a refactor of the API used for
-        if _buildcache_cmd is None:
+        # TODO: binary relocation
-            _buildcache_cmd = spack.main.SpackCommand('buildcache')
+        import spack.cmd.buildcache
 
         index_spec = next(x for x in index if x.dag_hash() == pkg_hash)
         # Reconstruct the compiler that we need to use for bootstrapping
@@ -282,13 +279,16 @@ def _install_by_hash(self, pkg_hash, pkg_sha256, index, bincache_platform):
                     'compilers', [{'compiler': compiler_entry}]
             ):
                 spec_str = '/' + pkg_hash
+                parser = argparse.ArgumentParser()
+                spack.cmd.buildcache.setup_parser(parser)
                 install_args = [
                     'install',
                     '--sha256', pkg_sha256,
                     '--only-root',
                     '-a', '-u', '-o', '-f', spec_str
                 ]
-                _buildcache_cmd(*install_args, fail_on_error=False)
+                args = parser.parse_args(install_args)
+                spack.cmd.buildcache.installtarball(args)
 
     def _install_and_test(
             self, abstract_spec, bincache_platform, bincache_data, test_fn
@@ -421,9 +421,12 @@ def try_search_path(self, executables, abstract_spec_str):
             abstract_spec_str += ' os=fe'
 
         concrete_spec = spack.spec.Spec(abstract_spec_str)
+        if concrete_spec.name == 'patchelf':
+            concrete_spec._old_concretize(deprecation_warning=False)
+        else:
             concrete_spec.concretize()
 
-        msg = "[BOOTSTRAP GnuPG] Try installing '{0}' from sources"
+        msg = "[BOOTSTRAP] Try installing '{0}' from sources"
         tty.debug(msg.format(abstract_spec_str))
         concrete_spec.package.do_install()
         if _executables_in_store(executables, concrete_spec, query_info=info):
@@ -644,8 +647,30 @@ def _add_externals_if_missing():
     spack.detection.update_configuration(detected_packages, scope='bootstrap')
 
 
+#: Reference counter for the bootstrapping configuration context manager
+_REF_COUNT = 0
+
+
 @contextlib.contextmanager
 def ensure_bootstrap_configuration():
+    # The context manager is reference counted to ensure we don't swap multiple
+    # times if there's nested use of it in the stack. One compelling use case
+    # is bootstrapping patchelf during the bootstrap of clingo.
+    global _REF_COUNT
+    already_swapped = bool(_REF_COUNT)
+    _REF_COUNT += 1
+    try:
+        if already_swapped:
+            yield
+        else:
+            with _ensure_bootstrap_configuration():
+                yield
+    finally:
+        _REF_COUNT -= 1
+
+
+@contextlib.contextmanager
+def _ensure_bootstrap_configuration():
     bootstrap_store_path = store_path()
     user_configuration = _read_and_sanitize_configuration()
     with spack.environment.no_active_environment():
@@ -753,10 +778,23 @@ def gnupg_root_spec():
 
 def ensure_gpg_in_path_or_raise():
     """Ensure gpg or gpg2 are in the PATH or raise."""
-    ensure_executables_in_path_or_raise(
+    return ensure_executables_in_path_or_raise(
         executables=['gpg2', 'gpg'], abstract_spec=gnupg_root_spec()
     )
 
+
+def patchelf_root_spec():
+    """Return the root spec used to bootstrap patchelf"""
+    return _root_spec('patchelf@0.13:')
+
+
+def ensure_patchelf_in_path_or_raise():
+    """Ensure patchelf is in the PATH or raise."""
+    return ensure_executables_in_path_or_raise(
+        executables=['patchelf'], abstract_spec=patchelf_root_spec()
+    )
+
+
 ###
 # Development dependencies
 ###

lib/spack/spack/relocate.py

@@ -14,6 +14,7 @@
 import llnl.util.lang
 import llnl.util.tty as tty
 
+import spack.bootstrap
 import spack.platforms
 import spack.repo
 import spack.spec
@@ -75,32 +76,16 @@ def __init__(self, old_path, new_path):
 
 
 def _patchelf():
-    """Return the full path to the patchelf binary, if available, else None.
+    """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.
-    """
-    # Check if patchelf is already in the PATH
-    patchelf = executable.which('patchelf')
-    if patchelf is not None:
-        return patchelf.path
-
-    # Check if patchelf spec is installed
-    spec = spack.spec.Spec('patchelf')
-    spec._old_concretize(deprecation_warning=False)
-    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 is_macos:
         return None
 
-    # Install the spec and return its path
+    patchelf = executable.which('patchelf')
-    spec.package.do_install()
+    if patchelf is None:
-    return exe_path if os.path.exists(exe_path) else None
+        with spack.bootstrap.ensure_bootstrap_configuration():
+            patchelf = spack.bootstrap.ensure_patchelf_in_path_or_raise()
+
+    return patchelf.path
 
 
 def _elf_rpaths_for(path):

lib/spack/spack/test/bootstrap.py

@@ -140,3 +140,13 @@ def test_custom_store_in_environment(mutable_config, tmpdir):
         with spack.bootstrap.ensure_bootstrap_configuration():
             pass
         assert str(spack.store.root) == '/tmp/store'
+
+
+def test_nested_use_of_context_manager(mutable_config):
+    """Test nested use of the context manager"""
+    user_config = spack.config.config
+    with spack.bootstrap.ensure_bootstrap_configuration():
+        assert spack.config.config != user_config
+        with spack.bootstrap.ensure_bootstrap_configuration():
+            assert spack.config.config != user_config
+    assert spack.config.config == user_config

lib/spack/spack/test/relocate.py

@@ -2,7 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-import collections
 import os.path
 import re
 import shutil
@@ -73,47 +72,6 @@ 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.fixture()
 def mock_patchelf(tmpdir, mock_executable):
     def _factory(output):
@@ -227,6 +185,7 @@ 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):
     compiler = spack.util.executable.Executable('/usr/bin/gcc')
     executable = str(source_file).replace('.c', '.x')
@@ -240,6 +199,7 @@ def test_file_is_relocatable(source_file, is_relocatable):
 
 
 @pytest.mark.requires_executables('patchelf', 'strings', 'file')
+@skip_unless_linux
 def test_patchelf_is_relocatable():
     patchelf = os.path.realpath(spack.relocate._patchelf())
     assert llnl.util.filesystem.is_exe(patchelf)
@@ -263,12 +223,6 @@ def test_file_is_relocatable_errors(tmpdir):
         assert 'is not an absolute path' in str(exc_info.value)
 
 
-@skip_unless_linux
-def test_search_patchelf(expected_patchelf_path):
-    current = spack.relocate._patchelf()
-    assert current == expected_patchelf_path
-
-
 @pytest.mark.parametrize('patchelf_behavior,expected', [
     ('echo ', []),
     ('echo /opt/foo/lib:/opt/foo/lib64', ['/opt/foo/lib', '/opt/foo/lib64']),