Fixup Macho-O rpath for osx-dynamic #39313

m-kuhn · 2024-06-16T06:56:25Z

This sets install name id and rpath on shared libraries and executables for macos using *-osx-* triplets.

It is supposed to solve general problems like #31719, continuing on #31720, also referring #23035

Fixed ports also include:

vcpkg install proj:x64-osx-dynamic, executes the (installed) sqlite3 tool during build
vcpkg install gdk-pixbuf:x64-osx-dynamic, executes a helper tool within the build folder

What it does:

Fixes install name on itself to be with @rpath prefix
It makes sure that lib is added to rpath (or debug/lib for anything within the debug folder).

Open question: is this a problem and should it be required to also lookup libraries in other folders (apart from system libraries)? If yes, for which port?

Supersedes #36556, follows the rpath style of linux as suggested in #36556 (comment) .

The fixup can be disabled by changing set(VCPKG_FIXUP_MACHO_RPATH OFF) in the triplet.

dg0yt · 2024-06-16T16:14:19Z

#39319 should be the corresponding CI test to prevent regressions: Succeeds for linux now but fails for osx. Must succeed for osx if #39313 is right. I only tested linux ;-)

dg0yt · 2024-06-17T04:24:15Z

NOT category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. .
Ports may use dynamic linkage also in static triplets. That's why rpath fixup is already enabled in x64-linux.

m-kuhn · 2024-06-17T08:23:46Z

#39319 should be the corresponding CI test to prevent regressions: Succeeds for linux now but fails for osx. Must succeed for osx if #39313 is right. I only tested linux ;-)

Thanks for this.
It fails with:

CMake Error at scripts/test_ports/rpath-test/portfile.cmake:21 (message):
  Actual: 'release', expected: 'debug'
Call Stack (most recent call first):
  scripts/ports.cmake:176 (include)

It breaks with the assumption that tools will always link to release libraries. Do I have to forget this assumption?

Also, as far as I can see on the installed rpath-test-tool the rpath is not visible, is the debug installation path in tools by convention or should this be burnt into the binary somehow?

dg0yt · 2024-06-17T08:32:06Z

I'm afraid the test is right: Tools from the debug config must link the debug libs. That's why it is designed this way, running an executable from an installed (test) port which loads a shared lib.

It breaks with the assumption that tools will always link to release libraries. Do I have to forget this assumption?

It is not so easy: Binaries in tools/<PORT>/debug normally come from the debug config, i.e. they were build against link libs from debug. So they must also use the runtime from the debug config.

In particular, the autotools helper functions redirect debug binaries to tools/<PORT>/debug/bin.

Everything else is left mostly undefined in vcpkg. #17607

scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake

m-kuhn · 2024-06-17T09:17:08Z

It is not so easy: Binaries in tools//debug normally come from the debug config, i.e. they were build against link libs from debug. So they must also use the runtime from the debug config.

If tools/<PORT>/debug is a convention I can follow, I can implement the changes

dg0yt · 2024-06-17T09:59:28Z

How much does z_vcpkg_calculate_corrected_macho_rpath really need to differ from z_vcpkg_calculate_corrected_rpath, apart from $ORIGIN -> @loader_path?
(Cannot really think about it right now.)

m-kuhn · 2024-06-17T10:04:32Z

How much does z_vcpkg_calculate_corrected_macho_rpath really need to differ from z_vcpkg_calculate_corrected_rpath, apart from $ORIGIN -> @loader_path? (Cannot really think about it right now.)

Good thought. Minimal. It doesn't take any : path separation into account.

m-kuhn · 2024-06-17T11:31:56Z

#39319 should be the corresponding CI test to prevent regressions: Succeeds for linux now but fails for osx. Must succeed for osx if #39313 is right. I only tested linux ;-)

Thanks for this. It fails with:
CMake Error at scripts/test_ports/rpath-test/portfile.cmake:21 (message):
  Actual: 'release', expected: 'debug'
Call Stack (most recent call first):
  scripts/ports.cmake:176 (include)
It breaks with the assumption that tools will always link to release libraries. Do I have to forget this assumption?

Also, as far as I can see on the installed rpath-test-tool the rpath is not visible, is the debug installation path in tools by convention or should this be burnt into the binary somehow?

