Fix the rpath with option - plugins fail without #5298

byrnHDF · 2025-02-06T22:18:12Z

Fixes issue #5297 Removal of rpath breaks plugins by restoring the necessary rpath changes

jhendersonHDF · 2025-02-06T22:24:47Z

config/cmake/HDFMacros.cmake

@@ -458,6 +458,21 @@ macro (HDF_DIR_PATHS package_prefix)
  endif ()
  message(STATUS "Final: ${${package_prefix}_INSTALL_DOC_DIR}")

+  # Define the needed INSTALL_RPATH for HDF Standard binary packages
+  option (HDF5_SET_DEFAULT_RPATH "Set the default RPATH for the common installation" OFF)


Why again with even more options? Better solutions were given in the related issue in the first place, including never overriding CMAKE_INSTALL_RPATH.

Because it would require multiple options with STRING attributes to be on the command line when one option just does it correctly.

I get the convenience aspect, but really the command-line options should pretty much be the standard way. Plus, if you turn this option on we're back to overriding what may have been desired to be passed in from the user, in addition to what we're setting.

Are we really sure that modifying the rpath is absolutely necessary and that we aren't doing something wrong here in the first place?

Yes, modifying the rpath gives the app/lib an expected search path

jhendersonHDF · 2025-02-06T22:27:38Z

config/cmake/cacheinit.cmake

