Externals with merged prefixes (#22653)

We remove system paths from search variables like PATH and 
from -L options because they may contain many packages and
could interfere with Spack-built packages. External packages 
may be installed to prefixes that are not actually system paths 
but are still "merged" in the sense that many other packages are
installed there. To avoid conflicts, this PR places all external
packages at the end of search paths.

lib/spack/spack/build_environment.py

@@ -306,6 +306,19 @@ def set_compiler_environment_variables(pkg, env):
     return env
 
 
+def _place_externals_last(spec_container):
+    """
+    For a (possibly unordered) container of specs, return an ordered list
+    where all external specs are at the end of the list. External packages
+    may be installed in merged prefixes with other packages, and so
+    they should be deprioritized for any search order (i.e. in PATH, or
+    for a set of -L entries in a compiler invocation).
+    """
+    first = list(x for x in spec_container if not x.external)
+    second = list(x for x in spec_container if x.external)
+    return first + second
+
+
 def set_build_environment_variables(pkg, env, dirty):
     """Ensure a clean install environment when we build packages.
 
@@ -323,6 +336,29 @@ def set_build_environment_variables(pkg, env, dirty):
     link_deps       = set(pkg.spec.traverse(root=False, deptype=('link')))
     build_link_deps = build_deps | link_deps
     rpath_deps      = get_rpath_deps(pkg)
+    # This includes all build dependencies and any other dependencies that
+    # should be added to PATH (e.g. supporting executables run by build
+    # dependencies)
+    build_and_supporting_deps = set()
+    for build_dep in build_deps:
+        build_and_supporting_deps.update(build_dep.traverse(deptype='run'))
+
+    # Establish an arbitrary but fixed ordering of specs so that resulting
+    # environment variable values are stable
+    def _order(specs):
+        return sorted(specs, key=lambda x: x.name)
+
+    # External packages may be installed in a prefix which contains many other
+    # package installs. To avoid having those installations override
+    # Spack-installed packages, they are placed at the end of search paths.
+    # System prefixes are removed entirely later on since they are already
+    # searched.
+    build_deps = _place_externals_last(_order(build_deps))
+    link_deps = _place_externals_last(_order(link_deps))
+    build_link_deps = _place_externals_last(_order(build_link_deps))
+    rpath_deps = _place_externals_last(_order(rpath_deps))
+    build_and_supporting_deps = _place_externals_last(
+        _order(build_and_supporting_deps))
 
     link_dirs = []
     include_dirs = []
@@ -369,21 +405,10 @@ def set_build_environment_variables(pkg, env, dirty):
     env.set(SPACK_INCLUDE_DIRS, ':'.join(include_dirs))
     env.set(SPACK_RPATH_DIRS, ':'.join(rpath_dirs))
 
-    build_prefixes      = [dep.prefix for dep in build_deps]
+    build_and_supporting_prefixes = filter_system_paths(
-    build_link_prefixes = [dep.prefix for dep in build_link_deps]
+        x.prefix for x in build_and_supporting_deps)
-
+    build_link_prefixes = filter_system_paths(
-    # add run-time dependencies of direct build-time dependencies:
+        x.prefix for x in build_link_deps)
-    for build_dep in build_deps:
-        for run_dep in build_dep.traverse(deptype='run'):
-            build_prefixes.append(run_dep.prefix)
-
-    # Filter out system paths: ['/', '/usr', '/usr/local']
-    # These paths can be introduced into the build when an external package
-    # is added as a dependency. The problem with these paths is that they often
-    # contain hundreds of other packages installed in the same directory.
-    # If these paths come first, they can overshadow Spack installations.
-    build_prefixes      = filter_system_paths(build_prefixes)
-    build_link_prefixes = filter_system_paths(build_link_prefixes)
 
     # Add dependencies to CMAKE_PREFIX_PATH
     env.set_path('CMAKE_PREFIX_PATH', build_link_prefixes)
@@ -398,7 +423,10 @@ def set_build_environment_variables(pkg, env, dirty):
         env.set('SPACK_COMPILER_EXTRA_RPATHS', extra_rpaths)
 
     # Add bin directories from dependencies to the PATH for the build.
-    for prefix in build_prefixes:
+    # These directories are added to the beginning of the search path, and in
+    # the order given by 'build_and_supporting_prefixes' (the iteration order
+    # is reversed because each entry is prepended)
+    for prefix in reversed(build_and_supporting_prefixes):
         for dirname in ['bin', 'bin64']:
             bin_dir = os.path.join(prefix, dirname)
             if os.path.isdir(bin_dir):
@@ -442,7 +470,7 @@ def set_build_environment_variables(pkg, env, dirty):
         env.set(SPACK_CCACHE_BINARY, ccache)
 
     # Add any pkgconfig directories to PKG_CONFIG_PATH
-    for prefix in build_link_prefixes:
+    for prefix in reversed(build_link_prefixes):
         for directory in ('lib', 'lib64', 'share'):
             pcdir = os.path.join(prefix, directory, 'pkgconfig')
             if os.path.isdir(pcdir):

lib/spack/spack/test/build_environment.py

@@ -11,6 +11,7 @@
 import spack.build_environment
 import spack.config
 import spack.spec
+import spack.util.spack_yaml as syaml
 from spack.paths import build_env_path
 from spack.build_environment import dso_suffix, _static_to_shared_library
 from spack.build_environment import determine_number_of_jobs
@@ -299,6 +300,45 @@ def normpaths(paths):
         delattr(dep_pkg, 'libs')
 
 
+def test_external_prefixes_last(mutable_config, mock_packages, working_env,
+                                monkeypatch):
+    # Sanity check: under normal circumstances paths associated with
+    # dt-diamond-left would appear first. We'll mark it as external in
+    # the test to check if the associated paths are placed last.
+    assert 'dt-diamond-left' < 'dt-diamond-right'
+
+    cfg_data = syaml.load_config("""\
+dt-diamond-left:
+  externals:
+  - spec: dt-diamond-left@1.0
+    prefix: /fake/path1
+  buildable: false
+""")
+    spack.config.set("packages", cfg_data)
+    top = spack.spec.Spec('dt-diamond').concretized()
+
+    def _trust_me_its_a_dir(path):
+        return True
+    monkeypatch.setattr(
+        os.path, 'isdir', _trust_me_its_a_dir
+    )
+
+    env_mods = EnvironmentModifications()
+    spack.build_environment.set_build_environment_variables(
+        top.package, env_mods, False)
+
+    env_mods.apply_modifications()
+    link_dir_var = os.environ['SPACK_LINK_DIRS']
+    link_dirs = link_dir_var.split(':')
+    external_lib_paths = set(['/fake/path1/lib', '/fake/path1/lib64'])
+    # The external lib paths should be the last two entries of the list and
+    # should not appear anywhere before the last two entries
+    assert (set(os.path.normpath(x) for x in link_dirs[-2:]) ==
+            external_lib_paths)
+    assert not (set(os.path.normpath(x) for x in link_dirs[:-2]) &
+                external_lib_paths)
+
+
 def test_parallel_false_is_not_propagating(config, mock_packages):
     class AttributeHolder(object):
         pass