Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: do not override cmake rpath defines #5271

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

haampie
Copy link

@haampie haampie commented Jan 24, 2025

Describe your changes

This removes overriding the values of

  • CMAKE_INSTALL_RPATH_USE_LINK_PATH Some package managers (e.g. Spack package manager) set this to ON to get automatic rpaths to e.g. libmpi.so and libz.so if they are in non-standard locations.
  • CMAKE_SKIP_BUILD_RPATH / CMAKE_BUILD_WITH_INSTALL_RPATH: they're redundant, just use the defaults to follow the principle of least astonishment. If you ever run a just-built executable during the build, then leaving those to the standard value is better, since you don't really know where the executables/libraries reside during the build.
  • CMAKE_INSTALL_NAME_DIR: this was a workaround for CMake < 3 on macOS. As of CMake 3, it's the default, see CMP0042.
  • CMAKE_INSTALL_RPATH. Setting this variable overrides what the user passes through -DCMAKE_INSTALL_RPATH. This PR results in $ORIGIN and $ORIGIN/../lib being dropped from install rpaths. These rpaths are not desirable on certain distros where the libs are installed in default search locations, and lookup in the ld.so cache is better. Currently, users cannot opt out of these rpaths. With this change, it's opt-in by passing -DCMAKE_INSTALL_RPATH=<install prefix>/lib if they install to non-standard location, like they would already do for every other CMake project out there.*

* if this project really insists on $ORIGIN type rpaths for locating libraries in hdf5's own install prefix, then I would suggest to make it a target property instead or use list(APPEND ...) instead of overriding the user value.

The reason for this changes is: the user knows better than the build system whether or not install rpaths are needed and what they should be. The build system does not know whether the install prefix is "non-standard" or if rpaths to dependencies are desirable. The rpath logic of HDF5 is opinionated, and has no opt-out, does not deal with dependencies like zlib / mpi / etc in non-standard directories, and is somewhat outdated w.r.t. macOS.

Issue ticket number (GitHub or JIRA)

Checklist before requesting a review

  • My code conforms to the guidelines in CONTRIBUTING.md
  • I made an entry in release_docs/RELEASE.txt (bug fixes, new features)
  • I added a test (bug fixes, new features)

@derobins derobins added Priority - 1. High 🔼 These are important issues that should be resolved in the next release Component - Build CMake, Autotools Type - Improvement Improvements that don't add a new feature or functionality labels Jan 24, 2025
@haampie haampie changed the title cmake: remove outdated rpath handling cmake: do not override cmake rpath defines Jan 24, 2025
@byrnHDF
Copy link
Contributor

byrnHDF commented Jan 24, 2025

Yes, there some holders from the early CMake 2.x beginnings that need revising. If everything works with the current CMake minimum, 3.18, I think this is a good idea.

Another note, is that we should really push to update the minimum CMake to at least 3.25 for the 2.0 release.

Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem sensible to me, although I'm not a CMake expert (yet ;-) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Build CMake, Autotools Priority - 1. High 🔼 These are important issues that should be resolved in the next release Type - Improvement Improvements that don't add a new feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants