cmake: remove custom CMAKE_INSTALL_RPATH (#46685)

The CMake builder in Spack actually adds incorrect rpaths. They are
unfiltered and incorrectly ordered compared to what the compiler wrapper
adds.

There is no need to specify paths to dependencies in `CMAKE_INSTALL_RPATH`
because of two reasons:

1. CMake preserves "toolchain" rpaths, which includes the rpaths injected
   by our compiler wrapper.
2. We use `CMAKE_INSTALL_RPATH_USE_LINK_PATH=ON`, so libraries we link
   to are rpath'ed automatically.

However, CMake does not create install rpaths to directories in the package's
own install prefix, so we set `CMAKE_INSTALL_RPATH` to the educated guess
`<prefix>/{lib,lib64}`, but omit dependencies.

5 files changed, 37 insertions, 72 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 4d35172651..af6d0f4e76 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -91,7 +91,7 @@ from spack.util.environment import (
 )
 from spack.util.executable import Executable
 from spack.util.log_parse import make_log_context, parse_log_events
-from spack.util.module_cmd import load_module, path_from_modules
+from spack.util.module_cmd import load_module
 
 #
 # This can be set by the user to globally disable parallel builds.
@@ -792,21 +792,6 @@ def get_rpath_deps(pkg: spack.package_base.PackageBase) -> List[spack.spec.Spec]
     return _get_rpath_deps_from_spec(pkg.spec, pkg.transitive_rpaths)
 
 
-def get_rpaths(pkg):
-    """Get a list of all the rpaths for a package."""
-    rpaths = [pkg.prefix.lib, pkg.prefix.lib64]
-    deps = get_rpath_deps(pkg)
-    rpaths.extend(d.prefix.lib for d in deps if os.path.isdir(d.prefix.lib))
-    rpaths.extend(d.prefix.lib64 for d in deps if os.path.isdir(d.prefix.lib64))
-    # Second module is our compiler mod name. We use that to get rpaths from
-    # module show output.
-    if pkg.compiler.modules and len(pkg.compiler.modules) > 1:
-        mod_rpath = path_from_modules([pkg.compiler.modules[1]])
-        if mod_rpath:
-            rpaths.append(mod_rpath)
-    return list(dedupe(filter_system_paths(rpaths)))
-
-
 def load_external_modules(pkg):
     """Traverse a package's spec DAG and load any external modules.
 
@@ -1141,32 +1126,6 @@ class SetupContext:
                 env.prepend_path("PATH", bin_dir)
 
 
-def get_cmake_prefix_path(pkg):
-    # Note that unlike modifications_from_dependencies, this does not include
-    # any edits to CMAKE_PREFIX_PATH defined in custom
-    # setup_dependent_build_environment implementations of dependency packages
-    build_deps = set(pkg.spec.dependencies(deptype=("build", "test")))
-    link_deps = set(pkg.spec.traverse(root=False, deptype=("link")))
-    build_link_deps = build_deps | link_deps
-    spack_built = []
-    externals = []
-    # modifications_from_dependencies updates CMAKE_PREFIX_PATH by first
-    # prepending all externals and then all non-externals
-    for dspec in pkg.spec.traverse(root=False, order="post"):
-        if dspec in build_link_deps:
-            if dspec.external:
-                externals.insert(0, dspec)
-            else:
-                spack_built.insert(0, dspec)
-
-    ordered_build_link_deps = spack_built + externals
-    cmake_prefix_path_entries = []
-    for spec in ordered_build_link_deps:
-        cmake_prefix_path_entries.extend(spec.package.cmake_prefix_paths)
-
-    return filter_system_paths(cmake_prefix_path_entries)
-
-
 def _setup_pkg_and_run(
     serialized_pkg: "spack.subprocess_context.PackageInstallContext",
     function: Callable,
diff --git a/lib/spack/spack/build_systems/cached_cmake.py b/lib/spack/spack/build_systems/cached_cmake.py
index 3706943704..d9b415cbc7 100644
--- a/lib/spack/spack/build_systems/cached_cmake.py
+++ b/lib/spack/spack/build_systems/cached_cmake.py
@@ -10,7 +10,6 @@ from typing import Tuple
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
 
-import spack.build_environment
 import spack.builder
 
 from .cmake import CMakeBuilder, CMakePackage
@@ -297,18 +296,6 @@ class CachedCMakeBuilder(CMakeBuilder):
     def std_initconfig_entries(self):
         cmake_prefix_path_env = os.environ["CMAKE_PREFIX_PATH"]
         cmake_prefix_path = cmake_prefix_path_env.replace(os.pathsep, ";")
-        cmake_rpaths_env = spack.build_environment.get_rpaths(self.pkg)
-        cmake_rpaths_path = ";".join(cmake_rpaths_env)
-        complete_rpath_list = cmake_rpaths_path
-        if "SPACK_COMPILER_EXTRA_RPATHS" in os.environ:
-            spack_extra_rpaths_env = os.environ["SPACK_COMPILER_EXTRA_RPATHS"]
-            spack_extra_rpaths_path = spack_extra_rpaths_env.replace(os.pathsep, ";")
-            complete_rpath_list = "{0};{1}".format(complete_rpath_list, spack_extra_rpaths_path)
-
-        if "SPACK_COMPILER_IMPLICIT_RPATHS" in os.environ:
-            spack_implicit_rpaths_env = os.environ["SPACK_COMPILER_IMPLICIT_RPATHS"]
-            spack_implicit_rpaths_path = spack_implicit_rpaths_env.replace(os.pathsep, ";")
-            complete_rpath_list = "{0};{1}".format(complete_rpath_list, spack_implicit_rpaths_path)
 
         return [
             "#------------------{0}".format("-" * 60),
@@ -318,8 +305,6 @@ class CachedCMakeBuilder(CMakeBuilder):
             "#------------------{0}\n".format("-" * 60),
             cmake_cache_string("CMAKE_PREFIX_PATH", cmake_prefix_path),
             cmake_cache_string("CMAKE_INSTALL_RPATH_USE_LINK_PATH", "ON"),
-            cmake_cache_string("CMAKE_BUILD_RPATH", complete_rpath_list),
-            cmake_cache_string("CMAKE_INSTALL_RPATH", complete_rpath_list),
             self.define_cmake_cache_from_variant("CMAKE_BUILD_TYPE", "build_type"),
         ]
 
diff --git a/lib/spack/spack/build_systems/cmake.py b/lib/spack/spack/build_systems/cmake.py
index 7752c473f6..f6d346155c 100644
--- a/lib/spack/spack/build_systems/cmake.py
+++ b/lib/spack/spack/build_systems/cmake.py
@@ -8,17 +8,19 @@ import pathlib
 import platform
 import re
 import sys
-from typing import List, Optional, Tuple
+from itertools import chain
+from typing import List, Optional, Set, Tuple
 
 import llnl.util.filesystem as fs
+from llnl.util.lang import stable_partition
 
-import spack.build_environment
 import spack.builder
 import spack.deptypes as dt
 import spack.error
 import spack.package_base
 from spack.directives import build_system, conflicts, depends_on, variant
 from spack.multimethod import when
+from spack.util.environment import filter_system_paths
 
 from ._checks import BaseBuilder, execute_build_time_tests
 
@@ -152,6 +154,24 @@ def generator(*names: str, default: Optional[str] = None):
         conflicts(f"generator={x}")
 
 
+def get_cmake_prefix_path(pkg: spack.package_base.PackageBase) -> List[str]:
+    """Obtain the CMAKE_PREFIX_PATH entries for a package, based on the cmake_prefix_path package
+    attribute of direct build/test and transitive link dependencies."""
+    # Add direct build/test deps
+    selected: Set[str] = {s.dag_hash() for s in pkg.spec.dependencies(deptype=dt.BUILD | dt.TEST)}
+    # Add transitive link deps
+    selected.update(s.dag_hash() for s in pkg.spec.traverse(root=False, deptype=dt.LINK))
+    # Separate out externals so they do not shadow Spack prefixes
+    externals, spack_built = stable_partition(
+        (s for s in pkg.spec.traverse(root=False, order="topo") if s.dag_hash() in selected),
+        lambda x: x.external,
+    )
+
+    return filter_system_paths(
+        path for spec in chain(spack_built, externals) for path in spec.package.cmake_prefix_paths
+    )
+
+
 class CMakePackage(spack.package_base.PackageBase):
     """Specialized class for packages built using CMake
 
@@ -358,6 +378,16 @@ class CMakeBuilder(BaseBuilder):
             "-G",
             generator,
             define("CMAKE_INSTALL_PREFIX", pathlib.Path(pkg.prefix).as_posix()),
+            define("CMAKE_INSTALL_RPATH_USE_LINK_PATH", True),
+            # only include the install prefix lib dirs; rpaths for deps are added by USE_LINK_PATH
+            define(
+                "CMAKE_INSTALL_RPATH",
+                [
+                    pathlib.Path(pkg.prefix, "lib").as_posix(),
+                    pathlib.Path(pkg.prefix, "lib64").as_posix(),
+                ],
+            ),
+            define("CMAKE_PREFIX_PATH", get_cmake_prefix_path(pkg)),
             define("CMAKE_BUILD_TYPE", build_type),
         ]
 
