Skip to content

Fixup Macho-O rpath for osx-dynamic #39313

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

Merged
BillyONeal merged 6 commits into from
Jun 27, 2024
Merged

Fixup Macho-O rpath for osx-dynamic #39313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a3d437
Fix Macho-O rpath for osx-dynamic
m-kuhn Jun 15, 2024
d6501c9
[rpath-test,rpath-test-binaries] Validate rpath fixup
dg0yt Jun 16, 2024
82e282d
Fix rpath for debug tools and add unit tests
m-kuhn Jun 17, 2024
fc9c3a7
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Jun 21, 2024
1f94843
Fix regex
m-kuhn Jun 25, 2024
8a79bec
Address review comments
m-kuhn Jun 25, 2024
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
154 changes: 154 additions & 0 deletions scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
function(z_vcpkg_calculate_corrected_macho_rpath)
Comment thread
m-kuhn marked this conversation as resolved.
cmake_parse_arguments(PARSE_ARGV 0 "arg"
""
"MACHO_FILE_DIR;OUT_NEW_RPATH_VAR"
"")

Comment thread
m-kuhn marked this conversation as resolved.
set(current_prefix "${CURRENT_PACKAGES_DIR}")
set(current_installed_prefix "${CURRENT_INSTALLED_DIR}")
file(RELATIVE_PATH relative_from_packages "${CURRENT_PACKAGES_DIR}" "${arg_MACHO_FILE_DIR}")
if("${relative_from_packages}/" MATCHES "^debug/")
set(current_prefix "${CURRENT_PACKAGES_DIR}/debug")
set(current_installed_prefix "${CURRENT_INSTALLED_DIR}/debug")
endif()

# compute path relative to lib
file(RELATIVE_PATH relative_to_lib "${arg_MACHO_FILE_DIR}" "${current_prefix}/lib")
# remove trailing slash
string(REGEX REPLACE "/+$" "" relative_to_lib "${relative_to_lib}")

if(NOT relative_to_lib STREQUAL "")
set(new_rpath "@loader_path/${relative_to_lib}")
else()
set(new_rpath "@loader_path")
endif()

set("${arg_OUT_NEW_RPATH_VAR}" "${new_rpath}" PARENT_SCOPE)
endfunction()

function(z_vcpkg_fixup_macho_rpath_in_dir)
# We need to iterate through everything because we
# can't predict where a Mach-O file will be located
file(GLOB root_entries LIST_DIRECTORIES TRUE "${CURRENT_PACKAGES_DIR}/*")

# Skip some folders for better throughput
list(APPEND folders_to_skip "include")
list(JOIN folders_to_skip "|" folders_to_skip_regex)
set(folders_to_skip_regex "^(${folders_to_skip_regex})$")

find_program(
install_name_tool_cmd
NAMES install_name_tool
DOC "Absolute path of install_name_tool cmd"
REQUIRED
)

find_program(
otool_cmd
NAMES otool
DOC "Absolute path of otool cmd"
REQUIRED
)

find_program(
file_cmd
NAMES file
DOC "Absolute path of file cmd"
REQUIRED
)
Comment on lines +43 to +62

@BillyONeal BillyONeal Jun 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are these and what does a user need to do if they aren't present?

@m-kuhn m-kuhn Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tools to modify and change dynamic libraries (install_name tool and otool) or inspect files (file tool).
In my experience they come pre-installed on macos systems, so I don't think "not present" is a real world scenario to take into account.


foreach(folder IN LISTS root_entries)
if(NOT IS_DIRECTORY "${folder}")
continue()
endif()

get_filename_component(folder_name "${folder}" NAME)
if(folder_name MATCHES "${folders_to_skip_regex}")
continue()
endif()