@@ -43,6 +43,8 @@ set (HDF5_MINGW_STATIC_GCC_LIBS ON CACHE BOOL "Statically link libgcc/libstdc++"
 set (HDF5_ALLOW_EXTERNAL_SUPPORT "TGZ" CACHE STRING "Allow External Library Building (NO GIT TGZ)" FORCE)
 set_property (CACHE HDF5_ALLOW_EXTERNAL_SUPPORT PROPERTY STRINGS NO GIT TGZ)

+set (HDF5_SET_DEFAULT_RPATH ON "Set the default RPATH for the common installation" FORCE)


Presumably this is just going to be a problem for ccmake builds since the cacheinit file isn't necessarily going to be used.

Why? ccmake could use the presets file and I will have a a followup PR that adds the dependent_option form to all options that are behind another option.

I'd say for the average user building from source there really is no reason to use the presets, cacheinit, etc. files. The default configuration you get when just running ccmake on a build directory should be the happy path.

The defaults should build a limited but workable solution. cacheinit.cmake is mostly for our testing as the settings match our expected/typical build. presets are useful for use cases.

release_docs/RELEASE.txt

haampie · 2025-02-08T08:14:58Z

config/cmake/HDF5Macros.cmake

+    option (HDF5_BUILD_WITH_INSTALL_NAME "Build with library install_name set to the installation path" OFF)
+    if (HDF5_BUILD_WITH_INSTALL_NAME)
+      set_target_properties (${libtarget} PROPERTIES
+          INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}/lib"


This is not immediately related to the issue, is it? The default install name as of CMake 3.0 is @rpath/lib<name>.dylib (before it was lib<name>.dylib). When users link against the library, it copies the install name @rpath/lib<name>.dylib as a load command, so rpaths set by the user in the dependent executable/library are respected when searching for libhdf5.dylib at runtime. Then the situation is exactly the same as on Linux, where users also have to set an rpath. If you set it to an absolute path, then rpaths no longer work, and the library is always opened directly by absolute path (not relocatable). So, bit unclear why this is added now, and why the apple situation should be different from linux.

This was added as it was previously removed - can be removed if some apple developer knows this is not needed.

haampie · 2025-02-08T08:15:21Z

config/cmake/HDF5Macros.cmake

+    if (HDF5_BUILD_WITH_INSTALL_NAME)
+      set_target_properties (${libtarget} PROPERTIES
+          INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}/lib"
+          BUILD_WITH_INSTALL_RPATH ${HDF5_BUILD_WITH_INSTALL_NAME}


Why BUILD_WITH_INSTALL_RPATH?

Definition:

BUILD_WITH_INSTALL_RPATH is a boolean specifying whether to link the target in the build tree with the INSTALL_RPATH. This takes precedence over SKIP_BUILD_RPATH and avoids the need for relinking before installation.

This property is initialized by the value of the CMAKE_BUILD_WITH_INSTALL_RPATH variable if it is set when a target is created.

If policy CMP0068 is not NEW, this property also controls use of INSTALL_NAME_DIR in the build tree on macOS. Either way, the BUILD_WITH_INSTALL_NAME_DIR target property takes precedence.

Eliminating this option then just forces the change onto the user?

I guess the idea of adding these lines was to use $ORIGIN/.. style relative rpaths also during the build? (But then why if (APPLE)?) That I think is redundant, cause cmake sets build specific rpaths for any target you're linking, including the path to just-built libhdf5.dylib. CMake knows better where executables and libraries are located relatively during the build, and it may differ from where they are upon install, so using install rpath during the build is not helpful.

If you don't set anything related to build rpaths, and you need to run a just-built executable as part of the build (before install), cmake ensures that these executables can find their libraries. If you meddle with it, then maybe not.

I think the only case where you may benefit from setting build rpaths yourself is when you run just-built executables during the build that dlopen(...) just-built libraries that cmake is not aware of as part of target_link_libraries.

Like the plugins.

I'm not aware of it's use by us in our CI. However, I also don't develop with macs either.
It is one of those long-ago implemented options, that just get brought along. I put it back, along with the other code to fix CI and plugins, but maybe it wasn't necessary?

haampie · 2025-02-08T08:18:52Z

config/cmake/HDFMacros.cmake

+        "@executable_path/../${${package_prefix}_INSTALL_LIB_DIR}"
+        "@executable_path/"


I think @executable_path is redundant. You want to resolve libraries relative to the current binary right? Then @loader_path should suffice.

In general, @loader_path is preferred over @executable_path, as it allows embedded frameworks to work in both an executable and a bundle, plugin, or sub-framework. The only downside is that @loader_path requires 10.4 or newer. If you're on 10.5 or newer, @rpath is even better than @loader_path.

I ran across this statement, which seems to just make things more confusing! Even though we only test on the latest two versions of MAC, forcing a single setting could cause someone problems. However, 10.5 is pretty old and maaybe it should just be @rpath?

Hm, I don't think the statement is entirely correct, in the sense that the one does not replace the other 🤔.

If you have libhdf5.dylib, the cmake "trick" is to set its install name to @rpath/libhdf5.dylib.

Then an executable or library that links against libhdf5.dylib will register @rpath/libhdf5.dylib as a needed library (LC_LOAD_DYLIB load command). If it additionally sets @loader_path/../lib as an rpath (LC_RPATH load command), then that value may be substituted for @rpath.

That way, after substitution: @rpath/libhdf5.dylib -> @loader_path/../lib/libhdf5.dylib -> $(dirname /path/to/binary)/../lib/libhdf5.dylib is a search path.

The main difference between linux and macOS is that for rpaths to "work" you need to opt-in by marking the needed library as @rpath/<name>. Otherwise I think @loader_path can be taken as equivalent to $ORIGIN on Linux. CMake enables this "opt-in" by default. And yeah, macOS 10.14 is pretty much just for retro apple machines.

So the suggestion then is to only use "@loader_path" ?

Yeah, that + requiring cmake 3.0 should give equivalent behavior as on linux.

Okay - will do.

lrknox · 2025-02-12T22:37:46Z

Builds and passes tests with zlib, libaec and hdf5_plugins on macOS 15.3.

byrnHDF · 2025-02-13T22:01:53Z

Just ran across this somewhat related question on the CMake discourse. https://discourse.cmake.org/t/question-re-cmake-build-with-install-rpath/13568
Might be just a Ninja issue, though.

byrnHDF added 2 commits February 6, 2025 16:10

Fix the rpath with option - plugins fail without

618fe55

Remove note belonging to different issue

28e052e

byrnHDF self-assigned this Feb 6, 2025

byrnHDF requested review from lrknox, derobins, fortnern, jhendersonHDF, qkoziol, vchoi-hdfgroup, bmribler, glennsong09, mattjala and brtnfld as code owners February 6, 2025 22:18

jhendersonHDF reviewed Feb 6, 2025

View reviewed changes

byrnHDF added 2 commits February 6, 2025 17:26

Append instead of overwriting.

998eed6

Remove OPTION as appending to the list will not overwrite

4614bbb

lrknox reviewed Feb 7, 2025

View reviewed changes

release_docs/RELEASE.txt Outdated Show resolved Hide resolved

haampie reviewed Feb 8, 2025

View reviewed changes

byrnHDF and others added 4 commits February 10, 2025 07:56

Merge branch 'HDFGroup:develop' into develop-rpath-option

3262728

Correct release note text

65363db

Only need loader_path

df9f6ba

Merge branch 'HDFGroup:develop' into develop-rpath-option

7472daa

lrknox previously approved these changes Feb 12, 2025

View reviewed changes

lrknox mentioned this pull request Feb 13, 2025

Use only one find_package call for zlib. #5280

Merged

Merge changes from develop

0f0dde7

byrnHDF dismissed lrknox’s stale review via 0f0dde7 February 14, 2025 17:42

Merge branch 'develop' into develop-rpath-option

36aeb68

lrknox approved these changes Feb 14, 2025

View reviewed changes

jhendersonHDF approved these changes Feb 14, 2025

View reviewed changes

lrknox merged commit d4200dc into HDFGroup:develop Feb 14, 2025
77 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the rpath with option - plugins fail without #5298

Fix the rpath with option - plugins fail without #5298

byrnHDF commented Feb 6, 2025

jhendersonHDF Feb 6, 2025

byrnHDF Feb 6, 2025

jhendersonHDF Feb 6, 2025

byrnHDF Feb 13, 2025

jhendersonHDF Feb 6, 2025

byrnHDF Feb 6, 2025

jhendersonHDF Feb 6, 2025

byrnHDF Feb 13, 2025

haampie Feb 8, 2025 •

edited

Loading

byrnHDF Feb 13, 2025

haampie Feb 8, 2025 •

edited

Loading

byrnHDF Feb 10, 2025 •

edited

Loading

haampie Feb 10, 2025 •

edited

Loading

byrnHDF Feb 10, 2025

haampie Feb 8, 2025 •

edited

Loading

byrnHDF Feb 10, 2025 •

edited

Loading

haampie Feb 10, 2025 •

edited

Loading

byrnHDF Feb 10, 2025

haampie Feb 10, 2025

byrnHDF Feb 10, 2025

lrknox commented Feb 12, 2025

byrnHDF commented Feb 13, 2025

		"@executable_path/../${${package_prefix}_INSTALL_LIB_DIR}"
		"@executable_path/"

Fix the rpath with option - plugins fail without #5298

Fix the rpath with option - plugins fail without #5298

Conversation

byrnHDF commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

byrnHDF Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

haampie Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

byrnHDF Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

haampie Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrknox commented Feb 12, 2025

byrnHDF commented Feb 13, 2025

haampie Feb 8, 2025 •

edited

Loading

haampie Feb 8, 2025 •

edited

Loading

byrnHDF Feb 10, 2025 •

edited

Loading

haampie Feb 10, 2025 •

edited

Loading

haampie Feb 8, 2025 •

edited

Loading

byrnHDF Feb 10, 2025 •

edited

Loading

haampie Feb 10, 2025 •

edited

Loading