Make spack config update work on environments (#36542)

Previously `spack -e bla config update <section>` would treat the
environment config scope as standard config file instead of a single
file config scope. This fixes that.

3 files changed, 46 insertions, 21 deletions

diff --git a/lib/spack/spack/cmd/config.py b/lib/spack/spack/cmd/config.py
index 0ab66d449f..e935858292 100644
--- a/lib/spack/spack/cmd/config.py
+++ b/lib/spack/spack/cmd/config.py
@@ -7,6 +7,7 @@ from __future__ import print_function
 import collections
 import os
 import shutil
+from typing import List
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
@@ -244,30 +245,35 @@ def config_remove(args):
     spack.config.set(path, existing, scope)
 
 
-def _can_update_config_file(scope_dir, cfg_file):
-    dir_ok = fs.can_write_to_dir(scope_dir)
-    cfg_ok = fs.can_access(cfg_file)
-    return dir_ok and cfg_ok
+def _can_update_config_file(scope: spack.config.ConfigScope, cfg_file):
+    if isinstance(scope, spack.config.SingleFileScope):
+        return fs.can_access(cfg_file)
+    return fs.can_write_to_dir(scope.path) and fs.can_access(cfg_file)
 
 
 def config_update(args):
     # Read the configuration files
     spack.config.config.get_config(args.section, scope=args.scope)
-    updates = spack.config.config.format_updates[args.section]
+    updates: List[spack.config.ConfigScope] = list(
+        filter(
+            lambda s: not isinstance(
+                s, (spack.config.InternalConfigScope, spack.config.ImmutableConfigScope)
+            ),
+            spack.config.config.format_updates[args.section],
+        )
+    )
 
     cannot_overwrite, skip_system_scope = [], False
     for scope in updates:
         cfg_file = spack.config.config.get_config_filename(scope.name, args.section)
-        scope_dir = scope.path
-        can_be_updated = _can_update_config_file(scope_dir, cfg_file)
+        can_be_updated = _can_update_config_file(scope, cfg_file)
         if not can_be_updated:
             if scope.name == "system":
                 skip_system_scope = True
-                msg = (
+                tty.warn(
                     'Not enough permissions to write to "system" scope. '
-                    "Skipping update at that location [cfg={0}]"
+                    f"Skipping update at that location [cfg={cfg_file}]"
                 )
-                tty.warn(msg.format(cfg_file))
                 continue
             cannot_overwrite.append((scope, cfg_file))
 
@@ -315,18 +321,14 @@ def config_update(args):
     # Get a function to update the format
     update_fn = spack.config.ensure_latest_format_fn(args.section)
     for scope in updates:
-        cfg_file = spack.config.config.get_config_filename(scope.name, args.section)
-        with open(cfg_file) as f:
-            data = syaml.load_config(f) or {}
-            data = data.pop(args.section, {})
+        data = scope.get_section(args.section).pop(args.section)
         update_fn(data)
 
         # Make a backup copy and rewrite the file
         bkp_file = cfg_file + ".bkp"
         shutil.copy(cfg_file, bkp_file)
         spack.config.config.update_config(args.section, data, scope=scope.name, force=True)
-        msg = 'File "{0}" updated [backup={1}]'
-        tty.msg(msg.format(cfg_file, bkp_file))
+        tty.msg(f'File "{cfg_file}" update [backup={bkp_file}]')
 
 
 def _can_revert_update(scope_dir, cfg_file, bkp_file):
diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index 9154593656..101fca2ff7 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -39,6 +39,7 @@ from contextlib import contextmanager
 from typing import Dict, List, Optional, Union
 
 import ruamel.yaml as yaml
+from ruamel.yaml.comments import Comment
 from ruamel.yaml.error import MarkedYAMLError
 
 import llnl.util.lang
@@ -543,16 +544,14 @@ class Configuration(object):
         scope = self._validate_scope(scope)  # get ConfigScope object
 
         # manually preserve comments
-        need_comment_copy = section in scope.sections and scope.sections[section] is not None
+        need_comment_copy = section in scope.sections and scope.sections[section]
         if need_comment_copy:
-            comments = getattr(
-                scope.sections[section][section], yaml.comments.Comment.attrib, None
-            )
+            comments = getattr(scope.sections[section][section], Comment.attrib, None)
 
         # read only the requested section's data.
         scope.sections[section] = syaml.syaml_dict({section: update_data})
         if need_comment_copy and comments:
-            setattr(scope.sections[section][section], yaml.comments.Comment.attrib, comments)
+            setattr(scope.sections[section][section], Comment.attrib, comments)
 
         scope._write_section(section)
 
diff --git a/lib/spack/spack/test/cmd/config.py b/lib/spack/spack/test/cmd/config.py
index 081e1dfefb..03c310ca1e 100644
--- a/lib/spack/spack/test/cmd/config.py
+++ b/lib/spack/spack/test/cmd/config.py
@@ -13,6 +13,7 @@ import spack.config
 import spack.database
 import spack.environment as ev
 import spack.main
+import spack.schema.config
 import spack.spec
 import spack.store
 import spack.util.spack_yaml as syaml
@@ -648,3 +649,26 @@ def test_config_prefer_upstream(
 
     # Make sure a message about the conflicting hdf5's was given.
     assert "- hdf5" in output
+
+
+def test_environment_config_update(tmpdir, mutable_config, monkeypatch):
+    with open(tmpdir.join("spack.yaml"), "w") as f:
+        f.write(
+            """\
+spack:
+  config:
+    ccache: true
+"""
+        )
+
+    def update_config(data):
+        data["ccache"] = False
+        return True
+
+    monkeypatch.setattr(spack.schema.config, "update", update_config)
+
+    with ev.Environment(str(tmpdir)):
+        config("update", "-y", "config")
+
+    with ev.Environment(str(tmpdir)) as e:
+        assert not e.raw_yaml["spack"]["config"]["ccache"]