spack install: simplify behavior when inside environments (#35206)

Example one:

```
spack install --add x y z
```

is equivalent to

```
spack add x y z
spack concretize
spack install --only-concrete
```

where `--only-concrete` installs without modifying spack.yaml/spack.lock

Example two:

```
spack install
```

concretizes current spack.yaml if outdated and installs all specs.

Example three:

```
spack install x y z
```

concretizes current spack.yaml if outdated and installs *only* concrete
specs in the environment that match abstract specs `x`, `y`, or `z`.

lib/spack/spack/cmd/install.py

@@ -263,146 +263,6 @@ def report_filename(args: argparse.Namespace, specs: List[spack.spec.Spec]) -> s
     return result
 
 
-def install_specs(specs, install_kwargs, cli_args):
-    try:
-        if ev.active_environment():
-            install_specs_inside_environment(specs, install_kwargs, cli_args)
-        else:
-            install_specs_outside_environment(specs, install_kwargs)
-    except spack.build_environment.InstallError as e:
-        if cli_args.show_log_on_error:
-            e.print_context()
-            assert e.pkg, "Expected InstallError to include the associated package"
-            if not os.path.exists(e.pkg.build_log_path):
-                tty.error("'spack install' created no log.")
-            else:
-                sys.stderr.write("Full build log:\n")
-                with open(e.pkg.build_log_path) as log:
-                    shutil.copyfileobj(log, sys.stderr)
-        raise
-
-
-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 not cli_args.add:
-            msg = (
-                "Cannot install '{0}' because it is not in the current environment."
-                " You can add it to the environment with 'spack add {0}', or as part"
-                " of the install command with 'spack install --add {0}'"
-            ).format(str(abstract))
-            tty.die(msg)
-
-        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 not cli_args.add:
-            # either the single match is a root spec (in which case
-            # the spec is not added to the env again), or the user did
-            # not specify --add (in which case it is assumed we are
-            # installing already-concretized specs in the env)
-            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 --add was 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 install_all_specs_from_active_environment(
-    install_kwargs, only_concrete, cli_test_arg, reporter_factory
-):
-    """Install all specs from the active environment
-
-    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
-
-    reporter = reporter_factory(specs) or lang.nullcontext()
-
-    tty.msg("Installing environment {0}".format(env.name))
-    with reporter:
-        env.install_all(**install_kwargs)
-
-    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()
-
-
 def compute_tests_install_kwargs(specs, cli_test_arg):
     """Translate the test cli argument into the proper install argument"""
     if cli_test_arg == "all":
@@ -412,43 +272,6 @@ def compute_tests_install_kwargs(specs, cli_test_arg):
     return False
 
 
-def specs_from_cli(args, install_kwargs):
-    """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:
-        concrete_specs = spack.cmd.parse_specs(
-            args.spec, concretize=True, tests=install_kwargs["tests"]
-        )
-    except SpackError as e:
-        tty.debug(e)
-        if args.log_format is not None:
-            reporter = args.reporter()
-            reporter.concretization_report(report_filename(args, abstract_specs), e.message)
-        raise
-    return abstract_specs, concrete_specs
-
-
-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"):
-                s = spack.spec.Spec.from_yaml(f)
-            else:
-                s = spack.spec.Spec.from_json(f)
-
-        concretized = s.concretized()
-        if concretized.dag_hash() != s.dag_hash():
-            msg = 'skipped invalid file "{0}". '
-            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
@@ -475,12 +298,40 @@ def require_user_confirmation_for_overwrite(concrete_specs, args):
         tty.die("Reinstallation aborted.")
 
 
+def _dump_log_on_error(e: spack.build_environment.InstallError):
+    e.print_context()
+    assert e.pkg, "Expected InstallError to include the associated package"
+    if not os.path.exists(e.pkg.build_log_path):
+        tty.error("'spack install' created no log.")
+    else:
+        sys.stderr.write("Full build log:\n")
+        with open(e.pkg.build_log_path, errors="replace") as log:
+            shutil.copyfileobj(log, sys.stderr)
+
+
+def _die_require_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"
+            "Did you mean to install using the `spack.yaml`"
+            " in this directory? Try: \n"
+            "    spack env activate .\n"
+            "    spack install\n"
+            "  OR\n"
+            "    spack --env . install"
+        )
+    tty.die(msg)
+
+
 def install(parser, args):
     # TODO: unify args.verbose?
     tty.set_verbose(args.verbose or args.install_verbose)
 
     if args.help_cdash:
