Spack environment updates take precedence (#9107)

Spack originally handled environment modifications in the following
order:

 1. clear environment variables
    (unless Spack was invoked with --dirty)
 2. apply spack-specific environment variable updates,
    including variables set by Spack core like CC/PKG_CONFIG_PATH
	and those set by installed dependencies (e.g. in
	setup_dependent_environment)
 3. load all external/compiler modules

1 and 2 were done together. This splits 1 into its own function and
imposes the following order for environment modifications:

 1. clear environment variables
 2. load all external/compiler modules
 3. apply spack-specific environment variable updates

As a result, prepend-path actions taken by Spack (or installed Spack
dependencies) take precedence over prepend-path actions from compiler
and external modules. Additionally, when Spack (or a package
dependency) sets/unsets an environment variable, that will override
the actions of external/compiler modules.

3 files changed, 64 insertions, 28 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index f4c4d71a85..b6288993dd 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -146,6 +146,33 @@ class MakeExecutable(Executable):
         return super(MakeExecutable, self).__call__(*args, **kwargs)
 
 
+def clean_environment():
+    # Stuff in here sanitizes the build environment to eliminate
+    # anything the user has set that may interfere. We apply it immediately
+    # unlike the other functions so it doesn't overwrite what the modules load.
+    env = EnvironmentModifications()
+
+    # Remove these vars from the environment during build because they
+    # can affect how some packages find libraries.  We want to make
+    # sure that builds never pull in unintended external dependencies.
+    env.unset('LD_LIBRARY_PATH')
+    env.unset('LIBRARY_PATH')
+    env.unset('CPATH')
+    env.unset('LD_RUN_PATH')
+    env.unset('DYLD_LIBRARY_PATH')
+
+    # Remove any macports installs from the PATH.  The macports ld can
+    # cause conflicts with the built-in linker on el capitan.  Solves
+    # assembler issues, e.g.:
+    #    suffix or operands invalid for `movq'"
+    path = get_path('PATH')
+    for p in path:
+        if '/macports/' in p:
+            env.remove_path('PATH', p)
+
+    env.apply_modifications()
+
+
 def set_compiler_environment_variables(pkg, env):
     assert pkg.spec.concrete
     compiler = pkg.compiler
@@ -278,27 +305,6 @@ def set_build_environment_variables(pkg, env, dirty):
     # Install root prefix
     env.set(SPACK_INSTALL, spack.store.root)
 
-    # Stuff in here sanitizes the build environment to eliminate
-    # anything the user has set that may interfere.
-    if not dirty:
-        # Remove these vars from the environment during build because they
-        # can affect how some packages find libraries.  We want to make
-        # sure that builds never pull in unintended external dependencies.
-        env.unset('LD_LIBRARY_PATH')
-        env.unset('LIBRARY_PATH')
-        env.unset('CPATH')
-        env.unset('LD_RUN_PATH')
-        env.unset('DYLD_LIBRARY_PATH')
-
-        # Remove any macports installs from the PATH.  The macports ld can
-        # cause conflicts with the built-in linker on el capitan.  Solves
-        # assembler issues, e.g.:
-        #    suffix or operands invalid for `movq'"
-        path = get_path('PATH')
-        for p in path:
-            if '/macports/' in p:
-                env.remove_path('PATH', p)
-
     # Set environment variables if specified for
     # the given compiler
     compiler = pkg.compiler
@@ -629,6 +635,9 @@ def setup_package(pkg, dirty):
     spack_env = EnvironmentModifications()
     run_env = EnvironmentModifications()
 
+    if not dirty:
+        clean_environment()
+
     set_compiler_environment_variables(pkg, spack_env)
     set_build_environment_variables(pkg, spack_env, dirty)
     pkg.architecture.platform.setup_platform_environment(pkg, spack_env)
@@ -656,14 +665,12 @@ def setup_package(pkg, dirty):
     set_module_variables_for_package(pkg, pkg.module)
     pkg.setup_environment(spack_env, run_env)
 
-    # Make sure nothing's strange about the Spack environment.
-    validate(spack_env, tty.warn)
-    spack_env.apply_modifications()
-
     # 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 the current ordering of environment modifications, this is strictly
+    # unnecessary. Modules affecting these variables will be overwritten anyway
     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
@@ -681,6 +688,10 @@ def setup_package(pkg, dirty):
 
         load_external_modules(pkg)
 
+    # Make sure nothing's strange about the Spack environment.
+    validate(spack_env, tty.warn)
+    spack_env.apply_modifications()
+
 
 def fork(pkg, function, dirty, fake):
     """Fork a child process to do part of a spack build.
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 8bf230a576..d3bff22583 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1478,7 +1478,7 @@ class Spec(object):
         if self.external:
             d['external'] = {
                 'path': self.external_path,
-                'module': bool(self.external_module)
+                'module': self.external_module
             }
 
         # TODO: restore build dependencies here once we have less picky
@@ -1916,7 +1916,7 @@ class Spec(object):
                     dedupe(p + patches))
 
         for s in self.traverse():
-            if s.external_module:
+            if s.external_module and not s.external_path:
                 compiler = spack.compilers.compiler_for_spec(
                     s.compiler, s.architecture)
                 for mod in compiler.modules:
diff --git a/lib/spack/spack/test/build_environment.py b/lib/spack/spack/test/build_environment.py
index 4ac63fa5dc..df88904b4f 100644
--- a/lib/spack/spack/test/build_environment.py
+++ b/lib/spack/spack/test/build_environment.py
@@ -131,7 +131,6 @@ def test_cc_not_changed_by_modules(monkeypatch):
 
 @pytest.mark.usefixtures('config', 'mock_packages')
 def test_compiler_config_modifications(monkeypatch):
-
     s = spack.spec.Spec('cmake')
     s.concretize()
     pkg = s.package
@@ -209,3 +208,29 @@ def test_compiler_config_modifications(monkeypatch):
     os.environ.pop('PATH_LIST', None)
     os.environ.pop('EMPTY_PATH_LIST', None)
     os.environ.pop('NEW_PATH_LIST', None)
+
+
+@pytest.mark.regression('9107')
+def test_spack_paths_before_module_paths(config, mock_packages, monkeypatch):
+    s = spack.spec.Spec('cmake')
+    s.concretize()
+    pkg = s.package
+
+    module_path = '/path/to/module'
+
+    def _set_wrong_cc(x):
+        os.environ['PATH'] = module_path + ':' + os.environ['PATH']
+
+    monkeypatch.setattr(
+        spack.build_environment, 'load_module', _set_wrong_cc
+    )
+    monkeypatch.setattr(
+        pkg.compiler, 'modules', ['some_module']
+    )
+
+    spack.build_environment.setup_package(pkg, False)
+
+    spack_path = os.path.join(spack.paths.prefix, 'lib/spack/env')
+    paths = os.environ['PATH'].split(':')
+
+    assert paths.index(spack_path) < paths.index(module_path)