file(GLOB_RECURSE macho_files LIST_DIRECTORIES FALSE "${folder}/*")
list(FILTER macho_files EXCLUDE REGEX "\\\.(cpp|cc|cxx|c|hpp|h|hh|hxx|inc|json|toml|yaml|man|m4|ac|am|in|log|txt|pyi?|pyc|pyx|pxd|pc|cmake|f77|f90|f03|fi|f|cu|mod|ini|whl|cat|csv|rst|md|npy|npz|template|build)$")
Comment thread
m-kuhn marked this conversation as resolved.
Outdated
list(FILTER macho_files EXCLUDE REGEX "/(copyright|LICENSE|METADATA)$")

foreach(macho_file IN LISTS macho_files)
if(IS_SYMLINK "${macho_file}")
continue()
endif()

# Determine if the file is a Mach-O executable or shared library
execute_process(
COMMAND "${file_cmd}" -b "${macho_file}"
OUTPUT_VARIABLE file_output
OUTPUT_STRIP_TRAILING_WHITESPACE
)
if(file_output MATCHES ".*Mach-O.*shared library.*")
set(file_type "shared")
elseif(file_output MATCHES ".*Mach-O.*executable.*")
set(file_type "executable")
else()
continue()
Comment thread
m-kuhn marked this conversation as resolved.
endif()

get_filename_component(macho_file_dir "${macho_file}" DIRECTORY)
get_filename_component(macho_file_name "${macho_file}" NAME)

z_vcpkg_calculate_corrected_macho_rpath(
MACHO_FILE_DIR "${macho_file_dir}"
OUT_NEW_RPATH_VAR new_rpath
)
Comment thread
m-kuhn marked this conversation as resolved.

if("${file_type}" STREQUAL "shared")
# Set the install name for shared libraries
execute_process(
COMMAND "${install_name_tool_cmd}" -id "@rpath/${macho_file_name}" "${macho_file}"
OUTPUT_QUIET
ERROR_VARIABLE set_id_error
)
message(STATUS "Set install name id of '${macho_file}' (To '@rpath/${macho_file_name}')")
if(NOT "${set_id_error}" STREQUAL "")
message(WARNING "Couldn't adjust install name of '${macho_file}': ${set_id_error}")
continue()
endif()
endif()

# Clear all existing rpaths
execute_process(
COMMAND "${otool_cmd}" -l "${macho_file}"
OUTPUT_VARIABLE get_rpath_ov
RESULT_VARIABLE get_rpath_rv
)

if(NOT get_rpath_rv EQUAL 0)
message(FATAL_ERROR "Could not obtain rpath list from '${macho_file}'")
endif()
# Extract the LC_RPATH load commands and extract the paths
string(REGEX REPLACE "[^\n]+cmd LC_RPATH\n[^\n]+\n[^\n]+path ([^\n]+) \\(offset[^\n]+\n" "rpath \\1\n" get_rpath_ov "${get_rpath_ov}")
string(REGEX MATCHALL "rpath [^\n]+" get_rpath_ov "${get_rpath_ov}")
string(REGEX REPLACE "rpath " "" rpath_list "${get_rpath_ov}")

foreach(rpath IN LISTS rpath_list)
execute_process(
COMMAND "${install_name_tool_cmd}" -delete_rpath "${rpath}" "${macho_file}"

@sharadhr sharadhr Jun 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about the LC_LOAD_DYLIB field? How come we don't use install_name_tool -change here for dependent libraries?

@m-kuhn m-kuhn Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have tested this approach with a wide variety of ports (qt, qt5, even python via open-vcpkg) and it worked reliably.
Do you have a port which needs attention or do you want to provide a specific alternative implementation?

@sharadhr sharadhr Jun 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it works reliably, great! No further questions.

@aabellagm aabellagm Jul 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @m-kuhn
I've seen the PR merged and I give it a try.
I think it may be needed to take into account the LC_LOAD_DYLIB when it is pointing to other VCPKG dynamic libraries.

Here the output of a just built ffmpeg library, which depends on other ffmpeg libraries:

% otool -L libavcodec.60.31.102.dylib
libavcodec.60.31.102.dylib (architecture arm64):
	@rpath/libavcodec.60.31.102.dylib (compatibility version 60.0.0, current version 60.31.102)
	/Users/megadev/aag/vcpkg/packages/ffmpeg_arm64-osx-dynamic/lib/libswresample.4.dylib (compatibility version 4.0.0, current version 4.12.100)
	/Users/megadev/aag/vcpkg/packages/ffmpeg_arm64-osx-dynamic/lib/libavutil.58.dylib (compatibility version 58.0.0, current version 58.29.100)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1336.0.0)
	/System/Library/Frameworks/AudioToolbox.framework/Versions/A/AudioToolbox (compatibility version 1.0.0, current version 1000.0.0)
	/System/Library/Frameworks/VideoToolbox.framework/Versions/A/VideoToolbox (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 2048.1.255)
	/System/Library/Frameworks/CoreMedia.framework/Versions/A/CoreMedia (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreVideo.framework/Versions/A/CoreVideo (compatibility version 1.2.0, current version 1.5.0)
	/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1226.0.0)

