Config: cache results of get_config (#19605)

`config.get_config` now caches the results and returns the same
configuration if called multiple times with the same arguments
(i.e. the same section and scope).

As a consequence, it is expected that users will always call
update methods provided in the `config` module after changing
the configuration (even if manipulating it as a Python nested
dictionary). The following two examples should cover most
scenarios:

* Most configuration update logic in the core (e.g. relating to
  adding new compiler) should call `Configuration.update_config`
* Tests that need to change the global configuration should use the
  newly-provided `config.replace_config` function.

(if neither of these methods apply, then the essential requirement
is to use a method marked as `_config_mutator`)

Failure to call such a function after modifying the configuration
will lead to unexpected results (e.g. calling `get_config` after
changing the configuration will not reflect the changes since the
first call to get_config).

4 files changed, 49 insertions, 12 deletions

diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index 9c12454edb..393a2ebb31 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -2,7 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 """This module implements Spack's configuration file handling.
 
 This implements Spack's configuration system, which handles merging
@@ -29,9 +28,9 @@ When read in, Spack validates configurations with jsonschemas.  The
 schemas are in submodules of :py:mod:`spack.schema`.
 
 """
-
 import collections
 import copy
+import functools
 import os
 import re
 import sys
@@ -157,7 +156,7 @@ class ConfigScope(object):
             self.sections[section] = data
         return self.sections[section]
 
-    def write_section(self, section):
+    def _write_section(self, section):
         filename = self.get_section_filename(section)
         data = self.get_section(section)
 
@@ -253,7 +252,7 @@ class SingleFileScope(ConfigScope):
                 self.sections[section_key] = {section_key: data}
         return self.sections.get(section, None)
 
-    def write_section(self, section):
+    def _write_section(self, section):
         data_to_write = self._raw_data
 
         # If there is no existing data, this section SingleFileScope has never
@@ -305,7 +304,7 @@ class ImmutableConfigScope(ConfigScope):
     This is used for ConfigScopes passed on the command line.
     """
 
-    def write_section(self, section):
+    def _write_section(self, section):
         raise ConfigError("Cannot write to immutable scope %s" % self)
 
     def __repr__(self):
@@ -341,7 +340,7 @@ class InternalConfigScope(ConfigScope):
             self.sections[section] = None
         return self.sections[section]
 
-    def write_section(self, section):
+    def _write_section(self, section):
         """This only validates, as the data is already in memory."""
         data = self.get_section(section)
         if data is not None:
@@ -371,6 +370,18 @@ class InternalConfigScope(ConfigScope):
         return result
 
 
+def _config_mutator(method):
+    """Decorator to mark all the methods in the Configuration class
+    that mutate the underlying configuration. Used to clear the
+    memoization cache.
+    """
+    @functools.wraps(method)
+    def _method(self, *args, **kwargs):
+        self._get_config_memoized.cache.clear()
+        return method(self, *args, **kwargs)
+    return _method
+
+
 class Configuration(object):
     """A full Spack configuration, from a hierarchy of config files.
 
@@ -390,6 +401,7 @@ class Configuration(object):
             self.push_scope(scope)
         self.format_updates = collections.defaultdict(list)
 
+    @_config_mutator
     def push_scope(self, scope):
         """Add a higher precedence scope to the Configuration."""
         cmd_line_scope = None
@@ -404,11 +416,13 @@ class Configuration(object):
         if cmd_line_scope:
             self.scopes['command_line'] = cmd_line_scope
 
+    @_config_mutator
     def pop_scope(self):
         """Remove the highest precedence scope and return it."""
         name, scope = self.scopes.popitem(last=True)
         return scope
 
+    @_config_mutator
     def remove_scope(self, scope_name):
         return self.scopes.pop(scope_name)
 
@@ -472,6 +486,7 @@ class Configuration(object):
         scope = self._validate_scope(scope)
         return scope.get_section_filename(section)
 
+    @_config_mutator
     def clear_caches(self):
         """Clears the caches for configuration files,
 
@@ -479,6 +494,7 @@ class Configuration(object):
         for scope in self.scopes.values():
             scope.clear()
 
+    @_config_mutator
     def update_config(self, section, update_data, scope=None, force=False):
         """Update the configuration file for a particular scope.
 
@@ -526,7 +542,7 @@ class Configuration(object):
                     yaml.comments.Comment.attrib,
                     comments)
 