@@ -372,15 +402,6 @@ class CMakeBuilder(BaseBuilder):
         _conditional_cmake_defaults(pkg, args)
         _maybe_set_python_hints(pkg, args)
 
-        # Set up CMake rpath
-        args.extend(
-            [
-                define("CMAKE_INSTALL_RPATH_USE_LINK_PATH", True),
-                define("CMAKE_INSTALL_RPATH", spack.build_environment.get_rpaths(pkg)),
-                define("CMAKE_PREFIX_PATH", spack.build_environment.get_cmake_prefix_path(pkg)),
-            ]
-        )
-
         return args
 
     @staticmethod
diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py
index 692f9d84cb..ad4c797cf9 100644
--- a/lib/spack/spack/test/packages.py
+++ b/lib/spack/spack/test/packages.py
@@ -7,7 +7,7 @@ import os
 
 import pytest
 
-import spack.build_environment
+import spack.build_systems.cmake as cmake
 import spack.directives
 import spack.error
 import spack.fetch_strategy
@@ -140,7 +140,7 @@ def test_url_for_version_with_no_urls(mock_packages, config):
 def test_custom_cmake_prefix_path(mock_packages, config):
     spec = Spec("depends-on-define-cmake-prefix-paths").concretized()
 
-    assert spack.build_environment.get_cmake_prefix_path(spec.package) == [
+    assert cmake.get_cmake_prefix_path(spec.package) == [
         spec["define-cmake-prefix-paths"].prefix.test
     ]
 
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index 2e0ef7c9b9..8db0211e4e 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -15,7 +15,7 @@ import shlex
 import subprocess
 import sys
 from functools import wraps
-from typing import Any, Callable, Dict, List, MutableMapping, Optional, Tuple, Union
+from typing import Any, Callable, Dict, Iterable, List, MutableMapping, Optional, Tuple, Union
 
 from llnl.path import path_to_os_path, system_path_filter
 from llnl.util import tty
@@ -90,7 +90,7 @@ def is_system_path(path: Path) -> bool:
     return bool(path) and (os.path.normpath(path) in SYSTEM_DIRS)
 
 
-def filter_system_paths(paths: List[Path]) -> List[Path]:
+def filter_system_paths(paths: Iterable[Path]) -> List[Path]:
     """Returns a copy of the input where system paths are filtered out."""
     return [p for p in paths if not is_system_path(p)]