concretizer: make --force config and a common argument (spack#48838)

`--force` was previously available only on `spack concretize` but not on `spack spec`,
`spack solve`, and other commands that do concretization.

This means you can now preview a force-concretize on an environment or spec with
`spack spec -f` or `spack solve -f`.  You can also set concretization to *always* force
in config with:

```yaml
spack:
    concretizer:
        force: true
```

Making room for this required two breaking changes:
1. `spack install --file` no longer works. The `-f` / `--file` option conflicted
    with `--force`. If you wrote:

    ```
    spack install -f ./spec.json`
    ```

    you should simply remove the flag:

    ```
    spack install ./spec.json`
    ```

    Files as spec arguments have been supported since at least Spack `v0.10.0`.
    We did not preserve `--file` because it took a separate code path that was already
    buggy. Using one code path for spec files will allow us to handle them more reliably.

2. `spack mirror create --file` no longer has a short `-f` option 

    If you wrote:

    ```
    spack mirror create -f FILE
    ```

    You should now use the long argument:

    ```
    spack mirror create --file FILE
    ```

    We removed the `-f` option to make room for `-f / --force`.

- [x] make `concretizer:force` a configuration option
- [x] add `--force` to common concretizer arguments
- [x] remove the `-f` short option from `spack mirror create --file`
- [x] remove the `-f` / `--file` option from `spack install`

---------

Signed-off-by: Todd Gamblin <[email protected]>

Author: tgamblin

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

@@ -175,20 +175,17 @@ def _cdash_reporter(namespace):
 
     def _factory():
         def installed_specs(args):
+            packages = []
+
             if getattr(args, "spec", ""):
                 packages = args.spec
             elif getattr(args, "specs", ""):
                 packages = args.specs
             elif getattr(args, "package", ""):
                 # Ensure CI 'spack test run' can output CDash results
                 packages = args.package
-            else:
-                packages = []
-                for file in args.specfiles:
-                    with open(file, "r", encoding="utf-8") as f:
-                        s = spack.spec.Spec.from_yaml(f)
-                        packages.append(s.format())
-            return packages
+
+            return [str(spack.spec.Spec(s)) for s in packages]
 
         configuration = spack.reporters.CDashConfiguration(
             upload_url=namespace.cdash_upload_url,
@@ -198,6 +195,7 @@ def installed_specs(args):
             buildstamp=namespace.cdash_buildstamp,
             track=namespace.cdash_track,
         )
+
         return spack.reporters.CDash(configuration=configuration)
 
     return _factory
@@ -533,11 +531,22 @@ class ConfigSetAction(argparse.Action):
     """
 
     def __init__(
-        self, option_strings, dest, const, default=None, required=False, help=None, metavar=None
+        self,
+        option_strings,
+        dest,
+        const,
+        default=None,
+        required=False,
+        help=None,
+        metavar=None,
+        require_environment=False,
     ):
         # save the config option we're supposed to set
         self.config_path = dest
 
+        # save whether the option requires an active env
+        self.require_environment = require_environment
+
         # destination is translated to a legal python identifier by
         # substituting '_' for ':'.
         dest = dest.replace(":", "_")
@@ -553,6 +562,11 @@ def __init__(
         )
 
     def __call__(self, parser, namespace, values, option_string):
+        if self.require_environment and not ev.active_environment():
+            raise argparse.ArgumentTypeError(
+                f"argument '{self.option_strings[-1]}' requires an environment"
+            )
+
         # 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
@@ -573,6 +587,16 @@ def add_concretizer_args(subparser):
     Just substitute ``_`` for ``:``.
     """
     subgroup = subparser.add_argument_group("concretizer arguments")
+    subgroup.add_argument(
+        "-f",
+        "--force",
+        action=ConfigSetAction,
+        require_environment=True,
+        dest="concretizer:force",
+        const=True,
+        default=False,
+        help="allow changes to concretized specs in spack.lock (in an env)",
+    )
     subgroup.add_argument(
         "-U",
         "--fresh",

lib/spack/spack/cmd/concretize.py

@@ -17,9 +17,6 @@
 
 
 def setup_parser(subparser: argparse.ArgumentParser) -> None:
-    subparser.add_argument(
-        "-f", "--force", action="store_true", help="re-concretize even if already concretized"
-    )
     subparser.add_argument(
         "--test",
         default=None,
@@ -45,7 +42,7 @@ def concretize(parser, args):
         tests = False
 
     with env.write_transaction():
-        concretized_specs = env.concretize(force=args.force, tests=tests)
+        concretized_specs = env.concretize(tests=tests)
         if not args.quiet:
             if concretized_specs:
                 tty.msg(f"Concretized {plural(len(concretized_specs), 'spec')}:")

lib/spack/spack/cmd/install.py

@@ -13,7 +13,6 @@
 from llnl.util import tty
 
 import spack.cmd
-import spack.concretize
 import spack.config
 import spack.environment as ev
 import spack.paths
@@ -204,16 +203,6 @@ def setup_parser(subparser: argparse.ArgumentParser) -> None:
         help="(with environment) do not add spec to the environment as a root",
     )
 
-    subparser.add_argument(
-        "-f",
-        "--file",
-        action="append",
-        default=[],
-        dest="specfiles",
-        metavar="SPEC_YAML_FILE",
-        help="read specs to install from .yaml files",
-    )
-
     cd_group = subparser.add_mutually_exclusive_group()
     arguments.add_common_arguments(cd_group, ["clean", "dirty"])
 
@@ -334,7 +323,7 @@ def install(parser, args):
     install_kwargs = install_kwargs_from_args(args)
     env = ev.active_environment()
 
-    if not env and not args.spec and not args.specfiles:
+    if not env and not args.spec:
         _die_require_env()
 
     try:
@@ -436,28 +425,8 @@ def concrete_specs_from_cli(args, install_kwargs):
     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", encoding="utf-8") 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 = spack.concretize.concretize_one(s)
-        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):
-    concrete_specs = concrete_specs_from_cli(args, install_kwargs) + concrete_specs_from_file(args)
+    concrete_specs = concrete_specs_from_cli(args, install_kwargs)
 
     if len(concrete_specs) == 0:
         tty.die("The `spack install` command requires a spec to install.")

lib/spack/spack/cmd/mirror.py

@@ -46,7 +46,7 @@ def setup_parser(subparser: argparse.ArgumentParser) -> None:
         " in the current environment if there is an active environment"
         " (this requires significant time and space)",
     )