We can see that the dependencies of the library have a path to the packages directory. I guess the LC_LOAD_DYLIBs should be @rpath/<lib>.

Sadly I didn't have time before to test it before merging. So sorry.

@m-kuhn m-kuhn Jul 5, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback, much appreciated. I can reproduce this here.
Can you open an issue, so it's not forgotten?

@aabellagm aabellagm Jul 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay. Done #39890

OUTPUT_QUIET
ERROR_VARIABLE delete_rpath_error
)
message(STATUS "Remove RPATH from '${macho_file}' ('${rpath}')")
endforeach()

# Set the new rpath
execute_process(
COMMAND "${install_name_tool_cmd}" -add_rpath "${new_rpath}" "${macho_file}"
OUTPUT_QUIET
ERROR_VARIABLE set_rpath_error
)

if(NOT "${set_rpath_error}" STREQUAL "")
message(WARNING "Couldn't adjust RPATH of '${macho_file}': ${set_rpath_error}")
continue()
endif()

message(STATUS "Adjusted RPATH of '${macho_file}' (To '${new_rpath}')")
endforeach()
endforeach()
endfunction()
6 changes: 5 additions & 1 deletion scripts/ports.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ include("${SCRIPTS}/cmake/z_vcpkg_prettify_command_line.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_setup_pkgconfig_path.cmake")

include("${SCRIPTS}/cmake/z_vcpkg_fixup_rpath.cmake")
include("${SCRIPTS}/cmake/z_vcpkg_fixup_rpath_macho.cmake")

function(debug_message)
if(PORT_DEBUG)
Expand Down Expand Up @@ -174,10 +175,13 @@ if(CMD STREQUAL "BUILD")

include("${CURRENT_PORT_DIR}/portfile.cmake")
if(DEFINED PORT)
# Always fixup RPATH on linux unless explicitly disabled.
# Always fixup RPATH on linux and osx unless explicitly disabled.
if(VCPKG_FIXUP_ELF_RPATH OR (VCPKG_TARGET_IS_LINUX AND NOT DEFINED VCPKG_FIXUP_ELF_RPATH))
z_vcpkg_fixup_rpath_in_dir()
endif()
if(VCPKG_FIXUP_MACHO_RPATH OR (VCPKG_TARGET_IS_OSX AND NOT DEFINED VCPKG_FIXUP_MACHO_RPATH))
z_vcpkg_fixup_macho_rpath_in_dir()
endif()
include("${SCRIPTS}/build_info.cmake")
endif()
elseif(CMD STREQUAL "CREATE")
Expand Down
65 changes: 65 additions & 0 deletions scripts/test_ports/vcpkg-fixup-macho-rpath/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
set(VCPKG_POLICY_EMPTY_PACKAGE enabled)

# Test for empty string
set(macho_dir "${CURRENT_PACKAGES_DIR}/lib")
set(test_rpath "")
set(expected "@loader_path")

