From 233dabbd4fdcb1806e73fcedb1a12f6bdd624021 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Thu, 28 Oct 2021 10:46:51 -0700 Subject: bugfix: config edit should work with a malformed `spack.yaml` If you don't format `spack.yaml` correctly, `spack config edit` still fails and you have to edit your `spack.yaml` manually. - [x] Add some code to `_main()` to defer `ConfigFormatError` when loading the environment, until we know what command is being run. - [x] Make `spack config edit` use `SPACK_ENV` instead of the config scope object to find `spack.yaml`, so it can work even if the environment is bad. Co-authored-by: scheibelp --- lib/spack/spack/cmd/config.py | 17 +++++++--- lib/spack/spack/main.py | 24 +++++++++++--- share/spack/qa/run-unit-tests | 2 ++ share/spack/qa/test-env-cfg.sh | 73 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 106 insertions(+), 10 deletions(-) create mode 100755 share/spack/qa/test-env-cfg.sh diff --git a/lib/spack/spack/cmd/config.py b/lib/spack/spack/cmd/config.py index 2dc25353ed..0dfb40c04e 100644 --- a/lib/spack/spack/cmd/config.py +++ b/lib/spack/spack/cmd/config.py @@ -170,12 +170,19 @@ def config_edit(args): With no arguments and an active environment, edit the spack.yaml for the active environment. """ - scope, section = _get_scope_and_section(args) - if not scope and not section: - tty.die('`spack config edit` requires a section argument ' - 'or an active environment.') + spack_env = os.environ.get(ev.spack_env_var) + if spack_env and not args.scope: + # Don't use the scope object for envs, as `config edit` can be called + # for a malformed environment. Use SPACK_ENV to find spack.yaml. + config_file = ev.manifest_file(spack_env) + else: + # If we aren't editing a spack.yaml file, get config path from scope. + scope, section = _get_scope_and_section(args) + if not scope and not section: + tty.die('`spack config edit` requires a section argument ' + 'or an active environment.') + config_file = spack.config.config.get_config_filename(scope, section) - config_file = spack.config.config.get_config_filename(scope, section) if args.print_file: print(config_file) else: diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py index 651de68c49..c341271c02 100644 --- a/lib/spack/spack/main.py +++ b/lib/spack/spack/main.py @@ -740,7 +740,7 @@ def _main(argv=None): Args: argv (list or None): command line arguments, NOT including - the executable name. If None, parses from sys.argv. + the executable name. If None, parses from ``sys.argv``. """ # ------------------------------------------------------------------------ @@ -803,10 +803,17 @@ def _main(argv=None): spack.config.command_line_scopes = args.config_scopes # activate an environment if one was specified on the command line + env_format_error = None if not args.no_env: - env = spack.cmd.find_environment(args) - if env: - ev.activate(env, args.use_env_repo) + try: + env = spack.cmd.find_environment(args) + if env: + ev.activate(env, args.use_env_repo) + except spack.config.ConfigFormatError as e: + # print the context but delay this exception so that commands like + # `spack config edit` can still work with a bad environment. + e.print_context() + env_format_error = e # ------------------------------------------------------------------------ # Things that require configuration should go below here @@ -830,6 +837,13 @@ def _main(argv=None): # Re-parse with the proper sub-parser added. args, unknown = parser.parse_known_args() + # Now that we know what command this is and what its args are, determine + # whether we can continue with a bad environment and raise if not. + if env_format_error: + subcommand = getattr(args, "config_command", None) + if (cmd_name, subcommand) != ("config", "edit"): + raise env_format_error + # many operations will fail without a working directory. set_working_dir() @@ -854,7 +868,7 @@ def main(argv=None): The logic is all in ``_main()``. Args: - argv (list of str or None): command line arguments, NOT including + argv (list or None): command line arguments, NOT including the executable name. If None, parses from sys.argv. """ diff --git a/share/spack/qa/run-unit-tests b/share/spack/qa/run-unit-tests index 935b1905d1..b71103ea31 100755 --- a/share/spack/qa/run-unit-tests +++ b/share/spack/qa/run-unit-tests @@ -49,6 +49,8 @@ fi $coverage_run $(which spack) unit-test -x --verbose +bash "$QA_DIR/test-env-cfg.sh" + # Delete the symlink going from ./lib/spack/docs/_spack_root back to # the initial directory, since it causes ELOOP errors with codecov/actions@2 if [[ "$COVERAGE" == "true" ]]; then diff --git a/share/spack/qa/test-env-cfg.sh b/share/spack/qa/test-env-cfg.sh new file mode 100755 index 0000000000..ccc0e6e3b6 --- /dev/null +++ b/share/spack/qa/test-env-cfg.sh @@ -0,0 +1,73 @@ +#!/bin/bash +# +# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + +# +# This script ensures that Spack can help users edit an environment's +# manifest file even when it has invalid configuration. +# + +export QA_DIR=$(dirname "$0") +export SHARE_DIR=$(cd "$QA_DIR/.." && pwd) + +# Include convenience functions +. "$QA_DIR/test-framework.sh" +. "$QA_DIR/setup.sh" + +# Source setup-env.sh before tests +. "$SHARE_DIR/setup-env.sh" + +env_cfg="" + +function cleanup { + # Regardless of whether the test fails or succeeds, we can't remove the + # environment without restoring spack.yaml to match the schema + if [ ! -z "env_cfg" ]; then + echo "\ +spack: + specs: [] + view: False +" > "$env_cfg" + fi + + spack env deactivate + spack env rm -y broken-cfg-env +} + +trap cleanup EXIT + +spack env create broken-cfg-env +echo "Activating test environment" +spack env activate broken-cfg-env +env_cfg=`spack config --scope=env:broken-cfg-env edit --print-file` +# Save this, so we can make sure it is reported correctly when the environment +# contains broken configuration +orig_manifest_path="$env_cfg" + +echo "Environment config file: $env_cfg" +# Make sure we got a manifest file path +contains "spack.yaml" echo "$env_cfg" + +# Create an invalid packages.yaml configuration for the environment +echo "\ +spack: + specs: [] + view: False + packages: + what: +" > "$env_cfg" + +echo "Try 'spack config edit' with broken environment" +manifest_path=`spack config edit --print-file` +# Re-run command for coverage purposes +$coverage_run $(which spack) config edit --print-file + +if [ $orig_manifest_path = $manifest_path ]; then + pass +else + fail +fi + -- cgit v1.2.3-60-g2f50