-    create_parser.add_argument("-f", "--file", help="file with specs of packages to put in mirror")
+    create_parser.add_argument("--file", help="file with specs of packages to put in mirror")
     create_parser.add_argument(
         "--exclude-file",
         help="specs which Spack should not try to add to a mirror"

lib/spack/spack/environment/environment.py

@@ -1422,7 +1422,9 @@ 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):
+    def concretize(
+        self, force: Optional[bool] = None, tests: Union[bool, Sequence] = False
+    ) -> Sequence[SpecPair]:
         """Concretize user_specs in this environment.
 
         Only concretizes specs that haven't been concretized yet unless
@@ -1432,15 +1434,18 @@ def concretize(self, force=False, tests=False):
         write out a lockfile containing concretized specs.
 
         Arguments:
-            force (bool): re-concretize ALL specs, even those that were
-               already concretized
-            tests (bool or list or set): False to run no tests, True to test
-                all packages, or a list of package names to run tests for some
+            force: re-concretize ALL specs, even those that were already concretized;
+                defaults to ``spack.config.get("concretizer:force")``
+            tests: False to run no tests, True to test all packages, or a list of
+                package names to run tests for some
 
         Returns:
             List of specs that have been concretized. Each entry is a tuple of
             the user spec and the corresponding concretized spec.
         """
+        if force is None:
+            force = spack.config.get("concretizer:force")
+
         if force:
             # Clear previously concretized specs
             self.concretized_user_specs = []
@@ -1524,7 +1529,9 @@ def _get_specs_to_concretize(
         ]
         return new_user_specs, kept_user_specs, specs_to_concretize
 
-    def _concretize_together_where_possible(self, tests: bool = False) -> Sequence[SpecPair]:
+    def _concretize_together_where_possible(
+        self, tests: Union[bool, Sequence] = False
+    ) -> Sequence[SpecPair]:
         # Exit early if the set of concretized specs is the set of user specs
         new_user_specs, _, specs_to_concretize = self._get_specs_to_concretize()
         if not new_user_specs:
