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 <scheibel1@llnl.gov>

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:

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.
 
     """

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

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
+