commands: refactor --reuse handling to use config

`--reuse` was previously handled individually by each command that
needed it. We are growing more concretization options, and they'll
need their own section for commands that support them.

Now there are two concretization options:

* `--reuse`: Attempt to reuse packages from installs and buildcaches.
* `--fresh`: Opposite of reuse -- traditional spack install.

To handle thes, this PR adds a `ConfigSetAction` for `argparse`, so
that you can write argparse code like this:

```
     subgroup.add_argument(
        '--reuse', action=ConfigSetAction, dest="concretizer:reuse",
        const=True, default=None,
        help='reuse installed dependencies/buildcaches when possible'
     )
```

With this, you don't need to add logic to pull the argument out and
handle it; the `ConfigSetAction` just does it for you. This can probably
be used to clean up some other commands later, as well.

Code that was previously passing `reuse=True` around everywhere has
been refactored to use config, and config is set from the CLI using
a new `add_concretizer_args()` function in `spack.cmd.common.arguments`.

- [x] Add `ConfigSetAction` to simplify concretizer config on the CLI
- [x] Refactor code so that it does not pass `reuse=True` to every function.
- [x] Refactor commands to use `add_concretizer_args()` and to pass
      concretizer config using the config system.

lib/spack/spack/cmd/__init__.py

@@ -154,7 +154,6 @@ def parse_specs(args, **kwargs):
     concretize = kwargs.get('concretize', False)
     normalize = kwargs.get('normalize', False)
     tests = kwargs.get('tests', False)
-    reuse = kwargs.get('reuse', False)
 
     try:
         sargs = args
@@ -163,7 +162,7 @@ def parse_specs(args, **kwargs):
         specs = spack.spec.parse(sargs)
         for spec in specs:
             if concretize:
-                spec.concretize(tests=tests, reuse=reuse)  # implies normalize
+                spec.concretize(tests=tests)  # implies normalize
             elif normalize:
                 spec.normalize(tests=tests)
 

lib/spack/spack/cmd/common/arguments.py

@@ -322,11 +322,68 @@ def add_cdash_args(subparser, add_help):
     )
 
 
-@arg
+class ConfigSetAction(argparse.Action):
-def reuse():
+    """Generic action for setting spack config options from CLI.
-    return Args(
+
-        '--reuse', action='store_true', default=False,
+    This works like a ``store_const`` action but you can set the
-        help='reuse installed dependencies'
+    ``dest`` to some Spack configuration path (like ``concretizer:reuse``)
+    and the ``const`` will be stored there using ``spack.config.set()``
+    """
+    def __init__(self,
+                 option_strings,
+                 dest,
+                 const,
+                 default=None,
+                 required=False,
+                 help=None,
+                 metavar=None):
+        # save the config option we're supposed to set
+        self.config_path = dest
+
+        # destination is translated to a legal python identifier by
+        # substituting '_' for ':'.
+        dest = dest.replace(":", "_")
+
+        super(ConfigSetAction, self).__init__(
+            option_strings=option_strings,
+            dest=dest,
+            nargs=0,
+            const=const,
+            default=default,
+            required=required,
+            help=help
+        )
+
+    def __call__(self, parser, namespace, values, option_string):
+        # Retrieve the name of the config option and set it to
+        # the const from the constructor or a value from the CLI.
+        # Note that this is only called if the argument is actually
+        # specified on the command line.
+        spack.config.set(self.config_path, self.const, scope="command_line")
+
+
+def add_concretizer_args(subparser):
+    """Add a subgroup of arguments for controlling concretization.
+
+    These will appear in a separate group called 'concretizer arguments'.
+    There's no need to handle them in your command logic -- they all use
+    ``ConfigSetAction``, which automatically handles setting configuration
+    options.
+
+    If you *do* need to access a value passed on the command line, you can
+    get at, e.g., the ``concretizer:reuse`` via ``args.concretizer_reuse``.
+    Just substitute ``_`` for ``:``.
+    """
+    subgroup = subparser.add_argument_group("concretizer arguments")
+    subgroup.add_argument(
+        '-U', '--fresh', action=ConfigSetAction, dest="concretizer:reuse",
+        const=False, default=None,
+        help='do not reuse installed deps; build newest configuration'
+    )
+    subgroup.add_argument(
+        '--reuse', action=ConfigSetAction, dest="concretizer:reuse",
+        const=True, default=None,
+        help='reuse installed dependencies/buildcaches when possible'
     )
 
 

