From e3cd91af5376bd1d134b363be2f0ff962b103a92 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Thu, 18 Nov 2021 15:23:09 +0100 Subject: [PATCH] Refactor bootstrap of "spack style" dependencies (#27485) Remove the "get_executable" function from the spack.bootstrap module. Now "flake8", "isort", "mypy" and "black" will use the same bootstrapping method as GnuPG. --- lib/spack/spack/analyzers/libabigail.py | 12 +- lib/spack/spack/bootstrap.py | 234 +++++++++++++----------- lib/spack/spack/cmd/style.py | 51 +++--- lib/spack/spack/test/cmd/style.py | 15 +- 4 files changed, 156 insertions(+), 156 deletions(-) diff --git a/lib/spack/spack/analyzers/libabigail.py b/lib/spack/spack/analyzers/libabigail.py index 9b26f3ca6f..88802ec28c 100644 --- a/lib/spack/spack/analyzers/libabigail.py +++ b/lib/spack/spack/analyzers/libabigail.py @@ -2,8 +2,6 @@ # Spack Project Developers. See the top-level COPYRIGHT file for details. # # SPDX-License-Identifier: (Apache-2.0 OR MIT) - - import os import llnl.util.tty as tty @@ -16,6 +14,7 @@ import spack.monitor import spack.package import spack.repo +import spack.util.executable from .analyzer_base import AnalyzerBase @@ -40,13 +39,12 @@ def __init__(self, spec, dirname=None): tty.debug("Preparing to use Libabigail, will install if missing.") with spack.bootstrap.ensure_bootstrap_configuration(): - # libabigail won't install lib/bin/share without docs spec = spack.spec.Spec("libabigail+docs") - spec.concretize() - - self.abidw = spack.bootstrap.get_executable( - "abidw", spec=spec, install=True) + spack.bootstrap.ensure_executables_in_path_or_raise( + ["abidw"], abstract_spec=spec + ) + self.abidw = spack.util.executable.which('abidw') def run(self): """ diff --git a/lib/spack/spack/bootstrap.py b/lib/spack/spack/bootstrap.py index 52fadbf700..62621fe581 100644 --- a/lib/spack/spack/bootstrap.py +++ b/lib/spack/spack/bootstrap.py @@ -13,6 +13,8 @@ import re import sys +import six + try: import sysconfig # novm except ImportError: @@ -38,7 +40,6 @@ import spack.user_environment import spack.util.executable import spack.util.path -from spack.util.environment import EnvironmentModifications #: "spack buildcache" command, initialized lazily _buildcache_cmd = None @@ -60,29 +61,39 @@ def _register(cls): return _register -def _try_import_from_store(module, abstract_spec_str): +def _try_import_from_store(module, query_spec, query_info=None): """Return True if the module can be imported from an already installed spec, False otherwise. Args: module: Python module to be imported - abstract_spec_str: abstract spec that may provide the module + query_spec: spec that may provide the module + query_info (dict or None): if a dict is passed it is populated with the + command found and the concrete spec providing it """ - bincache_platform = spack.platforms.real_host() - if str(bincache_platform) == 'cray': - bincache_platform = spack.platforms.linux.Linux() - with spack.platforms.use_platform(bincache_platform): - abstract_spec_str = str(spack.spec.Spec(abstract_spec_str)) + # If it is a string assume it's one of the root specs by this module + if isinstance(query_spec, six.string_types): + bincache_platform = spack.platforms.real_host() + if str(bincache_platform) == 'cray': + bincache_platform = spack.platforms.linux.Linux() + with spack.platforms.use_platform(bincache_platform): + query_spec = str(spack.spec.Spec(query_spec)) - # We have to run as part of this python interpreter - abstract_spec_str += ' ^' + spec_for_current_python() + # We have to run as part of this python interpreter + query_spec += ' ^' + spec_for_current_python() - installed_specs = spack.store.db.query(abstract_spec_str, installed=True) + installed_specs = spack.store.db.query(query_spec, installed=True) for candidate_spec in installed_specs: - lib_spd = candidate_spec['python'].package.default_site_packages_dir + python_spec = candidate_spec['python'] + lib_spd = python_spec.package.default_site_packages_dir lib64_spd = lib_spd.replace('lib/', 'lib64/') + lib_debian_derivative = os.path.join( + 'lib', 'python{0}'.format(python_spec.version.up_to(1)), 'dist-packages' + ) + module_paths = [ + os.path.join(candidate_spec.prefix, lib_debian_derivative), os.path.join(candidate_spec.prefix, lib_spd), os.path.join(candidate_spec.prefix, lib64_spd) ] @@ -93,9 +104,11 @@ def _try_import_from_store(module, abstract_spec_str): if _python_import(module): msg = ('[BOOTSTRAP MODULE {0}] The installed spec "{1}/{2}" ' 'provides the "{0}" Python module').format( - module, abstract_spec_str, candidate_spec.dag_hash() + module, query_spec, candidate_spec.dag_hash() ) tty.debug(msg) + if query_info is not None: + query_info['spec'] = candidate_spec return True except Exception as e: msg = ('unexpected error while trying to import module ' @@ -105,7 +118,7 @@ def _try_import_from_store(module, abstract_spec_str): msg = "Spec {0} did not provide module {1}" tty.warn(msg.format(candidate_spec, module)) - sys.path = sys.path[:-2] + sys.path = sys.path[:-3] return False @@ -175,7 +188,7 @@ def _fix_ext_suffix(candidate_spec): os.symlink(abs_path, link_name) -def _executables_in_store(executables, abstract_spec_str): +def _executables_in_store(executables, query_spec, query_info=None): """Return True if at least one of the executables can be retrieved from a spec in store, False otherwise. @@ -185,12 +198,14 @@ def _executables_in_store(executables, abstract_spec_str): Args: executables: list of executables to be searched - abstract_spec_str: abstract spec that may provide the executable + query_spec: spec that may provide the executable + query_info (dict or None): if a dict is passed it is populated with the + command found and the concrete spec providing it """ executables_str = ', '.join(executables) msg = "[BOOTSTRAP EXECUTABLES {0}] Try installed specs with query '{1}'" - tty.debug(msg.format(executables_str, abstract_spec_str)) - installed_specs = spack.store.db.query(abstract_spec_str, installed=True) + tty.debug(msg.format(executables_str, query_spec)) + installed_specs = spack.store.db.query(query_spec, installed=True) if installed_specs: for concrete_spec in installed_specs: bin_dir = concrete_spec.prefix.bin @@ -199,6 +214,11 @@ def _executables_in_store(executables, abstract_spec_str): if (os.path.exists(bin_dir) and os.path.isdir(bin_dir) and spack.util.executable.which_string(*executables, path=bin_dir)): spack.util.environment.path_put_first('PATH', [bin_dir]) + if query_info is not None: + query_info['command'] = spack.util.executable.which( + *executables, path=bin_dir + ) + query_info['spec'] = concrete_spec return True return False @@ -209,6 +229,7 @@ class _BuildcacheBootstrapper(object): def __init__(self, conf): self.name = conf['name'] self.url = conf['info']['url'] + self.last_search = None @staticmethod def _spec_and_platform(abstract_spec_str): @@ -304,7 +325,9 @@ def _install_and_test( pkg_hash, pkg_sha256, index, bincache_platform ) - if test_fn(): + info = {} + if test_fn(query_spec=abstract_spec, query_info=info): + self.last_search = info return True return False @@ -315,8 +338,8 @@ def mirror_scope(self): ) def try_import(self, module, abstract_spec_str): - test_fn = functools.partial(_try_import_from_store, module, abstract_spec_str) - if test_fn(): + test_fn, info = functools.partial(_try_import_from_store, module), {} + if test_fn(query_spec=abstract_spec_str, query_info=info): return True tty.info("Bootstrapping {0} from pre-built binaries".format(module)) @@ -329,15 +352,12 @@ def try_import(self, module, abstract_spec_str): ) def try_search_path(self, executables, abstract_spec_str): - test_fn = functools.partial( - _executables_in_store, executables, abstract_spec_str - ) - if test_fn(): + test_fn, info = functools.partial(_executables_in_store, executables), {} + if test_fn(query_spec=abstract_spec_str, query_info=info): + self.last_search = info return True - abstract_spec, bincache_platform = self._spec_and_platform( - abstract_spec_str - ) + abstract_spec, bincache_platform = self._spec_and_platform(abstract_spec_str) tty.info("Bootstrapping {0} from pre-built binaries".format(abstract_spec.name)) data = self._read_metadata(abstract_spec.name) return self._install_and_test( @@ -350,10 +370,12 @@ class _SourceBootstrapper(object): """Install the software needed during bootstrapping from sources.""" def __init__(self, conf): self.conf = conf + self.last_search = None - @staticmethod - def try_import(module, abstract_spec_str): - if _try_import_from_store(module, abstract_spec_str): + def try_import(self, module, abstract_spec_str): + info = {} + if _try_import_from_store(module, abstract_spec_str, query_info=info): + self.last_search = info return True tty.info("Bootstrapping {0} from sources".format(module)) @@ -384,10 +406,15 @@ def try_import(module, abstract_spec_str): # Install the spec that should make the module importable concrete_spec.package.do_install(fail_fast=True) - return _try_import_from_store(module, abstract_spec_str=abstract_spec_str) + if _try_import_from_store(module, query_spec=concrete_spec, query_info=info): + self.last_search = info + return True + return False def try_search_path(self, executables, abstract_spec_str): - if _executables_in_store(executables, abstract_spec_str): + info = {} + if _executables_in_store(executables, abstract_spec_str, query_info=info): + self.last_search = info return True # If we compile code from sources detecting a few build tools @@ -404,7 +431,11 @@ def try_search_path(self, executables, abstract_spec_str): msg = "[BOOTSTRAP GnuPG] Try installing '{0}' from sources" tty.debug(msg.format(abstract_spec_str)) concrete_spec.package.do_install() - return _executables_in_store(executables, abstract_spec_str) + print('Here') + if _executables_in_store(executables, concrete_spec, query_info=info): + self.last_search = info + return True + return False def _make_bootstrapper(conf): @@ -527,9 +558,13 @@ def ensure_executables_in_path_or_raise(executables, abstract_spec): Raises: RuntimeError: if the executables cannot be ensured to be in PATH + + Return: + Executable object """ - if spack.util.executable.which_string(*executables): - return + cmd = spack.util.executable.which(*executables) + if cmd: + return cmd executables_str = ', '.join(executables) source_configs = spack.config.get('bootstrap:sources', []) @@ -543,7 +578,17 @@ def ensure_executables_in_path_or_raise(executables, abstract_spec): b = _make_bootstrapper(current_config) try: if b.try_search_path(executables, abstract_spec): - return + # Additional environment variables needed + concrete_spec, cmd = b.last_search['spec'], b.last_search['command'] + env_mods = spack.util.environment.EnvironmentModifications() + for dep in concrete_spec.traverse( + root=True, order='post', deptype=('link', 'run') + ): + env_mods.extend( + spack.user_environment.environment_modifications_for_spec(dep) + ) + cmd.add_default_envmod(env_mods) + return cmd except Exception as e: msg = '[BOOTSTRAP EXECUTABLES {0}] Unexpected error "{1}"' tty.debug(msg.format(executables_str, str(e))) @@ -563,75 +608,6 @@ def _python_import(module): return True -def get_executable(exe, spec=None, install=False): - """Find an executable named exe, either in PATH or in Spack - - Args: - exe (str): needed executable name - spec (spack.spec.Spec or str): spec to search for exe in (default exe) - install (bool): install spec if not available - - When ``install`` is True, Spack will use the python used to run Spack as an - external. The ``install`` option should only be used with packages that - install quickly (when using external python) or are guaranteed by Spack - organization to be in a binary mirror (clingo). - """ - # Search the system first - runner = spack.util.executable.which(exe) - if runner: - return runner - - # Check whether it's already installed - spec = spack.spec.Spec(spec or exe) - installed_specs = spack.store.db.query(spec, installed=True) - for ispec in installed_specs: - # filter out directories of the same name as the executable - exe_path = [exe_p for exe_p in fs.find(ispec.prefix, exe) - if fs.is_exe(exe_p)] - if exe_path: - ret = spack.util.executable.Executable(exe_path[0]) - envmod = EnvironmentModifications() - for dep in ispec.traverse(root=True, order='post'): - envmod.extend( - spack.user_environment.environment_modifications_for_spec(dep) - ) - ret.add_default_envmod(envmod) - return ret - else: - tty.warn('Exe %s not found in prefix %s' % (exe, ispec.prefix)) - - def _raise_error(executable, exe_spec): - error_msg = 'cannot find the executable "{0}"'.format(executable) - if exe_spec: - error_msg += ' from spec "{0}'.format(exe_spec) - raise RuntimeError(error_msg) - - # If we're not allowed to install this for ourselves, we can't find it - if not install: - _raise_error(exe, spec) - - with spack_python_interpreter(): - # We will install for ourselves, using this python if needed - # Concretize the spec - spec.concretize() - - spec.package.do_install() - # filter out directories of the same name as the executable - exe_path = [exe_p for exe_p in fs.find(spec.prefix, exe) - if fs.is_exe(exe_p)] - if exe_path: - ret = spack.util.executable.Executable(exe_path[0]) - envmod = EnvironmentModifications() - for dep in spec.traverse(root=True, order='post'): - envmod.extend( - spack.user_environment.environment_modifications_for_spec(dep) - ) - ret.add_default_envmod(envmod) - return ret - - _raise_error(exe, spec) - - def _bootstrap_config_scopes(): tty.debug('[BOOTSTRAP CONFIG SCOPE] name=_builtin') config_scopes = [ @@ -784,5 +760,49 @@ 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( - executables=['gpg2', 'gpg'], abstract_spec=gnupg_root_spec(), + executables=['gpg2', 'gpg'], abstract_spec=gnupg_root_spec() ) + +### +# Development dependencies +### + + +def isort_root_spec(): + return _root_spec('py-isort@4.3.5:') + + +def ensure_isort_in_path_or_raise(): + """Ensure that isort is in the PATH or raise.""" + executable, root_spec = 'isort', isort_root_spec() + return ensure_executables_in_path_or_raise([executable], abstract_spec=root_spec) + + +def mypy_root_spec(): + return _root_spec('py-mypy@0.900:') + + +def ensure_mypy_in_path_or_raise(): + """Ensure that mypy is in the PATH or raise.""" + executable, root_spec = 'mypy', mypy_root_spec() + return ensure_executables_in_path_or_raise([executable], abstract_spec=root_spec) + + +def black_root_spec(): + return _root_spec('py-black') + + +def ensure_black_in_path_or_raise(): + """Ensure that isort is in the PATH or raise.""" + executable, root_spec = 'black', black_root_spec() + return ensure_executables_in_path_or_raise([executable], abstract_spec=root_spec) + + +def flake8_root_spec(): + return _root_spec('py-flake8') + + +def ensure_flake8_in_path_or_raise(): + """Ensure that flake8 is in the PATH or raise.""" + executable, root_spec = 'flake8', flake8_root_spec() + return ensure_executables_in_path_or_raise([executable], abstract_spec=root_spec) diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py index eb95904dfb..648f30bed0 100644 --- a/lib/spack/spack/cmd/style.py +++ b/lib/spack/spack/cmd/style.py @@ -48,10 +48,10 @@ def grouper(iterable, n, fillvalue=None): #: double-check the results of other tools (if, e.g., --fix was provided) #: The list maps an executable name to a spack spec needed to install it. tool_order = [ - ("isort", "py-isort@4.3.5:"), - ("mypy", "py-mypy@0.900:"), - ("black", "py-black"), - ("flake8", "py-flake8"), + ("isort", spack.bootstrap.ensure_isort_in_path_or_raise), + ("mypy", spack.bootstrap.ensure_mypy_in_path_or_raise), + ("black", spack.bootstrap.ensure_black_in_path_or_raise), + ("flake8", spack.bootstrap.ensure_flake8_in_path_or_raise), ] #: tools we run in spack style @@ -387,40 +387,33 @@ def prefix_relative(path): file_list = [prefix_relative(p) for p in file_list] - returncode = 0 + return_code = 0 with working_dir(args.root): if not file_list: file_list = changed_files(args.base, args.untracked, args.all) print_style_header(file_list, args) - # run tools in order defined in tool_order - returncode = 0 - for tool_name, tool_spec in tool_order: - if getattr(args, tool_name): + commands = {} + with spack.bootstrap.ensure_bootstrap_configuration(): + for tool_name, bootstrap_fn in tool_order: + # Skip the tool if it was not requested + if not getattr(args, tool_name): + continue + + commands[tool_name] = bootstrap_fn() + + for tool_name, bootstrap_fn in tool_order: + # Skip the tool if it was not requested + if not getattr(args, tool_name): + continue + run_function, required = tools[tool_name] print_tool_header(tool_name) + return_code |= run_function(commands[tool_name], file_list, args) - try: - # Bootstrap tools so we don't need to require install - with spack.bootstrap.ensure_bootstrap_configuration(): - spec = spack.spec.Spec(tool_spec) - cmd = None - cmd = spack.bootstrap.get_executable( - tool_name, spec=spec, install=True - ) - if not cmd: - color.cprint(" @y{%s not in PATH, skipped}" % tool_name) - continue - returncode |= run_function(cmd, file_list, args) - - except Exception as e: - raise spack.error.SpackError( - "Couldn't bootstrap %s:" % tool_name, str(e) - ) - - if returncode == 0: + if return_code == 0: tty.msg(color.colorize("@*{spack style checks were clean}")) else: tty.error(color.colorize("@*{spack style found errors}")) - return returncode + return return_code diff --git a/lib/spack/spack/test/cmd/style.py b/lib/spack/spack/test/cmd/style.py index 29cde14400..4e8b3b9784 100644 --- a/lib/spack/spack/test/cmd/style.py +++ b/lib/spack/spack/test/cmd/style.py @@ -41,7 +41,8 @@ def has_develop_branch(): # The style tools have requirements to use newer Python versions. We simplify by # requiring Python 3.6 or higher to run spack style. skip_old_python = pytest.mark.skipif( - sys.version_info < (3, 6), reason='requires Python 3.6 or higher') + sys.version_info < (3, 6), reason='requires Python 3.6 or higher' +) @pytest.fixture(scope="function") @@ -164,18 +165,6 @@ def test_style_is_package(tmpdir): assert not spack.cmd.style.is_package("lib/spack/external/pytest.py") -@skip_old_python -def test_bad_bootstrap(monkeypatch): - """Ensure we fail gracefully when we can't bootstrap spack style.""" - monkeypatch.setattr(spack.cmd.style, "tool_order", [ - ("isort", "py-isort@4.3:4.0"), # bad spec to force concretization failure - ]) - # zero out path to ensure we don't find isort - with pytest.raises(spack.error.SpackError) as e: - style(env={"PATH": ""}) - assert "Couldn't bootstrap isort" in str(e) - - @pytest.fixture def external_style_root(flake8_package_with_errors, tmpdir): """Create a mock git repository for running spack style."""