@@ -1549,7 +1556,7 @@ def _concretize_together_where_possible(self, tests: bool = False) -> Sequence[S
 
         return ret
 
-    def _concretize_together(self, tests: bool = False) -> Sequence[SpecPair]:
+    def _concretize_together(self, tests: Union[bool, Sequence] = False) -> Sequence[SpecPair]:
         """Concretization strategy that concretizes all the specs
         in the same DAG.
         """
@@ -1590,7 +1597,7 @@ def _concretize_together(self, tests: bool = False) -> Sequence[SpecPair]:
         # Return the portion of the return value that is new
         return concretized_specs[: len(new_user_specs)]
 
-    def _concretize_separately(self, tests=False):
+    def _concretize_separately(self, tests: Union[bool, Sequence] = False):
         """Concretization strategy that concretizes separately one
         user spec after the other.
         """

lib/spack/spack/schema/concretizer.py

@@ -15,6 +15,7 @@
         "type": "object",
         "additionalProperties": False,
         "properties": {
+            "force": {"type": "boolean", "default": False},
             "reuse": {
                 "oneOf": [
                     {"type": "boolean"},

lib/spack/spack/test/cmd/ci.py

@@ -1068,13 +1068,13 @@ def test_ci_rebuild_index(
             with open(tmp_path / "spec.json", "w", encoding="utf-8") as f:
                 f.write(concrete_spec.to_json(hash=ht.dag_hash))
 
-            install_cmd("--fake", "--add", "-f", str(tmp_path / "spec.json"))
+            install_cmd("--fake", str(tmp_path / "spec.json"))
             buildcache_cmd("push", "-u", "-f", mirror_url, "callpath")
             ci_cmd("rebuild-index")
 
             with capsys.disabled():
-                output = buildcache_cmd("list", "--allarch")
-                assert "callpath" in output
+                output = buildcache_cmd("list", "-L", "--allarch")
+                assert concrete_spec.dag_hash() + " callpath" in output
 
 
 def test_ci_get_stack_changed(mock_git_repo, monkeypatch):

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

@@ -339,40 +339,6 @@ def test_install_invalid_spec():
         install("conflict%~")
 
 
-@pytest.mark.usefixtures("noop_install", "mock_packages", "config")
-@pytest.mark.parametrize(
-    "spec,concretize,error_code",
-    [
-        (Spec("mpi"), False, 1),
-        (Spec("mpi"), True, 0),
-        (Spec("boost"), False, 1),
-        (Spec("boost"), True, 0),
-    ],
-)
-def test_install_from_file(spec, concretize, error_code, tmpdir):
-    if concretize:
-        spec = spack.concretize.concretize_one(spec)
-
-    specfile = tmpdir.join("spec.yaml")
-
-    with specfile.open("w") as f:
-        spec.to_yaml(f)
-
-    err_msg = "does not contain a concrete spec" if error_code else ""
-
-    # Relative path to specfile (regression for #6906)
-    with fs.working_dir(specfile.dirname):
-        # A non-concrete spec will fail to be installed
-        out = install("-f", specfile.basename, fail_on_error=False)
-    assert install.returncode == error_code
-    assert err_msg in out
-
-    # Absolute path to specfile (regression for #6983)
-    out = install("-f", str(specfile), fail_on_error=False)
-    assert install.returncode == error_code
-    assert err_msg in out
-
-
 @pytest.mark.disable_clean_stage_check
 @pytest.mark.usefixtures("mock_packages", "mock_archive", "mock_fetch", "install_mockery")
 @pytest.mark.parametrize(
@@ -483,6 +449,11 @@ def test_junit_output_with_errors(
     assert f'error message="{msg}"' in content
 
 
+@pytest.fixture(params=["yaml", "json"])
+def spec_format(request):
+    return request.param
+
+
 @pytest.mark.usefixtures("noop_install", "mock_packages", "config")
 @pytest.mark.parametrize(
     "clispecs,filespecs",
@@ -494,15 +465,15 @@ def test_junit_output_with_errors(
         [["cmake", "libelf"], ["mpi", "boost"]],
     ],
 )
-def test_install_mix_cli_and_files(clispecs, filespecs, tmpdir):
+def test_install_mix_cli_and_files(spec_format, clispecs, filespecs, tmpdir):
     args = clispecs
 
     for spec in filespecs:
-        filepath = tmpdir.join(spec + ".yaml")
-        args = ["-f", str(filepath)] + args
+        filepath = tmpdir.join(spec + f".{spec_format}")
+        args = [str(filepath)] + args
         s = spack.concretize.concretize_one(spec)
         with filepath.open("w") as f:
-            s.to_yaml(f)
+            s.to_yaml(f) if spec_format == "yaml" else s.to_json(f)
 
     install(*args, fail_on_error=False)
     assert install.returncode == 0
@@ -638,7 +609,6 @@ def test_cdash_install_from_spec_json(
             "--cdash-build=my_custom_build",
             "--cdash-site=my_custom_site",
             "--cdash-track=my_custom_track",
-            "-f",
             spec_json_path,
         )
 
@@ -851,11 +821,11 @@ def test_install_no_add_in_env(tmpdir, mutable_mock_env_path, mock_fetch, instal
 
         # Make sure we can install a concrete dependency spec from a spec.json
         # file on disk, and the spec is installed but not added as a root
-        mpi_spec_json_path = tmpdir.join("{0}.json".format(mpi_spec.name))
+        mpi_spec_json_path = tmpdir.join(f"{mpi_spec.name}.json")
         with open(mpi_spec_json_path.strpath, "w", encoding="utf-8") as fd:
             fd.write(mpi_spec.to_json(hash=ht.dag_hash))
 
-        install("-f", mpi_spec_json_path.strpath)
+        install(mpi_spec_json_path.strpath)
         assert mpi_spec not in e.roots()
 
         find_output = find("-l", output=str)