lib/spack/spack/cmd/concretize.py

@@ -13,7 +13,6 @@
 
 
 def setup_parser(subparser):
-    spack.cmd.common.arguments.add_common_arguments(subparser, ['reuse'])
     subparser.add_argument(
         '-f', '--force', action='store_true',
         help="Re-concretize even if already concretized.")
@@ -24,6 +23,8 @@ def setup_parser(subparser):
 dependencies are only added for the environment's root specs. When 'all' is
 chosen, test dependencies are enabled for all packages in the environment.""")
 
+    spack.cmd.common.arguments.add_concretizer_args(subparser)
+
 
 def concretize(parser, args):
     env = spack.cmd.require_active_env(cmd_name='concretize')
@@ -36,8 +37,6 @@ def concretize(parser, args):
         tests = False
 
     with env.write_transaction():
-        concretized_specs = env.concretize(
+        concretized_specs = env.concretize(force=args.force, tests=tests)
-            force=args.force, tests=tests, reuse=args.reuse
-        )
         ev.display_specs(concretized_specs)
         env.write()

lib/spack/spack/cmd/dev_build.py

@@ -19,7 +19,7 @@
 
 
 def setup_parser(subparser):
-    arguments.add_common_arguments(subparser, ['jobs', 'reuse'])
+    arguments.add_common_arguments(subparser, ['jobs'])
     subparser.add_argument(
         '-d', '--source-path', dest='source_path', default=None,
         help="path to source directory. defaults to the current directory")
@@ -59,6 +59,8 @@ def setup_parser(subparser):
     cd_group = subparser.add_mutually_exclusive_group()
     arguments.add_common_arguments(cd_group, ['clean', 'dirty'])
 
+    spack.cmd.common.arguments.add_concretizer_args(subparser)
+
 
 def dev_build(self, args):
     if not args.spec:
@@ -86,7 +88,7 @@ def dev_build(self, args):
     # Forces the build to run out of the source directory.
     spec.constrain('dev_path=%s' % source_path)
 
-    spec.concretize(reuse=args.reuse)
+    spec.concretize()
     package = spack.repo.get(spec)
 
     if package.installed:

lib/spack/spack/cmd/install.py

@@ -78,7 +78,7 @@ def setup_parser(subparser):
     subparser.add_argument(
         '-u', '--until', type=str, dest='until', default=None,
         help="phase to stop after when installing (default None)")
-    arguments.add_common_arguments(subparser, ['jobs', 'reuse'])
+    arguments.add_common_arguments(subparser, ['jobs'])
     subparser.add_argument(
         '--overwrite', action='store_true',
         help="reinstall an existing spec, even if it has dependents")
@@ -182,6 +182,8 @@ def setup_parser(subparser):
     arguments.add_cdash_args(subparser, False)
     arguments.add_common_arguments(subparser, ['yes_to_all', 'spec'])
 
+    spack.cmd.common.arguments.add_concretizer_args(subparser)
+
 
 def default_log_file(spec):
     """Computes the default filename for the log file and creates
@@ -339,7 +341,7 @@ def get_tests(specs):
 
             if not args.only_concrete:
                 with env.write_transaction():
-                    concretized_specs = env.concretize(tests=tests, reuse=args.reuse)
+                    concretized_specs = env.concretize(tests=tests)
                     ev.display_specs(concretized_specs)
 
                     # save view regeneration for later, so that we only do it
