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

1 files changed, 252 insertions, 234 deletions

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)