Fix spack -c <override> when env active (#47403)

Set command line scopes last in _main, so they are higher scopes

Restore the global configuration in a spawned process by inspecting
the result of ctx.get_start_method()

Add the ability to pass a mp.context to PackageInstallContext.

Add shell-tests to check overriding the configuration:
- Using both -c and -C from command line
- With and without an environment active

lib/spack/spack/config.py

@@ -427,6 +427,10 @@ def __init__(self, *scopes: ConfigScope) -> None:
             self.push_scope(scope)
         self.format_updates: Dict[str, List[ConfigScope]] = collections.defaultdict(list)
 
+    def ensure_unwrapped(self) -> "Configuration":
+        """Ensure we unwrap this object from any dynamic wrapper (like Singleton)"""
+        return self
+
     @_config_mutator
     def push_scope(self, scope: ConfigScope) -> None:
         """Add a higher precedence scope to the Configuration."""
@@ -752,10 +756,6 @@ def override(
         assert scope is overrides
 
 
-#: configuration scopes added on the command line set by ``spack.main.main()``
-COMMAND_LINE_SCOPES: List[str] = []
-
-
 def _add_platform_scope(cfg: Configuration, name: str, path: str, writable: bool = True) -> None:
     """Add a platform-specific subdirectory for the current platform."""
     platform = spack.platforms.host().name
@@ -860,13 +860,6 @@ def create() -> Configuration:
         # Each scope can have per-platfom overrides in subdirectories
         _add_platform_scope(cfg, name, path)
 
-    # add command-line scopes
-    _add_command_line_scopes(cfg, COMMAND_LINE_SCOPES)
-
-    # we make a special scope for spack commands so that they can
-    # override configuration options.
-    cfg.push_scope(InternalConfigScope("command_line"))
-
     return cfg
 
 

lib/spack/spack/main.py

@@ -911,13 +911,6 @@ def _main(argv=None):
     # Make spack load / env activate work on macOS
     restore_macos_dyld_vars()
 
-    # make spack.config aware of any command line configuration scopes
-    if args.config_scopes:
-        spack.config.COMMAND_LINE_SCOPES = args.config_scopes
-
-    # ensure options on spack command come before everything
-    setup_main_options(args)
-
     # activate an environment if one was specified on the command line
     env_format_error = None
     if not args.no_env:
@@ -931,6 +924,12 @@ def _main(argv=None):
             e.print_context()
             env_format_error = e
 
+    # Push scopes from the command line last
+    if args.config_scopes:
+        spack.config._add_command_line_scopes(spack.config.CONFIG, args.config_scopes)
+    spack.config.CONFIG.push_scope(spack.config.InternalConfigScope("command_line"))
+    setup_main_options(args)
+
     # ------------------------------------------------------------------------
     # Things that require configuration should go below here
     # ------------------------------------------------------------------------

lib/spack/spack/subprocess_context.py

@@ -17,7 +17,6 @@
 import multiprocessing
 import pickle
 import pydoc
-import sys
 from types import ModuleType
 
 import spack.config
@@ -27,9 +26,6 @@
 import spack.repo
 import spack.store
 
-_SERIALIZE = sys.platform == "win32" or (sys.version_info >= (3, 8) and sys.platform == "darwin")
-
-
 patches = None
 
 
@@ -56,7 +52,7 @@ def _restore_and_run(self, fn, test_state):
         fn()
 
     def create(self):
-        test_state = TestState()
+        test_state = GlobalStateMarshaler()
         return multiprocessing.Process(target=self._restore_and_run, args=(self.fn, test_state))
 
 
@@ -65,49 +61,56 @@ class PackageInstallContext:
     needs to be transmitted to a child process.
     """
 
-    def __init__(self, pkg):
-        if _SERIALIZE:
+    def __init__(self, pkg, *, ctx=None):
+        ctx = ctx or multiprocessing.get_context()
+        self.serialize = ctx.get_start_method() != "fork"
+        if self.serialize:
             self.serialized_pkg = serialize(pkg)
+            self.global_state = GlobalStateMarshaler()
             self.serialized_env = serialize(spack.environment.active_environment())
         else:
             self.pkg = pkg
+            self.global_state = None
             self.env = spack.environment.active_environment()
         self.spack_working_dir = spack.paths.spack_working_dir
-        self.test_state = TestState()
 
     def restore(self):
-        self.test_state.restore()
         spack.paths.spack_working_dir = self.spack_working_dir
-        env = pickle.load(self.serialized_env) if _SERIALIZE else self.env
+        env = pickle.load(self.serialized_env) if self.serialize else self.env
+        # Activating the environment modifies the global configuration, so globals have to
+        # be restored afterward, in case other modifications were applied on top (e.g. from
+        # command line)
         if env:
             spack.environment.activate(env)
+
+        if self.serialize:
+            self.global_state.restore()
+
         # Order of operation is important, since the package might be retrieved
         # from a repo defined within the environment configuration
-        pkg = pickle.load(self.serialized_pkg) if _SERIALIZE else self.pkg
+        pkg = pickle.load(self.serialized_pkg) if self.serialize else self.pkg
         return pkg
 
 
-class TestState:
-    """Spack tests may modify state that is normally read from disk in memory;
-    this object is responsible for properly serializing that state to be
-    applied to a subprocess. This isn't needed outside of a testing environment
-    but this logic is designed to behave the same inside or outside of tests.
+class GlobalStateMarshaler:
+    """Class to serialize and restore global state for child processes.
+
+    Spack may modify state that is normally read from disk or command line in memory;
+    this object is responsible for properly serializing that state to be applied to a subprocess.
     """
 
     def __init__(self):
-        if _SERIALIZE:
-            self.config = spack.config.CONFIG
-            self.platform = spack.platforms.host
-            self.test_patches = store_patches()
-            self.store = spack.store.STORE
+        self.config = spack.config.CONFIG.ensure_unwrapped()
+        self.platform = spack.platforms.host
+        self.test_patches = store_patches()
+        self.store = spack.store.STORE
 
     def restore(self):
-        if _SERIALIZE:
-            spack.config.CONFIG = self.config
-            spack.repo.PATH = spack.repo.create(self.config)
-            spack.platforms.host = self.platform
-            spack.store.STORE = self.store
-            self.test_patches.restore()
+        spack.config.CONFIG = self.config
+        spack.repo.PATH = spack.repo.create(self.config)
+        spack.platforms.host = self.platform
+        spack.store.STORE = self.store
+        self.test_patches.restore()
 
 
 class TestPatches:

share/spack/qa/config_state.py

@@ -0,0 +1,36 @@
+# Copyright 2013-2024 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)
+"""Used to test correct application of config line scopes in various cases.
+
+The option `config:cache` is supposed to be False, and overridden to True
+from the command line.
+"""
+import multiprocessing as mp
+
+import spack.config
+import spack.subprocess_context
+
+
+def show_config(serialized_state):
+    _ = serialized_state.restore()
+    result = spack.config.CONFIG.get("config:ccache")
+    if result is not True:
+        raise RuntimeError(f"Expected config:ccache:true, but got {result}")
+
+
+if __name__ == "__main__":
+    print("Testing spawn")
+    ctx = mp.get_context("spawn")
+    serialized_state = spack.subprocess_context.PackageInstallContext(None, ctx=ctx)
+    p = ctx.Process(target=show_config, args=(serialized_state,))
+    p.start()
+    p.join()
+
+    print("Testing fork")
+    ctx = mp.get_context("fork")
+    serialized_state = spack.subprocess_context.PackageInstallContext(None, ctx=ctx)
+    p = ctx.Process(target=show_config, args=(serialized_state,))
+    p.start()
+    p.join()

share/spack/qa/run-unit-tests

@@ -52,7 +52,7 @@ if [[ "$UNIT_TEST_COVERAGE" != "true" ]] && python -m pytest -VV 2>&1 | grep xdi
 fi
 
 # We are running pytest-cov after the addition of pytest-xdist, since it integrates
-# other pugins for pytest automatically. We still need to use "coverage" explicitly
+# other plugins for pytest automatically. We still need to use "coverage" explicitly
 # for the commands above.
 #
 # There is a need to pass the configuration file explicitly due to a bug:

share/spack/qa/setup-env-test.sh

@@ -207,3 +207,20 @@ fails spack env deactivate
 
 echo "Correct error exit codes for unit-test when it fails"
 fails spack unit-test fail
+
+title "Testing config override from command line, outside of an environment"
+contains 'True' spack -c config:ccache:true python -c "import spack.config;print(spack.config.CONFIG.get('config:ccache'))"
+contains 'True' spack -C "$SHARE_DIR/qa/configuration" python -c "import spack.config;print(spack.config.CONFIG.get('config:ccache'))"
+succeeds spack -c config:ccache:true python "$SHARE_DIR/qa/config_state.py"
+succeeds spack -C "$SHARE_DIR/qa/configuration" python "$SHARE_DIR/qa/config_state.py"
+
+title "Testing config override from command line, inside an environment"
+spack env activate --temp
+spack config add "config:ccache:false"
+
+contains 'True' spack -c config:ccache:true python -c "import spack.config;print(spack.config.CONFIG.get('config:ccache'))"
+contains 'True' spack -C "$SHARE_DIR/qa/configuration" python -c "import spack.config;print(spack.config.CONFIG.get('config:ccache'))"
+succeeds spack -c config:ccache:true python "$SHARE_DIR/qa/config_state.py"
+succeeds spack -C "$SHARE_DIR/qa/configuration" python "$SHARE_DIR/qa/config_state.py"
+
+spack env deactivate