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.

4 files changed, 110 insertions, 15 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index db484c12c4..4d272b51f3 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -71,6 +71,7 @@ import spack.main
 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 @@ class MakeExecutable(Executable):
 
 
 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
-    # have to occur after the spack_env object has its modifications applied.
-    # Otherwise the environment modifications 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:
-        load_module(pkg.architecture.target.module_name)
-
-    load_external_modules(pkg)
+    # Loading modules, in particular if they are meant to be used outside
+    # of Spack, can change environment variables that are relevant to the
+    # build of packages. To avoid a polluted environment, preserve the
+    # value of a few, selected, environment variables
+    with preserve_environment('CC', 'CXX', 'FC', 'F77'):
+        # All module loads that otherwise would belong in previous
+        # functions have to occur after the spack_env object has its
+        # modifications applied. Otherwise the environment modifications
+        # 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:
+            load_module(pkg.architecture.target.module_name)
+
+        load_external_modules(pkg)
 
 
 def fork(pkg, function, dirty, fake):
diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 0db7706d9c..fa67cb658b 100644
--- a/lib/spack/spack/environment.py
+++ b/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 re
 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]
diff --git a/lib/spack/spack/test/build_environment.py b/lib/spack/spack/test/build_environment.py
index 504febffac..982e7822b3 100644
--- a/lib/spack/spack/test/build_environment.py
+++ b/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'
diff --git a/lib/spack/spack/test/environment.py b/lib/spack/spack/test/environment.py
index bc1b6da3a8..f3272d0e78 100644
--- a/lib/spack/spack/test/environment.py
+++ b/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'