Preserve Spack CC/FC/F77/CXX settings when loading modules (#8346)

Fixes #8345

Spack environment modifications are applied before modules are loaded; this
includes settings to CC, FC, F77, and CXX, which point to the Spack compiler
wrappers. If the loaded modules set CC, this overrides the Spack compiler
wrappers. This PR adds a context manager to preserve the values of CC etc. that
are set by Spack: any effects on the CC, FC, F77, and CXX variables from modules
are undone and their original values are restored.

lib/spack/spack/build_environment.py

@@ -71,6 +71,7 @@
 import spack.paths
 import spack.store
 from spack.environment import EnvironmentModifications, validate
+from spack.environment import preserve_environment
 from spack.util.environment import env_flag, filter_system_paths, get_path
 from spack.util.executable import Executable
 from spack.util.module_cmd import load_module, get_path_from_module
@@ -130,7 +131,7 @@ def __call__(self, *args, **kwargs):
 
 
 def set_compiler_environment_variables(pkg, env):
-    assert(pkg.spec.concrete)
+    assert pkg.spec.concrete
     compiler = pkg.compiler
 
     # Set compiler variables used by CMake and autotools
@@ -610,20 +611,26 @@ def setup_package(pkg, dirty):
     validate(spack_env, tty.warn)
     spack_env.apply_modifications()
 
-    # All module loads that otherwise would belong in previous functions
+    # Loading modules, in particular if they are meant to be used outside
-    # have to occur after the spack_env object has its modifications applied.
+    # of Spack, can change environment variables that are relevant to the
-    # Otherwise the environment modifications could undo module changes, such
+    # build of packages. To avoid a polluted environment, preserve the
-    # as unsetting LD_LIBRARY_PATH after a module changes it.
+    # value of a few, selected, environment variables
-    for mod in pkg.compiler.modules:
+    with preserve_environment('CC', 'CXX', 'FC', 'F77'):
-        # Fixes issue https://github.com/spack/spack/issues/3153
+        # All module loads that otherwise would belong in previous
-        if os.environ.get("CRAY_CPU_TARGET") == "mic-knl":
+        # functions have to occur after the spack_env object has its
-            load_module("cce")
+        # modifications applied. Otherwise the environment modifications
-        load_module(mod)
+        # could undo module changes, such as unsetting LD_LIBRARY_PATH
+        # after a module changes it.
+        for mod in pkg.compiler.modules:
+            # Fixes issue https://github.com/spack/spack/issues/3153
+            if os.environ.get("CRAY_CPU_TARGET") == "mic-knl":
+                load_module("cce")
+            load_module(mod)
 
-    if pkg.architecture.target.module_name:
+        if pkg.architecture.target.module_name:
-        load_module(pkg.architecture.target.module_name)
+            load_module(pkg.architecture.target.module_name)
 
-    load_external_modules(pkg)
+        load_external_modules(pkg)
 
 
 def fork(pkg, function, dirty, fake):

lib/spack/spack/environment.py

@@ -23,6 +23,7 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 ##############################################################################
 import collections
+import contextlib
 import inspect
 import json
 import os
@@ -30,6 +31,9 @@
 import sys
 import os.path
 import subprocess
+
+import llnl.util.tty as tty
+
 from llnl.util.lang import dedupe
 
 
@@ -598,3 +602,41 @@ def inspect_path(root, inspections, exclude=None):
                 env.prepend_path(variable, expected)
 
     return env
+
+
+@contextlib.contextmanager
+def preserve_environment(*variables):
+    """Ensures that the value of the environment variables passed as
+    arguments is the same before entering to the context manager and after
+    exiting it.
+
+    Variables that are unset before entering the context manager will be
+    explicitly unset on exit.
+
+    Args:
+        variables (list of str): list of environment variables to be preserved
+    """
+    cache = {}
+    for var in variables:
+        # The environment variable to be preserved might not be there.
+        # In that case store None as a placeholder.
+        cache[var] = os.environ.get(var, None)
+
+    yield
+
+    for var in variables:
+        value = cache[var]
+        msg = '[PRESERVE_ENVIRONMENT]'
+        if value is not None:
+            # Print a debug statement if the value changed
+            if var not in os.environ:
+                msg += ' {0} was unset, will be reset to "{1}"'
+                tty.debug(msg.format(var, value))
+            elif os.environ[var] != value:
+                msg += ' {0} was set to "{1}", will be reset to "{2}"'
+                tty.debug(msg.format(var, os.environ[var], value))
+            os.environ[var] = value
+        elif var in os.environ:
+            msg += ' {0} was set to "{1}", will be unset'
+            tty.debug(msg.format(var, os.environ[var]))
+            del os.environ[var]

lib/spack/spack/test/build_environment.py

@@ -25,6 +25,8 @@
 import os
 import pytest
 
+import spack.build_environment
+import spack.spec
 from spack.paths import build_env_path
 from spack.build_environment import dso_suffix, _static_to_shared_library
 from spack.util.executable import Executable
@@ -96,3 +98,28 @@ def test_static_to_shared_library(build_environment):
 
             assert output == expected[arch].format(
                 static_lib, shared_lib, os.path.basename(shared_lib))
+
+
+@pytest.mark.regression('8345')
+@pytest.mark.usefixtures('config', 'mock_packages')
+def test_cc_not_changed_by_modules(monkeypatch):
+
+    s = spack.spec.Spec('cmake')
+    s.concretize()
+    pkg = s.package
+
+    def _set_wrong_cc(x):
+        os.environ['CC'] = 'NOT_THIS_PLEASE'
+        os.environ['ANOTHER_VAR'] = 'THIS_IS_SET'
+
+    monkeypatch.setattr(
+        spack.build_environment, 'load_module', _set_wrong_cc
+    )
+    monkeypatch.setattr(
+        pkg.compiler, 'modules', ['some_module']
+    )
+
+    spack.build_environment.setup_package(pkg, False)
+
+    assert os.environ['CC'] != 'NOT_THIS_PLEASE'
+    assert os.environ['ANOTHER_VAR'] == 'THIS_IS_SET'

lib/spack/spack/test/environment.py

@@ -306,3 +306,22 @@ def test_source_files(files_to_be_sourced):
     assert modifications['PATH_LIST'][1].value == '/path/fourth'
     assert isinstance(modifications['PATH_LIST'][2], PrependPath)
     assert modifications['PATH_LIST'][2].value == '/path/first'
+
+
+@pytest.mark.regression('8345')
+def test_preserve_environment(prepare_environment_for_tests):
+    # UNSET_ME is defined, and will be unset in the context manager,
+    # NOT_SET is not in the environment and will be set within the
+    # context manager, PATH_LIST is set and will be changed.
+    with environment.preserve_environment('UNSET_ME', 'NOT_SET', 'PATH_LIST'):
+        os.environ['NOT_SET'] = 'a'
+        assert os.environ['NOT_SET'] == 'a'
+
+        del os.environ['UNSET_ME']
+        assert 'UNSET_ME' not in os.environ
+
+        os.environ['PATH_LIST'] = 'changed'
+
+    assert 'NOT_SET' not in os.environ
+    assert os.environ['UNSET_ME'] == 'foo'
+    assert os.environ['PATH_LIST'] == '/path/second:/path/third'