FTR: fixed

sharadhr · 2024-06-24T15:38:59Z

@m-kuhn, thanks for continuing to work on this; much appreciated.

Could the portfile variable documentation be updated simultaneously as well? I presume an entry VCPKG_FIXUP_MACHO_RPATH needs to be added. It'd be good to also have a short paragraph detailing the motivation for this variable, along with the default behaviour on macOS (always enabled, unless switched off).

m-kuhn · 2024-06-24T16:11:44Z

@sharadhr Good idea! Do you want to open a PR directly?

sharadhr · 2024-06-24T19:31:26Z

@m-kuhn Sure, I can do that! The documentation is in a different repository, so I'll have to link in this change too.

BillyONeal

@data-queue @JavierMatosD I know you both use macOS a lot of the time, can you check that this behavior seems reasonable to you?

I'm marking this 'request changes' only because EXCLUDE REGEX "\\\. seems wrong to me and my other comments are kinda nitpicks.

BillyONeal · 2024-06-25T01:19:29Z

scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake

+    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
+      )


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

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.

scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake

triplets/community/arm64-osx-dynamic.cmake

scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake

scripts/test_ports/rpath-test-binaries/project/main.cpp

scripts/test_ports/vcpkg-fixup-macho-rpath/portfile.cmake

BillyONeal · 2024-06-25T01:36:09Z

scripts/test_ports/vcpkg-fixup-macho-rpath/vcpkg.json

+  "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"


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

Follows the pattern of the linux/elf equivalent

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

Co-authored-by: Billy O'Neal <[email protected]>

m-kuhn · 2024-06-25T04:55:07Z

Thank you for the review @BillyONeal I have incorporated the suggestions.

sharadhr · 2024-06-25T10:17:34Z

scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake

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


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

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?

If it works reliably, great! No further questions.

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.

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

Sorry for the delay. Done #39890

JavierMatosD

@data-queue @JavierMatosD I know you both use macOS a lot of the time, can you check that this behavior seems reasonable to you?

@BillyONeal. Not a full review but this looks good to me. otool and install_name_tool should be available (or at least highly likely to be available) if either Xcode or Command Line Tools is installed.

I'm marking this 'request changes' only because EXCLUDE REGEX "\. seems wrong to me and my other comments are kinda nitpicks.

This is no longer in the PR :)

Seems like your feedback was addressed :)

BillyONeal · 2024-06-27T21:26:27Z

Thanks for the new feature!

Extending microsoft#39313 to fix issues such as microsoft#14785 with openssl where libssl wasn't pointing to the rpath fixed id of libcrypto.

#8) Extending microsoft#39313 to fix issues such as microsoft#14785 with openssl where libssl wasn't pointing to the rpath fixed id of libcrypto. Co-authored-by: Derek Cyrus-Chow <[email protected]>

Fix Macho-O rpath for osx-dynamic

8a3d437

m-kuhn mentioned this pull request Jun 16, 2024

[scripts|cmake] Fix up install name and rpath on osx dynamic #36556

Closed

dg0yt mentioned this pull request Jun 16, 2024

[rpath-test,rpath-test-binaries] Validate rpath fixup #39319

Merged

LilyWangLL assigned MonicaLiu0311 Jun 17, 2024

MonicaLiu0311 added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. labels Jun 17, 2024

MonicaLiu0311 removed the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label Jun 17, 2024

[rpath-test,rpath-test-binaries] Validate rpath fixup

d6501c9

dg0yt reviewed Jun 17, 2024

View reviewed changes

scripts/cmake/z_vcpkg_fixup_rpath_macho.cmake Show resolved Hide resolved

Fix rpath for debug tools and add unit tests

82e282d

m-kuhn force-pushed the fix-macho-rpath branch from a59471a to 82e282d Compare June 17, 2024 10:05

Merge remote-tracking branch 'origin/master' into HEAD

fc9c3a7

MonicaLiu0311 previously approved these changes Jun 24, 2024

View reviewed changes

MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jun 24, 2024

sharadhr mentioned this pull request Jun 24, 2024

Add VCPKG_FIXUP_MACHO_RPATH microsoft/vcpkg-docs#346

Merged

BillyONeal previously requested changes Jun 25, 2024

