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.

Sign up for GitHub

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

Fix the rpath with option - plugins fail without #5298

Merged
merged 10 commits into from
Feb 14, 2025
Merged

Fix the rpath with option - plugins fail without #5298

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
618fe55
Fix the rpath with option - plugins fail without
byrnHDF Feb 6, 2025
28e052e
Remove note belonging to different issue
byrnHDF Feb 6, 2025
998eed6
Append instead of overwriting.
byrnHDF Feb 6, 2025
4614bbb
Remove OPTION as appending to the list will not overwrite
byrnHDF Feb 7, 2025
3262728
Merge branch 'HDFGroup:develop' into develop-rpath-option
byrnHDF Feb 10, 2025
65363db
Correct release note text
byrnHDF Feb 10, 2025
df9f6ba
Only need loader_path
byrnHDF Feb 10, 2025
7472daa
Merge branch 'HDFGroup:develop' into develop-rpath-option
byrnHDF Feb 11, 2025
0f0dde7
Merge changes from develop
byrnHDF Feb 14, 2025
36aeb68
Merge branch 'develop' into develop-rpath-option
byrnHDF Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
"ZLIB_USE_LOCALCONTENT": "OFF",
"LIBAEC_USE_LOCALCONTENT": "OFF",
"HDF5_USE_ZLIB_STATIC": "ON",
"HDF5_USE_LIBAEC_STATIC": "ON"
"HDF5_USE_LIBAEC_STATIC": "ON",
"HDF5_ENABLE_SZIP_SUPPORT": "ON",
"HDF5_ENABLE_ZLIB_SUPPORT": "ON"
}
},
{
Expand Down
23 changes: 23 additions & 0 deletions config/cmake/HDF5Macros.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,29 @@ macro (H5_SET_LIB_OPTIONS libtarget libname libtype libpackage)
endif ()
endif ()
HDF_SET_LIB_OPTIONS (${libtarget} ${LIB_OUT_NAME} ${libtype})

#-- Apple Specific install_name for libraries
if (APPLE)
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"
Copy link
Contributor

@haampie haampie Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

BUILD_WITH_INSTALL_RPATH ${HDF5_BUILD_WITH_INSTALL_NAME}
Copy link
Contributor

@haampie haampie Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why BUILD_WITH_INSTALL_RPATH?

Copy link
Contributor Author

@byrnHDF byrnHDF Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@haampie haampie Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

)
endif ()
if (HDF5_BUILD_FRAMEWORKS)
if (${libtype} MATCHES "SHARED")
# adapt target to build frameworks instead of dylibs
set_target_properties(${libtarget} PROPERTIES
XCODE_ATTRIBUTE_INSTALL_PATH "@rpath"
FRAMEWORK TRUE
FRAMEWORK_VERSION ${HDF5_PACKAGE_VERSION_MAJOR}
MACOSX_FRAMEWORK_IDENTIFIER org.hdfgroup.${libtarget}
MACOSX_FRAMEWORK_SHORT_VERSION_STRING ${HDF5_PACKAGE_VERSION_MAJOR}
MACOSX_FRAMEWORK_BUNDLE_VERSION ${HDF5_PACKAGE_VERSION_MAJOR})
endif ()
endif ()
endif ()
endmacro ()

# Initialize the list of VFDs to be used for testing and create a test folder for each VFD
Expand Down
12 changes: 12 additions & 0 deletions config/cmake/HDFMacros.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,18 @@ macro (HDF_DIR_PATHS package_prefix)
endif ()
message(STATUS "Final: ${${package_prefix}_INSTALL_DOC_DIR}")

# Append the needed INSTALL_RPATH for HDF Standard binary packages
if (APPLE)
list(APPEND CMAKE_INSTALL_RPATH
"@executable_path/../${${package_prefix}_INSTALL_LIB_DIR}"
"@executable_path/"
Copy link
Contributor

@haampie haampie Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@byrnHDF byrnHDF Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@haampie haampie Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay - will do.

"@loader_path/../${${package_prefix}_INSTALL_LIB_DIR}"
"@loader_path/"
)
else ()
list(APPEND CMAKE_INSTALL_RPATH "\$ORIGIN/../${${package_prefix}_INSTALL_LIB_DIR}:\$ORIGIN/")
endif ()

if (DEFINED ADDITIONAL_CMAKE_PREFIX_PATH AND EXISTS "${ADDITIONAL_CMAKE_PREFIX_PATH}")
set (CMAKE_PREFIX_PATH ${ADDITIONAL_CMAKE_PREFIX_PATH} ${CMAKE_PREFIX_PATH})
endif ()
Expand Down
6 changes: 6 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,12 @@ Bug Fixes since HDF5-2.0.0 release

Configuration
-------------
- The RPATH settings were restored by appending to the CMAKE_INSTALL_RPATH

The RPATH settings were removed by a pull-request #5271, but the settings are needed
under certain conditions. The settings have been restored by appending the necessary paths
and will not override and existing settings.

- Use pre-installed libaec compression library

The CMake logic for finding the libaec compression library has been
Expand Down
Loading