From 5bf6b7e6a5f52429011ca16e0275848c27df4c9b Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 5 Aug 2022 12:36:33 +0200 Subject: Refactor `cmd/install.py` for better readability (#31936) * Extracted two functions in cmd/install.py * Extracted a function to perform installation from the active environment * Rename a few functions, remove args from their arguments * Rework conditional in install_from_active_environment to reduce nesting in the function * Extract functions to parsespecs from cli and files * Extract functions to getuser confirmation for overwrite * Extract functions to install specs inside and outside environments * Rename a couple of functions * Fix outdated comment * Add missing imports * Split conditional to dedent one level * Invert check and exit early to dedent one level when requiring user confirmation --- lib/spack/spack/cmd/install.py | 486 +++++++++++++++++++++-------------------- 1 file changed, 252 insertions(+), 234 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py index c08eb2f3fd..f74b164b31 100644 --- a/lib/spack/spack/cmd/install.py +++ b/lib/spack/spack/cmd/install.py @@ -15,10 +15,14 @@ import llnl.util.tty as tty import spack.build_environment import spack.cmd import spack.cmd.common.arguments as arguments +import spack.config import spack.environment as ev import spack.fetch_strategy +import spack.package_base import spack.paths import spack.report +import spack.spec +import spack.store from spack.error import SpackError from spack.installer import PackageInstaller @@ -27,43 +31,28 @@ section = "build" level = "short" -def update_kwargs_from_args(args, kwargs): - """Parse cli arguments and construct a dictionary - that will be passed to the package installer.""" - - kwargs.update( - { - "fail_fast": args.fail_fast, - "keep_prefix": args.keep_prefix, - "keep_stage": args.keep_stage, - "restage": not args.dont_restage, - "install_source": args.install_source, - "verbose": args.verbose or args.install_verbose, - "fake": args.fake, - "dirty": args.dirty, - "use_cache": args.use_cache, - "cache_only": args.cache_only, - "include_build_deps": args.include_build_deps, - "explicit": True, # Always true for install command - "stop_at": args.until, - "unsigned": args.unsigned, - } - ) - - kwargs.update( - { - "install_deps": ("dependencies" in args.things_to_install), - "install_package": ("package" in args.things_to_install), - } - ) - - if hasattr(args, "setup"): - setups = set() - for arglist_s in args.setup: - for arg in [x.strip() for x in arglist_s.split(",")]: - setups.add(arg) - kwargs["setup"] = setups - tty.msg("Setup={0}".format(kwargs["setup"])) +def install_kwargs_from_args(args): + """Translate command line arguments into a dictionary that will be passed + to the package installer. + """ + return { + "fail_fast": args.fail_fast, + "keep_prefix": args.keep_prefix, + "keep_stage": args.keep_stage, + "restage": not args.dont_restage, + "install_source": args.install_source, + "verbose": args.verbose or args.install_verbose, + "fake": args.fake, + "dirty": args.dirty, + "use_cache": args.use_cache, + "cache_only": args.cache_only, + "include_build_deps": args.include_build_deps, + "explicit": True, # Use true as a default for install command + "stop_at": args.until, + "unsigned": args.unsigned, + "install_deps": ("dependencies" in args.things_to_install), + "install_package": ("package" in args.things_to_install), + } def setup_parser(subparser): @@ -224,8 +213,7 @@ packages. If neither are chosen, don't run tests for any packages.""", ) arguments.add_cdash_args(subparser, False) arguments.add_common_arguments(subparser, ["yes_to_all", "spec"]) - - spack.cmd.common.arguments.add_concretizer_args(subparser) + arguments.add_concretizer_args(subparser) def default_log_file(spec): @@ -239,87 +227,12 @@ def default_log_file(spec): return fs.os.path.join(dirname, basename) -def install_specs(cli_args, kwargs, specs): - """Do the actual installation. - - Args: - cli_args (argparse.Namespace): argparse namespace with command arguments - kwargs (dict): keyword arguments - specs (list): list of (abstract, concrete) spec tuples - """ - - # handle active environment, if any - env = ev.active_environment() - +def install_specs(specs, install_kwargs, cli_args): try: - if env: - specs_to_install = [] - specs_to_add = [] - for abstract, concrete in specs: - # This won't find specs added to the env since last - # concretize, therefore should we consider enforcing - # concretization of the env before allowing to install - # specs? - m_spec = env.matching_spec(abstract) - - # If there is any ambiguity in the above call to matching_spec - # (i.e. if more than one spec in the environment matches), then - # SpackEnvironmentError is raised, with a message listing the - # the matches. Getting to this point means there were either - # no matches or exactly one match. - - if not m_spec: - tty.debug("{0} matched nothing in the env".format(abstract.name)) - # no matches in the env - if cli_args.no_add: - msg = ( - "You asked to install {0} without adding it " - + "(--no-add), but no such spec exists in " - + "environment" - ).format(abstract.name) - tty.die(msg) - else: - tty.debug("adding {0} as a root".format(abstract.name)) - specs_to_add.append((abstract, concrete)) - - continue - - tty.debug( - "exactly one match for {0} in env -> {1}".format( - m_spec.name, m_spec.dag_hash() - ) - ) - - if m_spec in env.roots() or cli_args.no_add: - # either the single match is a root spec (and --no-add is - # the default for roots) or --no-add was stated explicitly - tty.debug("just install {0}".format(m_spec.name)) - specs_to_install.append(m_spec) - else: - # the single match is not a root (i.e. it's a dependency), - # and --no-add was not specified, so we'll add it as a - # root before installing - tty.debug("add {0} then install it".format(m_spec.name)) - specs_to_add.append((abstract, concrete)) - - if specs_to_add: - tty.debug("Adding the following specs as roots:") - for abstract, concrete in specs_to_add: - tty.debug(" {0}".format(abstract.name)) - with env.write_transaction(): - specs_to_install.append(env.concretize_and_add(abstract, concrete)) - env.write(regenerate=False) - - # Install the validated list of cli specs - if specs_to_install: - tty.debug("Installing the following cli specs:") - for s in specs_to_install: - tty.debug(" {0}".format(s.name)) - env.install_specs(specs_to_install, args=cli_args, **kwargs) + if ev.active_environment(): + install_specs_inside_environment(specs, install_kwargs, cli_args) else: - installs = [(concrete.package, kwargs) for _, concrete in specs] - builder = PackageInstaller(installs) - builder.install() + install_specs_outside_environment(specs, install_kwargs) except spack.build_environment.InstallError as e: if cli_args.show_log_on_error: e.print_context() @@ -332,113 +245,179 @@ def install_specs(cli_args, kwargs, specs): raise -def install(parser, args, **kwargs): - # TODO: unify args.verbose? - tty.set_verbose(args.verbose or args.install_verbose) +def install_specs_inside_environment(specs, install_kwargs, cli_args): + specs_to_install, specs_to_add = [], [] + env = ev.active_environment() + for abstract, concrete in specs: + # This won't find specs added to the env since last + # concretize, therefore should we consider enforcing + # concretization of the env before allowing to install + # specs? + m_spec = env.matching_spec(abstract) + + # If there is any ambiguity in the above call to matching_spec + # (i.e. if more than one spec in the environment matches), then + # SpackEnvironmentError is raised, with a message listing the + # the matches. Getting to this point means there were either + # no matches or exactly one match. + + if not m_spec and cli_args.no_add: + msg = ( + "You asked to install {0} without adding it (--no-add), but no such spec " + "exists in environment" + ).format(abstract.name) + tty.die(msg) - if args.help_cdash: - parser = argparse.ArgumentParser( - formatter_class=argparse.RawDescriptionHelpFormatter, - epilog=textwrap.dedent( - """\ + if not m_spec: + tty.debug("adding {0} as a root".format(abstract.name)) + specs_to_add.append((abstract, concrete)) + continue + + tty.debug("exactly one match for {0} in env -> {1}".format(m_spec.name, m_spec.dag_hash())) + + if m_spec in env.roots() or cli_args.no_add: + # either the single match is a root spec (and --no-add is + # the default for roots) or --no-add was stated explicitly + tty.debug("just install {0}".format(m_spec.name)) + specs_to_install.append(m_spec) + else: + # the single match is not a root (i.e. it's a dependency), + # and --no-add was not specified, so we'll add it as a + # root before installing + tty.debug("add {0} then install it".format(m_spec.name)) + specs_to_add.append((abstract, concrete)) + if specs_to_add: + tty.debug("Adding the following specs as roots:") + for abstract, concrete in specs_to_add: + tty.debug(" {0}".format(abstract.name)) + with env.write_transaction(): + specs_to_install.append(env.concretize_and_add(abstract, concrete)) + env.write(regenerate=False) + # Install the validated list of cli specs + if specs_to_install: + tty.debug("Installing the following cli specs:") + for s in specs_to_install: + tty.debug(" {0}".format(s.name)) + env.install_specs(specs_to_install, **install_kwargs) + + +def install_specs_outside_environment(specs, install_kwargs): + installs = [(concrete.package, install_kwargs) for _, concrete in specs] + builder = PackageInstaller(installs) + builder.install() + + +def print_cdash_help(): + parser = argparse.ArgumentParser( + formatter_class=argparse.RawDescriptionHelpFormatter, + epilog=textwrap.dedent( + """\ environment variables: - SPACK_CDASH_AUTH_TOKEN - authentication token to present to CDash - """ - ), - ) - arguments.add_cdash_args(parser, True) - parser.print_help() - return +SPACK_CDASH_AUTH_TOKEN + authentication token to present to CDash + """ + ), + ) + arguments.add_cdash_args(parser, True) + parser.print_help() + +def _create_log_reporter(args): + # TODO: remove args injection to spack.report.collect_info, since a class in core + # TODO: shouldn't know what are the command line arguments a command use. reporter = spack.report.collect_info( spack.package_base.PackageInstaller, "_install_task", args.log_format, args ) if args.log_file: reporter.filename = args.log_file + return reporter - def get_tests(specs): - if args.test == "all": - return True - elif args.test == "root": - return [spec.name for spec in specs] - else: - return False - # Parse cli arguments and construct a dictionary - # that will be passed to the package installer - update_kwargs_from_args(args, kwargs) +def install_all_specs_from_active_environment( + install_kwargs, only_concrete, cli_test_arg, reporter +): + """Install all specs from the active environment - if not args.spec and not args.specfiles: - # if there are no args but an active environment - # then install the packages from it. - env = ev.active_environment() - if env: - tests = get_tests(env.user_specs) - kwargs["tests"] = tests - - if not args.only_concrete: - with env.write_transaction(): - concretized_specs = env.concretize(tests=tests) - ev.display_specs(concretized_specs) - - # save view regeneration for later, so that we only do it - # once, as it can be slow. - env.write(regenerate=False) - - specs = env.all_specs() - if specs: - if not args.log_file and not reporter.filename: - reporter.filename = default_log_file(specs[0]) - reporter.specs = specs - - tty.msg("Installing environment {0}".format(env.name)) - with reporter("build"): - env.install_all(**kwargs) + Args: + install_kwargs (dict): dictionary of options to be passed to the installer + only_concrete (bool): if true don't concretize the environment, but install + only the specs that are already concrete + cli_test_arg (bool or str): command line argument to select which test to run + reporter: reporter object for the installations + """ + env = ev.active_environment() + if not env: + msg = "install requires a package argument or active environment" + if "spack.yaml" in os.listdir(os.getcwd()): + # There's a spack.yaml file in the working dir, the user may + # have intended to use that + msg += "\n\n" + msg += "Did you mean to install using the `spack.yaml`" + msg += " in this directory? Try: \n" + msg += " spack env activate .\n" + msg += " spack install\n" + msg += " OR\n" + msg += " spack --env . install" + tty.die(msg) + + install_kwargs["tests"] = compute_tests_install_kwargs(env.user_specs, cli_test_arg) + if not only_concrete: + with env.write_transaction(): + concretized_specs = env.concretize(tests=install_kwargs["tests"]) + ev.display_specs(concretized_specs) + + # save view regeneration for later, so that we only do it + # once, as it can be slow. + env.write(regenerate=False) + + specs = env.all_specs() + if not specs: + msg = "{0} environment has no specs to install".format(env.name) + tty.msg(msg) + return - else: - msg = "{0} environment has no specs to install".format(env.name) - tty.msg(msg) + if not reporter.filename: + reporter.filename = default_log_file(specs[0]) + reporter.specs = specs - tty.debug("Regenerating environment views for {0}".format(env.name)) - with env.write_transaction(): - # write env to trigger view generation and modulefile - # generation - env.write() - return - else: - msg = "install requires a package argument or active environment" - if "spack.yaml" in os.listdir(os.getcwd()): - # There's a spack.yaml file in the working dir, the user may - # have intended to use that - msg += "\n\n" - msg += "Did you mean to install using the `spack.yaml`" - msg += " in this directory? Try: \n" - msg += " spack env activate .\n" - msg += " spack install\n" - msg += " OR\n" - msg += " spack --env . install" - tty.die(msg) + tty.msg("Installing environment {0}".format(env.name)) + with reporter("build"): + env.install_all(**install_kwargs) - if args.no_checksum: - spack.config.set("config:checksum", False, scope="command_line") + tty.debug("Regenerating environment views for {0}".format(env.name)) + with env.write_transaction(): + # write env to trigger view generation and modulefile + # generation + env.write() - if args.deprecated: - spack.config.set("config:deprecated", True, scope="command_line") - # 1. Abstract specs from cli - abstract_specs = spack.cmd.parse_specs(args.spec) - tests = get_tests(abstract_specs) - kwargs["tests"] = tests +def compute_tests_install_kwargs(specs, cli_test_arg): + """Translate the test cli argument into the proper install argument""" + if cli_test_arg == "all": + return True + elif cli_test_arg == "root": + return [spec.name for spec in specs] + return False + +def specs_from_cli(args, install_kwargs, reporter): + """Return abstract and concrete spec parsed from the command line.""" + abstract_specs = spack.cmd.parse_specs(args.spec) + install_kwargs["tests"] = compute_tests_install_kwargs(abstract_specs, args.test) try: - specs = spack.cmd.parse_specs(args.spec, concretize=True, tests=tests) + concrete_specs = spack.cmd.parse_specs( + args.spec, concretize=True, tests=install_kwargs["tests"] + ) except SpackError as e: tty.debug(e) reporter.concretization_report(e.message) raise + return abstract_specs, concrete_specs + - # 2. Concrete specs from yaml files +def concrete_specs_from_file(args): + """Return the list of concrete specs read from files.""" + result = [] for file in args.specfiles: with open(file, "r") as f: if file.endswith("yaml") or file.endswith("yml"): @@ -452,41 +431,80 @@ environment variables: msg += "The file does not contain a concrete spec." tty.warn(msg.format(file)) continue + result.append(concretized) + return result + + +def require_user_confirmation_for_overwrite(concrete_specs, args): + if args.yes_to_all: + return + + installed = list(filter(lambda x: x, map(spack.store.db.query_one, concrete_specs))) + display_args = {"long": True, "show_flags": True, "variants": True} + + if installed: + tty.msg("The following package specs will be " "reinstalled:\n") + spack.cmd.display_specs(installed, **display_args) + + not_installed = list(filter(lambda x: x not in installed, concrete_specs)) + if not_installed: + tty.msg( + "The following package specs are not installed and" + " the --overwrite flag was given. The package spec" + " will be newly installed:\n" + ) + spack.cmd.display_specs(not_installed, **display_args) - abstract_specs.append(s) - specs.append(concretized) + # We have some specs, so one of the above must have been true + answer = tty.get_yes_or_no("Do you want to proceed?", default=False) + if not answer: + tty.die("Reinstallation aborted.") - if len(specs) == 0: + +def install(parser, args): + # TODO: unify args.verbose? + tty.set_verbose(args.verbose or args.install_verbose) + + if args.help_cdash: + print_cdash_help() + return + + if args.no_checksum: + spack.config.set("config:checksum", False, scope="command_line") + + if args.deprecated: + spack.config.set("config:deprecated", True, scope="command_line") + + reporter = _create_log_reporter(args) + install_kwargs = install_kwargs_from_args(args) + + if not args.spec and not args.specfiles: + # If there are no args but an active environment then install the packages from it. + install_all_specs_from_active_environment( + install_kwargs=install_kwargs, + only_concrete=args.only_concrete, + cli_test_arg=args.test, + reporter=reporter, + ) + return + + # Specs from CLI + abstract_specs, concrete_specs = specs_from_cli(args, install_kwargs, reporter) + + # Concrete specs from YAML or JSON files + specs_from_file = concrete_specs_from_file(args) + abstract_specs.extend(specs_from_file) + concrete_specs.extend(specs_from_file) + + if len(concrete_specs) == 0: tty.die("The `spack install` command requires a spec to install.") - if not args.log_file and not reporter.filename: - reporter.filename = default_log_file(specs[0]) - reporter.specs = specs + if not reporter.filename: + reporter.filename = default_log_file(concrete_specs[0]) + reporter.specs = concrete_specs + with reporter("build"): if args.overwrite: - - installed = list(filter(lambda x: x, map(spack.store.db.query_one, specs))) - if not args.yes_to_all: - display_args = {"long": True, "show_flags": True, "variants": True} - - if installed: - tty.msg("The following package specs will be " "reinstalled:\n") - spack.cmd.display_specs(installed, **display_args) - - not_installed = list(filter(lambda x: x not in installed, specs)) - if not_installed: - tty.msg( - "The following package specs are not installed and" - " the --overwrite flag was given. The package spec" - " will be newly installed:\n" - ) - spack.cmd.display_specs(not_installed, **display_args) - - # We have some specs, so one of the above must have been true - answer = tty.get_yes_or_no("Do you want to proceed?", default=False) - if not answer: - tty.die("Reinstallation aborted.") - - # overwrite all concrete explicit specs from this build - kwargs["overwrite"] = [spec.dag_hash() for spec in specs] - install_specs(args, kwargs, zip(abstract_specs, specs)) + require_user_confirmation_for_overwrite(concrete_specs, args) + install_kwargs["overwrite"] = [spec.dag_hash() for spec in concrete_specs] + install_specs(zip(abstract_specs, concrete_specs), install_kwargs, args) -- cgit v1.2.3-60-g2f50