-        spack.cmd.common.arguments.print_cdash_help()
+        arguments.print_cdash_help()
         return
 
     if args.no_checksum:
@@ -489,43 +340,150 @@ def install(parser, args):
     if args.deprecated:
         spack.config.set("config:deprecated", True, scope="command_line")
 
-    spack.cmd.common.arguments.sanitize_reporter_options(args)
+    arguments.sanitize_reporter_options(args)
 
     def reporter_factory(specs):
         if args.log_format is None:
-            return None
+            return lang.nullcontext()
 
-        context_manager = spack.report.build_context_manager(
+        return spack.report.build_context_manager(
             reporter=args.reporter(), filename=report_filename(args, specs=specs), specs=specs
         )
-        return context_manager
 
     install_kwargs = install_kwargs_from_args(args)
 
-    if not args.spec and not args.specfiles:
+    env = ev.active_environment()
-        # If there are no args but an active environment then install the packages from it.
+
-        install_all_specs_from_active_environment(
+    if not env and not args.spec and not args.specfiles:
-            install_kwargs=install_kwargs,
+        _die_require_env()
-            only_concrete=args.only_concrete,
+
-            cli_test_arg=args.test,
+    try:
-            reporter_factory=reporter_factory,
+        if env:
-        )
+            install_with_active_env(env, args, install_kwargs, reporter_factory)
+        else:
+            install_without_active_env(args, install_kwargs, reporter_factory)
+    except spack.build_environment.InstallError as e:
+        if args.show_log_on_error:
+            _dump_log_on_error(e)
+        raise
+
+
+def _maybe_add_and_concretize(args, env, specs):
+    """Handle the overloaded spack install behavior of adding
+    and automatically concretizing specs"""
+
+    # Users can opt out of accidental concretizations with --only-concrete
+    if args.only_concrete:
         return
 
-    # Specs from CLI
+    # Otherwise, we will modify the environment.
-    abstract_specs, concrete_specs = specs_from_cli(args, install_kwargs)
+    with env.write_transaction():
+        # `spack add` adds these specs.
+        if args.add:
+            for spec in specs:
+                env.add(spec)
 
-    # Concrete specs from YAML or JSON files
+        # `spack concretize`
-    specs_from_file = concrete_specs_from_file(args)
+        tests = compute_tests_install_kwargs(env.user_specs, args.test)
-    abstract_specs.extend(specs_from_file)
+        concretized_specs = env.concretize(tests=tests)
-    concrete_specs.extend(specs_from_file)
+        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)
+
+
+def install_with_active_env(env: ev.Environment, args, install_kwargs, reporter_factory):
+    specs = spack.cmd.parse_specs(args.spec)
+
+    # The following two commands are equivalent:
+    # 1. `spack install --add x y z`
+    # 2. `spack add x y z && spack concretize && spack install --only-concrete`
+    # here we do the `add` and `concretize` part.
+    _maybe_add_and_concretize(args, env, specs)
+
+    # Now we're doing `spack install --only-concrete`.
+    if args.add or not specs:
+        specs_to_install = env.concrete_roots()
+        if not specs_to_install:
+            tty.msg(f"{env.name} environment has no specs to install")
+            return
+
+    # `spack install x y z` without --add is installing matching specs in the env.
+    else:
+        specs_to_install = env.all_matching_specs(*specs)
+        if not specs_to_install:
+            msg = (
+                "Cannot install '{0}' because no matching specs are in the current environment."
+                " You can add specs to the environment with 'spack add {0}', or as part"
+                " of the install command with 'spack install --add {0}'"
+            ).format(" ".join(args.spec))
+            tty.die(msg)
+
+    install_kwargs["tests"] = compute_tests_install_kwargs(specs_to_install, args.test)
+
+    if args.overwrite:
+        require_user_confirmation_for_overwrite(specs_to_install, args)
+        install_kwargs["overwrite"] = [spec.dag_hash() for spec in specs_to_install]
+
+    try:
+        with reporter_factory(specs_to_install):
+            env.install_specs(specs_to_install, **install_kwargs)
+    finally:
+        # TODO: this is doing way too much to trigger
+        # views and modules to be generated.
+        with env.write_transaction():
+            env.write(regenerate=True)
+
+
+def concrete_specs_from_cli(args, install_kwargs):
+    """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:
+        concrete_specs = spack.cmd.parse_specs(
+            args.spec, concretize=True, tests=install_kwargs["tests"]
+        )
+    except SpackError as e:
+        tty.debug(e)
+        if args.log_format is not None:
+            reporter = args.reporter()
+            reporter.concretization_report(report_filename(args, abstract_specs), e.message)
+        raise
+    return concrete_specs
+
+
+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"):
+                s = spack.spec.Spec.from_yaml(f)
+            else:
+                s = spack.spec.Spec.from_json(f)
+
+        concretized = s.concretized()
+        if concretized.dag_hash() != s.dag_hash():
+            msg = 'skipped invalid file "{0}". '
+            msg += "The file does not contain a concrete spec."
+            tty.warn(msg.format(file))
+            continue
+        result.append(concretized)
+    return result
+
+
+def install_without_active_env(args, install_kwargs, reporter_factory):
+    concrete_specs = concrete_specs_from_cli(args, install_kwargs) + concrete_specs_from_file(args)
 
     if len(concrete_specs) == 0:
         tty.die("The `spack install` command requires a spec to install.")
 
-    reporter = reporter_factory(concrete_specs) or lang.nullcontext()
+    with reporter_factory(concrete_specs):
-    with reporter:
         if args.overwrite:
             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)
+
+        installs = [(s.package, install_kwargs) for s in concrete_specs]
+        builder = PackageInstaller(installs)
+        builder.install()

lib/spack/spack/environment/environment.py

@@ -13,6 +13,7 @@
 import time
 import urllib.parse
 import urllib.request
+from typing import List, Optional
 
 import ruamel.yaml as yaml
 
@@ -59,7 +60,7 @@
 
 
 #: currently activated environment
-_active_environment = None
+_active_environment: Optional["Environment"] = None
 
 
 #: default path where environments are stored in the spack tree
@@ -1552,12 +1553,11 @@ def update_default_view(self, viewpath):
 
     def regenerate_views(self):
         if not self.views:
-            tty.debug("Skip view update, this environment does not" " maintain a view")
+            tty.debug("Skip view update, this environment does not maintain a view")
             return
 
-        concretized_root_specs = [s for _, s in self.concretized_specs()]
         for view in self.views.values():
-            view.regenerate(concretized_root_specs)
+            view.regenerate(self.concrete_roots())
 
     def check_views(self):
         """Checks if the environments default view can be activated."""
@@ -1565,7 +1565,7 @@ def check_views(self):
             # This is effectively a no-op, but it touches all packages in the
             # default view if they are installed.
             for view_name, view in self.views.items():
-                for _, spec in self.concretized_specs():
+                for spec in self.concrete_roots():
                     if spec in view and spec.package and spec.installed:
                         msg = '{0} in view "{1}"'
                         tty.debug(msg.format(spec.name, view_name))
@@ -1583,7 +1583,7 @@ def _env_modifications_for_default_view(self, reverse=False):
         visited = set()
 
         errors = []
-        for _, root_spec in self.concretized_specs():
+        for root_spec in self.concrete_roots():
             if root_spec in self.default_view and root_spec.installed and root_spec.package:
                 for spec in root_spec.traverse(deptype="run", root=True):
                     if spec.name in visited:
@@ -1800,9 +1800,6 @@ def install_specs(self, specs=None, **install_args):
                             "Could not install log links for {0}: {1}".format(spec.name, str(e))
                         )
 
-            with self.write_transaction():
-                self.regenerate_views()
-
     def all_specs(self):
         """Return all specs, even those a user spec would shadow."""
         roots = [self.specs_by_hash[h] for h in self.concretized_order]
@@ -1847,6 +1844,11 @@ def concretized_specs(self):
         for s, h in zip(self.concretized_user_specs, self.concretized_order):
             yield (s, self.specs_by_hash[h])
 
+    def concrete_roots(self):
+        """Same as concretized_specs, except it returns the list of concrete
+        roots *without* associated user spec"""
+        return [root for _, root in self.concretized_specs()]
+
     def get_by_hash(self, dag_hash):
         matches = {}
         roots = [self.specs_by_hash[h] for h in self.concretized_order]
@@ -1863,6 +1865,15 @@ def get_one_by_hash(self, dag_hash):
         assert len(hash_matches) == 1
         return hash_matches[0]
 
+    def all_matching_specs(self, *specs: spack.spec.Spec) -> List[Spec]:
+        """Returns all concretized specs in the environment satisfying any of the input specs"""
+        key = lambda s: s.dag_hash()
+        return [
+            s
+            for s in spack.traverse.traverse_nodes(self.concrete_roots(), key=key)
+            if any(s.satisfies(t) for t in specs)
+        ]
+
     @spack.repo.autospec
     def matching_spec(self, spec):
         """

