From a96f2f603b48708b5063a9b81aa525f8d9ccd8c2 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 26 Nov 2021 15:32:13 +0100 Subject: 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 | 28 ++++++++++++++++++ lib/spack/spack/bootstrap.py | 62 +++++++++++++++++++++++++++++++-------- lib/spack/spack/relocate.py | 31 +++++--------------- lib/spack/spack/test/bootstrap.py | 10 +++++++ lib/spack/spack/test/relocate.py | 50 ++----------------------------- 5 files changed, 98 insertions(+), 83 deletions(-) diff --git a/.github/workflows/bootstrap.yml b/.github/workflows/bootstrap.yml index ebfa93db19..ec7d9a3898 100644 --- a/.github/workflows/bootstrap.yml +++ b/.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" diff --git a/lib/spack/spack/bootstrap.py b/lib/spack/spack/bootstrap.py index f046a34a19..92ce0ed934 100644 --- a/lib/spack/spack/bootstrap.py +++ b/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.binary_distribution 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.user_environment 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 @@ class _BuildcacheBootstrapper(object): return data def _install_by_hash(self, pkg_hash, pkg_sha256, index, bincache_platform): - global _buildcache_cmd - - if _buildcache_cmd is None: - _buildcache_cmd = spack.main.SpackCommand('buildcache') + # 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 + # TODO: binary relocation + 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 @@ class _BuildcacheBootstrapper(object): '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 @@ class _SourceBootstrapper(object): abstract_spec_str += ' os=fe' concrete_spec = spack.spec.Spec(abstract_spec_str) - concrete_spec.concretize() + 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 ### diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py index 08ec1a18ee..9cf01d7c9c 100644 --- a/lib/spack/spack/relocate.py +++ b/lib/spack/spack/relocate.py @@ -14,6 +14,7 @@ import macholib.MachO 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 @@ class BinaryTextReplaceError(spack.error.SpackError): 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. - """ - # 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 + """Return the full path to the patchelf binary, if available, else None.""" if is_macos: return None - # Install the spec and return its path - spec.package.do_install() - return exe_path if os.path.exists(exe_path) else None + patchelf = executable.which('patchelf') + if patchelf is None: + with spack.bootstrap.ensure_bootstrap_configuration(): + patchelf = spack.bootstrap.ensure_patchelf_in_path_or_raise() + + return patchelf.path def _elf_rpaths_for(path): diff --git a/lib/spack/spack/test/bootstrap.py b/lib/spack/spack/test/bootstrap.py index 99c1a61fd3..fdfd1f9610 100644 --- a/lib/spack/spack/test/bootstrap.py +++ b/lib/spack/spack/test/bootstrap.py @@ -140,3 +140,13 @@ spack: 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 diff --git a/lib/spack/spack/test/relocate.py b/lib/spack/spack/test/relocate.py index 912e1004b2..61a605c976 100644 --- a/lib/spack/spack/test/relocate.py +++ b/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_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']), -- cgit v1.2.3-60-g2f50