Buildcache commands cleanup, again... (#39203)

* Inform mypy that tty.die is noreturn

* avoid temporary allocation in env

* update spack buildcache save-specfile

* fix spack buildcache check/download/get-buildcache-name

- ensure that required args and mutually exclusive ones are marked as
  such in argparse for better error messages
- deprecate --spec-file everywhere
- use disambiguate for better error messages

6 files changed, 114 insertions, 158 deletions

diff --git a/lib/spack/llnl/util/tty/__init__.py b/lib/spack/llnl/util/tty/__init__.py
index 05e81ef615..b3975cc08d 100644
--- a/lib/spack/llnl/util/tty/__init__.py
+++ b/lib/spack/llnl/util/tty/__init__.py
@@ -12,6 +12,7 @@ import textwrap
 import traceback
 from datetime import datetime
 from sys import platform as _platform
+from typing import NoReturn
 
 if _platform != "win32":
     import fcntl
@@ -244,7 +245,7 @@ def warn(message, *args, **kwargs):
     info("Warning: " + str(message), *args, **kwargs)
 
 
-def die(message, *args, **kwargs):
+def die(message, *args, **kwargs) -> NoReturn:
     kwargs.setdefault("countback", 4)
     error(message, *args, **kwargs)
     sys.exit(1)
diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index d7fca9f561..22117ba6fa 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -584,14 +584,14 @@ def require_active_env(cmd_name):
 
     if env:
         return env
-    else:
-        tty.die(
-            "`spack %s` requires an environment" % cmd_name,
-            "activate an environment first:",
-            "    spack env activate ENV",
-            "or use:",
-            "    spack -e ENV %s ..." % cmd_name,
-        )
+
+    tty.die(
+        "`spack %s` requires an environment" % cmd_name,
+        "activate an environment first:",
+        "    spack env activate ENV",
+        "or use:",
+        "    spack -e ENV %s ..." % cmd_name,
+    )
 
 
 def find_environment(args):
diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py
index d30498f0b5..74594ac3f1 100644
--- a/lib/spack/spack/cmd/buildcache.py
+++ b/lib/spack/spack/cmd/buildcache.py
@@ -2,12 +2,14 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+import argparse
 import glob
 import json
 import os
 import shutil
 import sys
 import tempfile
+from typing import List
 
 import llnl.util.tty as tty
 import llnl.util.tty.color as clr
@@ -18,7 +20,6 @@ import spack.cmd
 import spack.cmd.common.arguments as arguments
 import spack.config
 import spack.environment as ev
-import spack.hash_types as ht
 import spack.mirror
 import spack.relocate
 import spack.repo
@@ -28,7 +29,6 @@ import spack.util.crypto
 import spack.util.url as url_util
 import spack.util.web as web_util
 from spack.cmd import display_specs
-from spack.error import SpecError
 from spack.spec import Spec, save_dependency_specfiles
 from spack.stage import Stage
 from spack.util.string import plural
@@ -38,8 +38,8 @@ section = "packaging"
 level = "long"
 
 
-def setup_parser(subparser):
-    setup_parser.parser = subparser
+def setup_parser(subparser: argparse.ArgumentParser):
+    setattr(setup_parser, "parser", subparser)
     subparsers = subparser.add_subparsers(help="buildcache sub-commands")
 
     push = subparsers.add_parser("push", aliases=["create"], help=push_fn.__doc__)
@@ -158,14 +158,12 @@ def setup_parser(subparser):
         default=spack.config.default_modify_scope(),
         help="configuration scope containing mirrors to check",
     )
