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.

12 files changed, 140 insertions, 40 deletions

diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index 96cbafac89..9568ccc296 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/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)
 
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 9286701c18..bcfc2866c1 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -322,11 +322,68 @@ the build yourself.  Format: %%Y%%m%%d-%%H%%M-[cdash-track]"""
     )
 
 
-@arg
-def reuse():
-    return Args(
-        '--reuse', action='store_true', default=False,
-        help='reuse installed dependencies'
+class ConfigSetAction(argparse.Action):
+    """Generic action for setting spack config options from CLI.
+
+    This works like a ``store_const`` action but you can set the
+    ``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'
     )
 
 
diff --git a/lib/spack/spack/cmd/concretize.py b/lib/spack/spack/cmd/concretize.py
index f05febecd8..09486a3db8 100644
--- a/lib/spack/spack/cmd/concretize.py
+++ b/lib/spack/spack/cmd/concretize.py
@@ -13,7 +13,6 @@ level = "long"
 
 
 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(
-            force=args.force, tests=tests, reuse=args.reuse
-        )
+        concretized_specs = env.concretize(force=args.force, tests=tests)
         ev.display_specs(concretized_specs)
         env.write()
diff --git a/lib/spack/spack/cmd/dev_build.py b/lib/spack/spack/cmd/dev_build.py
index fba06c31e1..0d4a440306 100644
--- a/lib/spack/spack/cmd/dev_build.py
+++ b/lib/spack/spack/cmd/dev_build.py
@@ -19,7 +19,7 @@ level = "long"
 
 
 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 @@ packages. If neither are chosen, don't run tests for any packages.""")
     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:
diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py
index ac8bd0fdb5..e95f118956 100644
--- a/lib/spack/spack/cmd/install.py
+++ b/lib/spack/spack/cmd/install.py
@@ -78,7 +78,7 @@ the dependencies"""
     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 @@ 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)
+
 
 def default_log_file(spec):
     """Computes the default filename for the log file and creates
@@ -339,7 +341,7 @@ environment variables:
 
             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 @@ environment variables:
     kwargs['tests'] = tests
 
     try:
-        specs = spack.cmd.parse_specs(
-            args.spec, concretize=True, tests=tests, reuse=args.reuse
-        )
+        specs = spack.cmd.parse_specs(args.spec, concretize=True, tests=tests)
     except SpackError as e:
         tty.debug(e)
         reporter.concretization_report(e.message)
diff --git a/lib/spack/spack/cmd/solve.py b/lib/spack/spack/cmd/solve.py
index b3207d7070..81d6ed5268 100644
--- a/lib/spack/spack/cmd/solve.py
+++ b/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
diff --git a/lib/spack/spack/cmd/spec.py b/lib/spack/spack/cmd/spec.py
index 2b1ba62571..d15d63bb82 100644
--- a/lib/spack/spack/cmd/spec.py
+++ b/lib/spack/spack/cmd/spec.py
@@ -32,7 +32,7 @@ for further documentation regarding the spec syntax, see:
     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 @@ for further documentation regarding the spec syntax, see:
         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.
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 2364dd6ad9..55c8af97f7 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -1083,7 +1083,7 @@ class Environment(object):
         """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 @@ class Environment(object):
         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 @@ class Environment(object):
             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 display_specs(concretized_specs):
         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]
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 4011bd10a7..7a918cc0c5 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2605,7 +2605,7 @@ class Spec(object):
             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 @@ class Spec(object):
         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 @@ class Spec(object):
                 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
diff --git a/lib/spack/spack/test/cmd/common/arguments.py b/lib/spack/spack/test/cmd/common/arguments.py
index fe587fdfdb..5f1299a94a 100644
--- a/lib/spack/spack/test/cmd/common/arguments.py
+++ b/lib/spack/spack/test/cmd/common/arguments.py
@@ -11,6 +11,7 @@ import spack.cmd
 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
diff --git a/lib/spack/spack/test/cmd/spec.py b/lib/spack/spack/test/cmd/spec.py
index 89e4efae5c..74be6f4ca0 100644
--- a/lib/spack/spack/test/cmd/spec.py
+++ b/lib/spack/spack/test/cmd/spec.py
@@ -9,6 +9,7 @@ import pytest
 
 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')
 
diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
index a5b3dcb33e..fffc7e4935 100755
--- a/share/spack/spack-completion.bash
+++ b/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