lib/spack/spack/test/cmd/install.py

@@ -793,7 +793,7 @@ def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery, mutable_mock
     #         ^b
     #     a
     #         ^b
-    e = ev.create("test")
+    e = ev.create("test", with_view=False)
     e.add("mpileaks")
     e.add("libelf@0.8.10")  # so env has both root and dep libelf specs
     e.add("a")
@@ -829,14 +829,11 @@ def test_install_no_add_in_env(tmpdir, mock_fetch, install_mockery, mutable_mock
         # Assert using --no-add with a spec not in the env fails
         inst_out = install("--no-add", "boost", fail_on_error=False, output=str)
 
-        assert "You can add it to the environment with 'spack add " in inst_out
+        assert "You can add specs to the environment with 'spack add " in inst_out
 
-        # Without --add, ensure that install fails if the spec matches more
+        # Without --add, ensure that two packages "a" get installed
-        # than one root
+        inst_out = install("a", output=str)
-        with pytest.raises(ev.SpackEnvironmentError) as err:
+        assert len([x for x in e.all_specs() if x.installed and x.name == "a"]) == 2
-            inst_out = install("a", output=str)
-
-        assert "a matches multiple specs in the env" in str(err)
 
         # Install an unambiguous dependency spec (that already exists as a dep
         # in the environment) and make sure it gets installed (w/ deps),
@@ -1177,6 +1174,6 @@ def test_report_filename_for_cdash(install_mockery_mutable_config, mock_fetch):
     args = parser.parse_args(
         ["--cdash-upload-url", "https://blahblah/submit.php?project=debugging", "a"]
     )
-    _, specs = spack.cmd.install.specs_from_cli(args, {})
+    specs = spack.cmd.install.concrete_specs_from_cli(args, {})
     filename = spack.cmd.install.report_filename(args, specs)
     assert filename != "https://blahblah/submit.php?project=debugging"

share/spack/templates/depfile/Makefile

@@ -19,11 +19,11 @@ SPACK_INSTALL_FLAGS ?=
 {% endif %}
 
 # The spack install commands are of the form:
-# spack -e my_env --no-add --only=package --only=concrete /hash
+# spack -e my_env --only=package --only=concrete /hash
 # This is an involved way of expressing that Spack should only install
 # an individual concrete spec from the environment without deps.
 {{ install_target }}/%: | {{ dirs_target }}
-	{{ jobserver_support }}$(SPACK) -e '{{ environment }}' install $(SPACK_BUILDCACHE_FLAG) $(SPACK_INSTALL_FLAGS) --only-concrete --only=package --no-add /$(HASH) # $(SPEC)
+	{{ jobserver_support }}$(SPACK) -e '{{ environment }}' install $(SPACK_BUILDCACHE_FLAG) $(SPACK_INSTALL_FLAGS) --only-concrete --only=package /$(HASH) # $(SPEC)
 	@touch $@
 
 {{ install_deps_target }}/%: | {{ dirs_target }}