cmake: remove custom CMAKE_INSTALL_RPATH (spack#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.

lib/spack/spack/build_environment.py

@@ -91,7 +91,7 @@
 )
 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 @@ def _make_runnable(self, dep: spack.spec.Spec, env: EnvironmentModifications):
                 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,

lib/spack/spack/build_systems/cached_cmake.py

@@ -10,7 +10,6 @@
 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 @@ def initconfig_hardware_entries(self):
     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 @@ def std_initconfig_entries(self):
             "#------------------{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"),
         ]
 

lib/spack/spack/build_systems/cmake.py

@@ -8,17 +8,19 @@
 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 _values(x):
         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 @@ def std_args(pkg, generator=None):
             "-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 @@ def std_args(pkg, generator=None):
         _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

lib/spack/spack/test/packages.py

@@ -7,7 +7,7 @@
 
 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
     ]
 

lib/spack/spack/util/environment.py

@@ -15,7 +15,7 @@
 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)]
 

var/spack/repos/builtin/packages/cmake/package.py

@@ -4,10 +4,12 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import os
+import pathlib
 import re
 import sys
 
 import spack.build_environment
+from spack.build_systems.cmake import get_cmake_prefix_path
 from spack.package import *
 
 
@@ -332,6 +334,13 @@ def bootstrap_args(self):
         else:
             args.append("-DCMAKE_INSTALL_PREFIX=%s" % self.prefix)
 
+        # Make CMake find its own dependencies.
+        prefixes = get_cmake_prefix_path(self)
+        rpaths = [
+            pathlib.Path(self.prefix, "lib").as_posix(),
+            pathlib.Path(self.prefix, "lib64").as_posix(),
+        ]
+
         args.extend(
             [
                 f"-DCMAKE_BUILD_TYPE={self.spec.variants['build_type'].value}",
@@ -340,17 +349,9 @@ def bootstrap_args(self):
                 "-DCMake_TEST_INSTALL=OFF",
                 f"-DBUILD_CursesDialog={'ON' if '+ncurses' in spec else 'OFF'}",
                 f"-DBUILD_QtDialog={'ON' if spec.satisfies('+qtgui') else 'OFF'}",
-            ]
-        )
-
-        # Make CMake find its own dependencies.
-        rpaths = spack.build_environment.get_rpaths(self)
-        prefixes = spack.build_environment.get_cmake_prefix_path(self)
-        args.extend(
-            [
                 "-DCMAKE_INSTALL_RPATH_USE_LINK_PATH=ON",
-                "-DCMAKE_INSTALL_RPATH={0}".format(";".join(str(v) for v in rpaths)),
-                "-DCMAKE_PREFIX_PATH={0}".format(";".join(str(v) for v in prefixes)),
+                f"-DCMAKE_INSTALL_RPATH={';'.join(rpaths)}",
+                f"-DCMAKE_PREFIX_PATH={';'.join(str(v) for v in prefixes)}",
             ]
         )
 

var/spack/repos/builtin/packages/llvm-doe/package.py

@@ -9,8 +9,8 @@
 
 import llnl.util.tty as tty
 
-import spack.build_environment
 import spack.util.executable
+from spack.build_systems.cmake import get_cmake_prefix_path
 from spack.package import *
 
 
@@ -573,7 +573,7 @@ def post_install(self):
         # unnecessary if we build openmp via LLVM_ENABLE_RUNTIMES
         if self.spec.satisfies("+cuda ~omp_as_runtime"):
             ompdir = "build-bootstrapped-omp"
-            prefix_paths = spack.build_environment.get_cmake_prefix_path(self)
+            prefix_paths = get_cmake_prefix_path(self)
             prefix_paths.append(str(spec.prefix))
             # rebuild libomptarget to get bytecode runtime library files
             with working_dir(ompdir, create=True):

var/spack/repos/builtin/packages/llvm/package.py

@@ -10,8 +10,8 @@
 import llnl.util.tty as tty
 from llnl.util.lang import classproperty
 
-import spack.build_environment
 import spack.util.executable
+from spack.build_systems.cmake import get_cmake_prefix_path
 from spack.package import *
 from spack.package_base import PackageBase
 
@@ -1072,7 +1072,7 @@ def post_install(self):
         # unnecessary if we build openmp via LLVM_ENABLE_RUNTIMES
         if self.spec.satisfies("+cuda openmp=project"):
             ompdir = "build-bootstrapped-omp"
-            prefix_paths = spack.build_environment.get_cmake_prefix_path(self)
+            prefix_paths = get_cmake_prefix_path(self)
             prefix_paths.append(str(spec.prefix))
             # rebuild libomptarget to get bytecode runtime library files
             with working_dir(ompdir, create=True):