@@ -397,9 +399,7 @@ def get_tests(specs):
     kwargs['tests'] = tests
 
     try:
-        specs = spack.cmd.parse_specs(
+        specs = spack.cmd.parse_specs(args.spec, concretize=True, tests=tests)
-            args.spec, concretize=True, tests=tests, reuse=args.reuse
-        )
     except SpackError as e:
         tty.debug(e)
         reporter.concretization_report(e.message)

lib/spack/spack/cmd/solve.py

@@ -44,7 +44,7 @@ def setup_parser(subparser):
 
     # Below are arguments w.r.t. spec display (like spack spec)
     arguments.add_common_arguments(
-        subparser, ['long', 'very_long', 'install_status', 'reuse']
+        subparser, ['long', 'very_long', 'install_status']
     )
     subparser.add_argument(
         '-y', '--yaml', action='store_const', dest='format', default=None,
@@ -71,6 +71,8 @@ def setup_parser(subparser):
     subparser.add_argument(
         'specs', nargs=argparse.REMAINDER, help="specs of packages")
 
+    spack.cmd.common.arguments.add_concretizer_args(subparser)
+
 
 def solve(parser, args):
     # these are the same options as `spack spec`
@@ -104,7 +106,6 @@ def solve(parser, args):
 
     # set up solver parameters
     solver = asp.Solver()
-    solver.reuse = args.reuse
     solver.dump = dump
     solver.models = models
     solver.timers = args.timers

lib/spack/spack/cmd/spec.py

@@ -32,7 +32,7 @@ def setup_parser(subparser):
     spack help --spec
 """
     arguments.add_common_arguments(
-        subparser, ['long', 'very_long', 'install_status', 'reuse']
+        subparser, ['long', 'very_long', 'install_status']
     )
     subparser.add_argument(
         '-y', '--yaml', action='store_const', dest='format', default=None,
@@ -56,6 +56,8 @@ def setup_parser(subparser):
         help='show dependency types')
     arguments.add_common_arguments(subparser, ['specs'])
 
+    spack.cmd.common.arguments.add_concretizer_args(subparser)
+
 
 @contextlib.contextmanager
 def nullcontext():
@@ -83,21 +85,17 @@ def spec(parser, args):
     if args.install_status:
         tree_context = spack.store.db.read_transaction
 
-    concretize_kwargs = {
-        'reuse': args.reuse
-    }
-
     # Use command line specified specs, otherwise try to use environment specs.
     if args.specs:
         input_specs = spack.cmd.parse_specs(args.specs)
-        specs = [(s, s.concretized(**concretize_kwargs)) for s in input_specs]
+        specs = [(s, s.concretized()) for s in input_specs]
     else:
         env = ev.active_environment()
         if env:
-            env.concretize(**concretize_kwargs)
+            env.concretize()
             specs = env.concretized_specs()
         else:
-            tty.die("spack spec requires at least one spec or an active environmnt")
+            tty.die("spack spec requires at least one spec or an active environment")
 
     for (input, output) in specs:
         # With -y, just print YAML to output.

lib/spack/spack/environment/environment.py

@@ -1083,7 +1083,7 @@ def is_develop(self, spec):
         """Returns true when the spec is built from local sources"""
         return spec.name in self.dev_specs
 
-    def concretize(self, force=False, tests=False, reuse=False):
+    def concretize(self, force=False, tests=False, reuse=None):
         """Concretize user_specs in this environment.
 
         Only concretizes specs that haven't been concretized yet unless
@@ -1120,7 +1120,7 @@ def concretize(self, force=False, tests=False, reuse=False):
         msg = 'concretization strategy not implemented [{0}]'
         raise SpackEnvironmentError(msg.format(self.concretization))
 
-    def _concretize_together(self, tests=False, reuse=False):
+    def _concretize_together(self, tests=False, reuse=None):
         """Concretization strategy that concretizes all the specs
         in the same DAG.
         """
@@ -1160,7 +1160,7 @@ def _concretize_together(self, tests=False, reuse=False):
             self._add_concrete_spec(abstract, concrete)
         return concretized_specs
 
-    def _concretize_separately(self, tests=False, reuse=False):
+    def _concretize_separately(self, tests=False, reuse=None):
         """Concretization strategy that concretizes separately one
         user spec after the other.
         """
@@ -2009,7 +2009,7 @@ def _tree_to_display(spec):
         print('')
 
 
-def _concretize_from_constraints(spec_constraints, tests=False, reuse=False):
+def _concretize_from_constraints(spec_constraints, tests=False, reuse=None):
     # Accept only valid constraints from list and concretize spec
     # Get the named spec even if out of order
     root_spec = [s for s in spec_constraints if s.name]

lib/spack/spack/spec.py

@@ -2605,7 +2605,7 @@ def ensure_no_deprecated(root):
             msg += "    For each package listed, choose another spec\n"
             raise SpecDeprecatedError(msg)
 
-    def _new_concretize(self, tests=False, reuse=False):
+    def _new_concretize(self, tests=False, reuse=None):
         import spack.solver.asp
 
         if not self.name:
@@ -2637,7 +2637,7 @@ def _new_concretize(self, tests=False, reuse=False):
         self._dup(concretized)
         self._mark_concrete()
 
-    def concretize(self, tests=False, reuse=False):
+    def concretize(self, tests=False, reuse=None):
         """Concretize the current spec.
 
         Args:
@@ -2678,7 +2678,7 @@ def _mark_concrete(self, value=True):
                 s.clear_cached_hashes()
             s._mark_root_concrete(value)
 
-    def concretized(self, tests=False, reuse=False):
+    def concretized(self, tests=False, reuse=None):
         """This is a non-destructive version of concretize().
 
         First clones, then returns a concrete version of this package

lib/spack/spack/test/cmd/common/arguments.py

@@ -11,6 +11,7 @@
 import spack.cmd.common.arguments as arguments
 import spack.config
 import spack.environment as ev
+import spack.main
 
 
 @pytest.fixture()
@@ -112,3 +113,18 @@ def test_root_and_dep_match_returns_root(mock_packages, mutable_mock_env_path):
         env_spec2 = spack.cmd.matching_spec_from_env(
             spack.cmd.parse_specs(['b@1.0'])[0])
         assert env_spec2
+
+
+def test_concretizer_arguments(mutable_config, mock_packages):
+    """Ensure that ConfigSetAction is doing the right thing."""
+    spec = spack.main.SpackCommand("spec")
+
+    assert spack.config.get("concretizer:reuse", None) is None
+
+    spec("--reuse", "zlib")
+
+    assert spack.config.get("concretizer:reuse", None) is True
+
+    spec("--fresh", "zlib")
+
+    assert spack.config.get("concretizer:reuse", None) is False

lib/spack/spack/test/cmd/spec.py

@@ -9,6 +9,7 @@
 
 import spack.environment as ev
 import spack.spec
+import spack.store
 from spack.main import SpackCommand, SpackCommandError
 
 pytestmark = pytest.mark.usefixtures('config', 'mutable_mock_repo')
@@ -27,6 +28,33 @@ def test_spec():
     assert 'mpich@3.0.4' in output
 
 
+def test_spec_concretizer_args(mutable_config, mutable_database):
+    """End-to-end test of CLI concretizer prefs.
+
+    It's here to make sure that everything works from CLI
+    options to `solver.py`, and that config options are not
+    lost along the way.
+    """
+    if spack.config.get('config:concretizer') == 'original':
+        pytest.xfail('Known failure of the original concretizer')
+
+    # remove two non-preferred mpileaks installations
+    # so that reuse will pick up the zmpi one
+    uninstall = SpackCommand("uninstall")
+    uninstall("-y", "mpileaks^mpich")
+    uninstall("-y", "mpileaks^mpich2")
+
+    # get the hash of mpileaks^zmpi
+    mpileaks_zmpi = spack.store.db.query_one("mpileaks^zmpi")
+    h = mpileaks_zmpi.dag_hash()[:7]
+
+    output = spec("--fresh", "-l", "mpileaks")
+    assert h not in output
+
+    output = spec("--reuse", "-l", "mpileaks")
+    assert h in output
+
+
 def test_spec_yaml():
     output = spec('--yaml', 'mpileaks')
 

share/spack/spack-completion.bash

@@ -709,7 +709,7 @@ _spack_compilers() {
 }
 
 _spack_concretize() {
-    SPACK_COMPREPLY="-h --help --reuse -f --force --test"
+    SPACK_COMPREPLY="-h --help -f --force --test -U --fresh --reuse"
 }
 
 _spack_config() {
@@ -870,7 +870,7 @@ _spack_deprecate() {
 _spack_dev_build() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -j --jobs --reuse -d --source-path -i --ignore-dependencies -n --no-checksum --deprecated --keep-prefix --skip-patch -q --quiet --drop-in --test -b --before -u --until --clean --dirty"
+        SPACK_COMPREPLY="-h --help -j --jobs -d --source-path -i --ignore-dependencies -n --no-checksum --deprecated --keep-prefix --skip-patch -q --quiet --drop-in --test -b --before -u --until --clean --dirty -U --fresh --reuse"
     else
         _all_packages
     fi
@@ -1166,7 +1166,7 @@ _spack_info() {
 _spack_install() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --reuse --overwrite --fail-fast --keep-prefix --keep-stage --dont-restage --use-cache --no-cache --cache-only --monitor --monitor-save-local --monitor-tags --monitor-keep-going --monitor-host --monitor-prefix --include-build-deps --no-check-signature --require-full-hash-match --show-log-on-error --source -n --no-checksum --deprecated -v --verbose --fake --only-concrete --no-add -f --file --clean --dirty --test --run-tests --log-format --log-file --help-cdash --cdash-upload-url --cdash-build --cdash-site --cdash-track --cdash-buildstamp -y --yes-to-all"
+        SPACK_COMPREPLY="-h --help --only -u --until -j --jobs --overwrite --fail-fast --keep-prefix --keep-stage --dont-restage --use-cache --no-cache --cache-only --monitor --monitor-save-local --monitor-tags --monitor-keep-going --monitor-host --monitor-prefix --include-build-deps --no-check-signature --require-full-hash-match --show-log-on-error --source -n --no-checksum --deprecated -v --verbose --fake --only-concrete --no-add -f --file --clean --dirty --test --run-tests --log-format --log-file --help-cdash --cdash-upload-url --cdash-build --cdash-site --cdash-track --cdash-buildstamp -y --yes-to-all -U --fresh --reuse"
     else
         _all_packages
     fi
@@ -1652,7 +1652,7 @@ _spack_restage() {
 _spack_solve() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help --show --models -l --long -L --very-long -I --install-status --reuse -y --yaml -j --json -c --cover -N --namespaces -t --types --timers --stats"
+        SPACK_COMPREPLY="-h --help --show --models -l --long -L --very-long -I --install-status -y --yaml -j --json -c --cover -N --namespaces -t --types --timers --stats -U --fresh --reuse"
     else
         _all_packages
     fi
@@ -1661,7 +1661,7 @@ _spack_solve() {
 _spack_spec() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -l --long -L --very-long -I --install-status --reuse -y --yaml -j --json -c --cover -N --namespaces --hash-type -t --types"
+        SPACK_COMPREPLY="-h --help -l --long -L --very-long -I --install-status -y --yaml -j --json -c --cover -N --namespaces --hash-type -t --types -U --fresh --reuse"
     else
         _all_packages
     fi