-        scope.write_section(section)
+        scope._write_section(section)
 
     def get_config(self, section, scope=None):
         """Get configuration settings for a section.
@@ -552,6 +568,10 @@ class Configuration(object):
            }
 
         """
+        return self._get_config_memoized(section, scope)
+
+    @llnl.util.lang.memoized
+    def _get_config_memoized(self, section, scope):
         _validate_section_name(section)
 
         if scope is None:
@@ -619,6 +639,7 @@ class Configuration(object):
 
         return value
 
+    @_config_mutator
     def set(self, path, value, scope=None):
         """Convenience function for setting single values in config files.
 
@@ -784,6 +805,22 @@ def _config():
 config = llnl.util.lang.Singleton(_config)
 
 
+def replace_config(configuration):
+    """Replace the current global configuration with the instance passed as
+    argument.
+
+    Args:
+        configuration (Configuration): the new configuration to be used.
+
+    Returns:
+        The old configuration that has been removed
+    """
+    global config
+    config.clear_caches(), configuration.clear_caches()
+    old_config, config = config, configuration
+    return old_config
+
+
 def get(path, default=None, scope=None):
     """Module-level wrapper for ``Configuration.get()``."""
     return config.get(path, default, scope)
diff --git a/lib/spack/spack/test/cmd/common/arguments.py b/lib/spack/spack/test/cmd/common/arguments.py
index 5117607a2e..76e39bcad0 100644
--- a/lib/spack/spack/test/cmd/common/arguments.py
+++ b/lib/spack/spack/test/cmd/common/arguments.py
@@ -20,6 +20,7 @@ def parser():
     arguments.add_common_arguments(p, ['jobs'])
     yield p
     # Cleanup the command line scope if it was set during tests
+    spack.config.config.clear_caches()
     if 'command_line' in spack.config.config.scopes:
         spack.config.config.scopes['command_line'].clear()
 
diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
index f43a571e73..b852f4777e 100644
--- a/lib/spack/spack/test/config.py
+++ b/lib/spack/spack/test/config.py
@@ -766,7 +766,7 @@ config:
     assert data['config']['install_tree'] == {'root': 'dummy_tree_value'}
 
     with pytest.raises(spack.config.ConfigError):
-        scope.write_section('config')
+        scope._write_section('config')
 
 
 def test_single_file_scope(tmpdir, config):
@@ -839,7 +839,7 @@ def test_write_empty_single_file_scope(tmpdir):
     env_schema = spack.schema.env.schema
     scope = spack.config.SingleFileScope(
         'test', str(tmpdir.ensure('config.yaml')), env_schema, ['spack'])
-    scope.write_section('config')
+    scope._write_section('config')
     # confirm we can write empty config
     assert not scope.get_section('config')
 
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 0a39d1c6c3..f43d6933ba 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -318,8 +318,7 @@ spack.config.config = spack.config._config()
 @contextlib.contextmanager
 def use_configuration(config):
     """Context manager to swap out the global Spack configuration."""
-    saved = spack.config.config
-    spack.config.config = config
+    saved = spack.config.replace_config(config)
 
     # Avoid using real spack configuration that has been cached by other
     # tests, and avoid polluting the cache with spack test configuration
@@ -329,7 +328,7 @@ def use_configuration(config):
 
     yield
 
-    spack.config.config = saved
+    spack.config.replace_config(saved)
     spack.compilers._cache_config_file = saved_compiler_cache