View reviewed changes

MonicaLiu0311 removed the info:reviewed Pull Request changes follow basic guidelines label Jun 25, 2024

Fix regex

1f94843

Co-authored-by: Billy O'Neal <[email protected]>

m-kuhn dismissed MonicaLiu0311’s stale review via 1f94843 June 25, 2024 04:31

Address review comments

8a79bec

m-kuhn mentioned this pull request Jun 25, 2024

Python: Bump minimal python version to 3.10 qgis/QGIS#56499

Closed

sharadhr reviewed Jun 25, 2024

View reviewed changes

JavierMatosD approved these changes Jun 27, 2024

View reviewed changes

BillyONeal merged commit 874fff8 into microsoft:master Jun 27, 2024
17 checks passed

m-kuhn deleted the fix-macho-rpath branch June 27, 2024 22:00

MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jun 28, 2024

vserdyuk mentioned this pull request Jul 9, 2024

[qtbase] Build error on x64-osx with Qt customized to dynamic in triplet #39805

Open

This was referenced Jul 12, 2024

Fix osx-dynamic install names for updated dependent shared library ids #39889

Open

[openssl] on *-osx-dynamic triplets don't have install_name set properly #30805

Closed

aabellagm mentioned this pull request Jul 12, 2024

Fixup macOS libraries dependent path for dynamic libs #39890

Open

petersteneteg mentioned this pull request Aug 15, 2024

[vtk] vtkWrapHierarchy have wrong rpath after move into tools folder #39060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup Macho-O rpath for osx-dynamic #39313

Fixup Macho-O rpath for osx-dynamic #39313

m-kuhn commented Jun 16, 2024 •

edited

Loading

dg0yt commented Jun 16, 2024

dg0yt commented Jun 17, 2024 •

edited

Loading

m-kuhn commented Jun 17, 2024

dg0yt commented Jun 17, 2024

m-kuhn commented Jun 17, 2024

dg0yt commented Jun 17, 2024

m-kuhn commented Jun 17, 2024

m-kuhn commented Jun 17, 2024

sharadhr commented Jun 24, 2024 •

edited

Loading

m-kuhn commented Jun 24, 2024

sharadhr commented Jun 24, 2024

BillyONeal left a comment

BillyONeal Jun 25, 2024

m-kuhn Jun 25, 2024

BillyONeal Jun 25, 2024

m-kuhn Jun 25, 2024

dg0yt Jun 25, 2024

m-kuhn commented Jun 25, 2024

sharadhr Jun 25, 2024 •

edited

Loading

m-kuhn Jun 25, 2024

sharadhr Jun 25, 2024

aabellagm Jul 4, 2024

m-kuhn Jul 5, 2024

aabellagm Jul 12, 2024 •

edited

Loading

JavierMatosD left a comment •

edited

Loading

BillyONeal commented Jun 27, 2024

Fixup Macho-O rpath for osx-dynamic #39313

Fixup Macho-O rpath for osx-dynamic #39313

Conversation

m-kuhn commented Jun 16, 2024 • edited Loading

dg0yt commented Jun 16, 2024

dg0yt commented Jun 17, 2024 • edited Loading

m-kuhn commented Jun 17, 2024

dg0yt commented Jun 17, 2024

m-kuhn commented Jun 17, 2024

dg0yt commented Jun 17, 2024

m-kuhn commented Jun 17, 2024

m-kuhn commented Jun 17, 2024

sharadhr commented Jun 24, 2024 • edited Loading

m-kuhn commented Jun 24, 2024

sharadhr commented Jun 24, 2024

BillyONeal left a 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

m-kuhn commented Jun 25, 2024

sharadhr Jun 25, 2024 • 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

Choose a reason for hiding this comment

aabellagm Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

JavierMatosD left a comment • edited Loading

Choose a reason for hiding this comment

BillyONeal commented Jun 27, 2024

m-kuhn commented Jun 16, 2024 •

edited

Loading

dg0yt commented Jun 17, 2024 •

edited

Loading

sharadhr commented Jun 24, 2024 •

edited

Loading

sharadhr Jun 25, 2024 •

edited

Loading

aabellagm Jul 12, 2024 •

edited

Loading

JavierMatosD left a comment •

edited

Loading