z_vcpkg_calculate_corrected_macho_rpath(
MACHO_FILE_DIR "${macho_dir}"
OUT_NEW_RPATH_VAR new_rpath
)

if(NOT "x${new_rpath}x" STREQUAL "x${expected}x")
message(FATAL_ERROR "--- Calculated rpath does not match expected rpath: '${new_rpath}' != '${expected}' ")
else()
message(STATUS "--- Calculated rpath matches expected rpath: '${new_rpath}' ")
endif()

# Test for empty string in the tools directory
set(macho_dir "${CURRENT_PACKAGES_DIR}/tools/hdf5")
set(test_rpath "")
set(expected "@loader_path/../../lib")

z_vcpkg_calculate_corrected_macho_rpath(
MACHO_FILE_DIR "${macho_dir}"
OUT_NEW_RPATH_VAR new_rpath
)

if(NOT "x${new_rpath}x" STREQUAL "x${expected}x")
message(FATAL_ERROR "--- Calculated rpath does not match expected rpath: '${new_rpath}' != '${expected}' ")
else()
message(STATUS "--- Calculated rpath matches expected rpath: '${new_rpath}' ")
endif()

# macho dir in subdir
set(macho_dir "${CURRENT_PACKAGES_DIR}/lib/somesubdir")
set(test_rpath "")
set(expected "@loader_path/..")

z_vcpkg_calculate_corrected_macho_rpath(
MACHO_FILE_DIR "${macho_dir}"
OUT_NEW_RPATH_VAR new_rpath
)

if(NOT "x${new_rpath}x" STREQUAL "x${expected}x")
message(FATAL_ERROR "--- Calculated rpath for '${macho_dir}' does not match expected rpath: '${new_rpath}' != '${expected}' ")
else()
message(STATUS "--- Calculated rpath matches expected rpath: '${new_rpath}' ")
endif()

# Getting more complex
set(macho_dir "${CURRENT_PACKAGES_DIR}/plugins/notlib/extrasubdir")
set(test_rpath "")
set(expected "@loader_path/../../../lib")

z_vcpkg_calculate_corrected_macho_rpath(
MACHO_FILE_DIR "${macho_dir}"
OUT_NEW_RPATH_VAR new_rpath
)

if(NOT "x${new_rpath}x" STREQUAL "x${expected}x")
message(FATAL_ERROR "--- Calculated rpath does not match expected rpath: '${new_rpath}' != '${expected}' ")
else()
message(STATUS "--- Calculated rpath matches expected rpath: '${new_rpath}' ")
endif()
Comment thread
m-kuhn marked this conversation as resolved.
Outdated
6 changes: 6 additions & 0 deletions scripts/test_ports/vcpkg-fixup-macho-rpath/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "vcpkg-fixup-macho-rpath",
"version-date": "2024-06-15",
"description": "Test port to check the string replacement in z_vcpkg_fixup_macho_rpath",
"supports": "native & osx"

@BillyONeal BillyONeal Jun 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test just does string replacements; shouldn't it work on all platforms rather than only osx?

@m-kuhn m-kuhn Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follows the pattern of the linux/elf equivalent

@dg0yt dg0yt Jun 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might want to migrate this to unit-test-cmake.
But it doesn't have to be done now.

}
2 changes: 2 additions & 0 deletions triplets/community/arm64-osx-dynamic.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)

set(VCPKG_CMAKE_SYSTEM_NAME Darwin)
set(VCPKG_OSX_ARCHITECTURES arm64)

set(VCPKG_FIXUP_MACHO_RPATH ON)
Comment thread
m-kuhn marked this conversation as resolved.
Outdated
2 changes: 2 additions & 0 deletions triplets/community/x64-osx-dynamic.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ set(VCPKG_LIBRARY_LINKAGE dynamic)

set(VCPKG_CMAKE_SYSTEM_NAME Darwin)
set(VCPKG_OSX_ARCHITECTURES x86_64)

set(VCPKG_FIXUP_MACHO_RPATH ON)