-
-    check.add_argument(
-        "-s", "--spec", default=None, help="check single spec instead of release specs file"
+    check_spec_or_specfile = check.add_mutually_exclusive_group(required=True)
+    check_spec_or_specfile.add_argument(
+        "-s", "--spec", help="check single spec instead of release specs file"
     )
-
-    check.add_argument(
+    check_spec_or_specfile.add_argument(
         "--spec-file",
-        default=None,
         help="check single spec from json or yaml file instead of release specs file",
     )
 
@@ -173,16 +171,19 @@ def setup_parser(subparser):
 
     # Download tarball and specfile
     download = subparsers.add_parser("download", help=download_fn.__doc__)
-    download.add_argument(
-        "-s", "--spec", default=None, help="download built tarball for spec from mirror"
+    download_spec_or_specfile = download.add_mutually_exclusive_group(required=True)
+    download_spec_or_specfile.add_argument(
+        "-s", "--spec", help="download built tarball for spec from mirror"
     )
-    download.add_argument(
-        "--spec-file",
-        default=None,
-        help="download built tarball for spec (from json or yaml file) from mirror",
+    download_spec_or_specfile.add_argument(
+        "--spec-file", help="download built tarball for spec (from json or yaml file) from mirror"
     )
     download.add_argument(
-        "-p", "--path", default=None, help="path to directory where tarball should be downloaded"
+        "-p",
+        "--path",
+        required=True,
+        default=None,
+        help="path to directory where tarball should be downloaded",
     )
     download.set_defaults(func=download_fn)
 
@@ -190,32 +191,32 @@ def setup_parser(subparser):
     getbuildcachename = subparsers.add_parser(
         "get-buildcache-name", help=get_buildcache_name_fn.__doc__
     )
-    getbuildcachename.add_argument(
-        "-s", "--spec", default=None, help="spec string for which buildcache name is desired"
+    getbuildcachename_spec_or_specfile = getbuildcachename.add_mutually_exclusive_group(
+        required=True
     )
-    getbuildcachename.add_argument(
-        "--spec-file",
-        default=None,
-        help="path to spec json or yaml file for which buildcache name is desired",
+    getbuildcachename_spec_or_specfile.add_argument(
+        "-s", "--spec", help="spec string for which buildcache name is desired"
+    )
+    getbuildcachename_spec_or_specfile.add_argument(
+        "--spec-file", help="path to spec json or yaml file for which buildcache name is desired"
     )
     getbuildcachename.set_defaults(func=get_buildcache_name_fn)
 
     # Given the root spec, save the yaml of the dependent spec to a file
     savespecfile = subparsers.add_parser("save-specfile", help=save_specfile_fn.__doc__)
-    savespecfile.add_argument("--root-spec", default=None, help="root spec of dependent spec")
-    savespecfile.add_argument(
-        "--root-specfile",
-        default=None,
-        help="path to json or yaml file containing root spec of dependent spec",
+    savespecfile_spec_or_specfile = savespecfile.add_mutually_exclusive_group(required=True)
+    savespecfile_spec_or_specfile.add_argument("--root-spec", help="root spec of dependent spec")
+    savespecfile_spec_or_specfile.add_argument(
+        "--root-specfile", help="path to json or yaml file containing root spec of dependent spec"
     )
     savespecfile.add_argument(
         "-s",
         "--specs",
-        default=None,
+        required=True,
         help="list of dependent specs for which saved yaml is desired",
     )
     savespecfile.add_argument(
-        "--specfile-dir", default=None, help="path to directory where spec yamls should be saved"
+        "--specfile-dir", required=True, help="path to directory where spec yamls should be saved"
     )
     savespecfile.set_defaults(func=save_specfile_fn)
 
@@ -257,54 +258,24 @@ def setup_parser(subparser):
     update_index.set_defaults(func=update_index_fn)
 
 
-def _matching_specs(specs, spec_file):
-    """Return a list of matching specs read from either a spec file (JSON or YAML),
-    a query over the store or a query over the active environment.
-    """
-    env = ev.active_environment()
-    hashes = env.all_hashes() if env else None
-    if spec_file:
-        return spack.store.specfile_matches(spec_file, hashes=hashes)
-
-    if specs:
-        constraints = spack.cmd.parse_specs(specs)
-        return spack.store.find(constraints, hashes=hashes)
-
-    if env:
-        return [concrete for _, concrete in env.concretized_specs()]
-
-    tty.die(
-        "build cache file creation requires at least one"
-        " installed package spec, an active environment,"
-        " or else a path to a json or yaml file containing a spec"
-        " to install"
-    )
-
-
-def _concrete_spec_from_args(args):
-    spec_str, specfile_path = args.spec, args.spec_file
-
-    if not spec_str and not specfile_path:
-        tty.error("must provide either spec string or path to YAML or JSON specfile")
-        sys.exit(1)
-
-    if spec_str:
-        try:
-            constraints = spack.cmd.parse_specs(spec_str)
-            spec = spack.store.find(constraints)[0]
-            spec.concretize()
-        except SpecError as spec_error:
-            tty.error("Unable to concretize spec {0}".format(spec_str))
-            tty.debug(spec_error)
-            sys.exit(1)
-
-        return spec
-
-    return Spec.from_specfile(specfile_path)
+def _matching_specs(specs: List[Spec]) -> List[Spec]:
+    """Disambiguate specs and return a list of matching specs"""
+    return [spack.cmd.disambiguate_spec(s, ev.active_environment(), installed=any) for s in specs]
 
 
 def push_fn(args):
     """create a binary package and push it to a mirror"""
+    if args.spec_file:
+        tty.warn(
+            "The flag `--spec-file` is deprecated and will be removed in Spack 0.22. "
+            "Use positional arguments instead."
+        )
+
+    if args.specs or args.spec_file:
+        specs = _matching_specs(spack.cmd.parse_specs(args.specs or args.spec_file))
+    else:
+        specs = spack.cmd.require_active_env("buildcache push").all_specs()
+
     mirror = arguments.mirror_name_or_url(args.mirror)
 
     if args.allow_root:
@@ -315,7 +286,7 @@ def push_fn(args):
     url = mirror.push_url
 
     specs = bindist.specs_to_be_packaged(
-        _matching_specs(args.specs, args.spec_file),
+        specs,
         root="package" in args.things_to_install,
         dependencies="dependencies" in args.things_to_install,
     )
@@ -423,16 +394,21 @@ def preview_fn(args):
 def check_fn(args):
     """check specs against remote binary mirror(s) to see if any need to be rebuilt
 
-    either a single spec from --spec, or else the full set of release specs. this command uses the
-    process exit code to indicate its result, specifically, if the exit code is non-zero, then at
-    least one of the indicated specs needs to be rebuilt
+    this command uses the process exit code to indicate its result, specifically, if the
+    exit code is non-zero, then at least one of the indicated specs needs to be rebuilt
     """
-    if args.spec or args.spec_file:
-        specs = [_concrete_spec_from_args(args)]
+    if args.spec_file:
+        tty.warn(
+            "The flag `--spec-file` is deprecated and will be removed in Spack 0.22. "
+            "Use --spec instead."
+        )
+
+    specs = spack.cmd.parse_specs(args.spec or args.spec_file)
+
+    if specs:
+        specs = _matching_specs(specs, specs)
     else:
-        env = spack.cmd.require_active_env(cmd_name="buildcache")
-        env.concretize()
-        specs = env.all_specs()
+        specs = spack.cmd.require_active_env("buildcache check").all_specs()
 
     if not specs:
         tty.msg("No specs provided, exiting.")
@@ -462,26 +438,28 @@ def download_fn(args):
     code indicates that the command failed to download at least one of the required buildcache
     components
     """
-    if not args.spec and not args.spec_file:
-        tty.msg("No specs provided, exiting.")
-        return
+    if args.spec_file:
+        tty.warn(
+            "The flag `--spec-file` is deprecated and will be removed in Spack 0.22. "
+            "Use --spec instead."
+        )
 
-    if not args.path:
-        tty.msg("No download path provided, exiting")
-        return
+    specs = _matching_specs(spack.cmd.parse_specs(args.spec or args.spec_file))
 
-    spec = _concrete_spec_from_args(args)
-    result = bindist.download_single_spec(spec, args.path)
+    if len(specs) != 1:
+        tty.die("a single spec argument is required to download from a buildcache")
 
-    if not result:
+    if not bindist.download_single_spec(specs[0], args.path):
         sys.exit(1)
 
 
 def get_buildcache_name_fn(args):
     """get name (prefix) of buildcache entries for this spec"""
-    spec = _concrete_spec_from_args(args)
-    buildcache_name = bindist.tarball_name(spec, "")
-    print("{0}".format(buildcache_name))
+    tty.warn("This command is deprecated and will be removed in Spack 0.22.")
+    specs = _matching_specs(spack.cmd.parse_specs(args.spec or args.spec_file))
+    if len(specs) != 1:
+        tty.die("a single spec argument is required to get buildcache name")
+    print(bindist.tarball_name(specs[0], ""))
 
 
 def save_specfile_fn(args):
@@ -491,29 +469,24 @@ def save_specfile_fn(args):
     successful. if any errors or exceptions are encountered, or if expected command-line arguments
     are not provided, then the exit code will be non-zero
     """
-    if not args.root_spec and not args.root_specfile:
-        tty.msg("No root spec provided, exiting.")
-        sys.exit(1)
+    if args.root_specfile:
+        tty.warn(
+            "The flag `--root-specfile` is deprecated and will be removed in Spack 0.22. "
+            "Use --root-spec instead."
+        )
 
-    if not args.specs:
-        tty.msg("No dependent specs provided, exiting.")
-        sys.exit(1)
+    specs = spack.cmd.parse_specs(args.root_spec or args.root_specfile)
 
-    if not args.specfile_dir:
-        tty.msg("No yaml directory provided, exiting.")
-        sys.exit(1)
+    if len(specs) != 1:
+        tty.die("a single spec argument is required to save specfile")
+
+    root = specs[0]
+
+    if not root.concrete:
+        root.concretize()
 
-    if args.root_specfile:
-        with open(args.root_specfile) as fd:
-            root_spec_as_json = fd.read()
-        spec_format = "yaml" if args.root_specfile.endswith("yaml") else "json"
-    else:
-        root_spec = Spec(args.root_spec)
-        root_spec.concretize()
-        root_spec_as_json = root_spec.to_json(hash=ht.dag_hash)
-        spec_format = "json"
     save_dependency_specfiles(
-        root_spec_as_json, args.specfile_dir, args.specs.split(), spec_format
+        root, args.specfile_dir, dependencies=spack.cmd.parse_specs(args.specs)
     )
 
 
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index feb3747770..92d656f082 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -16,7 +16,7 @@ import time
 import urllib.parse
 import urllib.request
 import warnings
-from typing import Any, Dict, List, Optional, Set, Tuple, Union
+from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Union
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
@@ -1921,16 +1921,17 @@ class Environment:
                             "Could not install log links for {0}: {1}".format(spec.name, str(e))
                         )
 
-    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]
-        specs = [s for s in traverse.traverse_nodes(roots, key=traverse.by_dag_hash)]
-        specs.sort()
-        return specs
+    def all_specs_generator(self) -> Iterable[Spec]:
+        """Returns a generator for all concrete specs"""
+        return traverse.traverse_nodes(self.concrete_roots(), key=traverse.by_dag_hash)
+
+    def all_specs(self) -> List[Spec]:
+        """Returns a list of all concrete specs"""
+        return list(self.all_specs_generator())
 
     def all_hashes(self):
         """Return hashes of all specs."""
-        return [s.dag_hash() for s in self.all_specs()]
+        return [s.dag_hash() for s in self.all_specs_generator()]
 
     def roots(self):
         """Specs explicitly requested by the user *in this environment*.
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 6238bb8c65..2cfc3c759a 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -56,7 +56,7 @@ import itertools
 import os
 import re
 import warnings
-from typing import Tuple, Union
+from typing import List, Tuple, Union
 
 import llnl.util.filesystem as fs
 import llnl.util.lang as lang
@@ -5147,9 +5147,7 @@ class LazySpecCache(collections.defaultdict):
         return value
 
 
-def save_dependency_specfiles(
-    root_spec_info, output_directory, dependencies=None, spec_format="json"
-):
+def save_dependency_specfiles(root: Spec, output_directory: str, dependencies: List[Spec]):
     """Given a root spec (represented as a yaml object), index it with a subset
     of its dependencies, and write each dependency to a separate yaml file
     in the output directory.  By default, all dependencies will be written
@@ -5158,26 +5156,15 @@ def save_dependency_specfiles(
     incoming spec is not json, that can be specified with the spec_format
     parameter. This can be used to convert from yaml specfiles to the
     json format."""
-    if spec_format == "json":
-        root_spec = Spec.from_json(root_spec_info)
-    elif spec_format == "yaml":
-        root_spec = Spec.from_yaml(root_spec_info)
-    else:
-        raise SpecParseError("Unrecognized spec format {0}.".format(spec_format))
 
-    dep_list = dependencies
-    if not dep_list:
-        dep_list = [dep.name for dep in root_spec.traverse()]
+    for spec in root.traverse():
+        if not any(spec.satisfies(dep) for dep in dependencies):
+            continue
 
-    for dep_name in dep_list:
-        if dep_name not in root_spec:
-            msg = "Dependency {0} does not exist in root spec {1}".format(dep_name, root_spec.name)
-            raise SpecDependencyNotFoundError(msg)
-        dep_spec = root_spec[dep_name]
-        json_path = os.path.join(output_directory, "{0}.json".format(dep_name))
+        json_path = os.path.join(output_directory, f"{spec.name}.json")
 
         with open(json_path, "w") as fd:
-            fd.write(dep_spec.to_json(hash=ht.dag_hash))
+            fd.write(spec.to_json(hash=ht.dag_hash))
 
 
 class SpecParseError(spack.error.SpecError):
@@ -5398,11 +5385,6 @@ class ConflictsInSpecError(spack.error.SpecError, RuntimeError):
         super().__init__(message, long_message)
 
 
-class SpecDependencyNotFoundError(spack.error.SpecError):
-    """Raised when a failure is encountered writing the dependencies of
-    a spec."""
-
-
 class SpecDeprecatedError(spack.error.SpecError):
     """Raised when a spec concretizes to a deprecated spec or dependency."""
 
diff --git a/lib/spack/spack/test/spec_yaml.py b/lib/spack/spack/test/spec_yaml.py
index 594072ac4f..57487acf64 100644
--- a/lib/spack/spack/test/spec_yaml.py
+++ b/lib/spack/spack/test/spec_yaml.py
@@ -326,9 +326,8 @@ def test_save_dependency_spec_jsons_subset(tmpdir, config):
         spec_a = Spec("a").concretized()
         b_spec = spec_a["b"]
         c_spec = spec_a["c"]
-        spec_a_json = spec_a.to_json()
 
-        save_dependency_specfiles(spec_a_json, output_path, ["b", "c"])
+        save_dependency_specfiles(spec_a, output_path, [Spec("b"), Spec("c")])
 
         assert check_specs_equal(b_spec, os.path.join(output_path, "b.json"))
         assert check_specs_equal(c_spec, os.path.join(output_path, "c.json"))