-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fix osx-dynamic install names for updated dependent shared library ids #39889
Fix osx-dynamic install names for updated dependent shared library ids #39889
Conversation
Extending microsoft#39313 to fix issues such as microsoft#14785 with openssl where libssl wasn't pointing to the rpath fixed id of libcrypto.
@microsoft-github-policy-service agree company="Catapult" |
This comment was marked as outdated.
This comment was marked as outdated.
b0f9637
to
bfeaae4
Compare
Thanks for the feedback @m-kuhn I'm on it. |
Hi @m-kuhn,
Assuming the "library installed by another package" is installed with fix in this PR as-is, shouldn't it be depending on the fixed id as per #39313 instead of pointing to the "absolute path"? e.g. if I use this PR as-is for building
... I hit an issue testing it with |
Hi @derekcyruschow-catapult |
So this is before this PR change:
... and after this PR change:
So it seems to be similar to what I saw with Does that cover your concerns? |
Absolutely, thanks a lot for the tests. This looks good to me. |
I was unable to repro the original issue:
Can you explain how to demonstrate that this PR fixes the issue? Please mark "ready for review" when you've responded so I know to take a loot at this PR again. |
@JavierMatosD Try with libssl instead, which links with libcrypto. And with
Similar for ffmpeg in the related issue #39890 |
I've just pushed 5cfdd84 to escape regex characters in a file path (e.g. otherwise |
If the code needs such fixes, consider add such tests. |
is there any documentation on how to run these tests? |
https://learn.microsoft.com/en-us/vcpkg/produce/test-registry-ports-ado ? |
that's what I used
|
Is this only missing a test for the regex or something else? |
I think so - sorry I've not made the time to do this yet! |
Hi Any progress on this? I can confirm that this fixed issues for me where pkgconf was referring to paths from the Vcpkg build dir. I also have issues with ffmpeg linking to the build dir, and here the PR did not work for me.
Note the Adjusting the regexp to account for the addition I running macOS 14.6.1
Seems we are coupling quite closely to the free text output of otool. I have no idea how stable that will be. |
Tests provided in #41603 |
@derekcyruschow-catapult do these tests look good to you? |
Thanks so much for this @m-kuhn (and sorry for not responding to your comment last week)! I'll cherry-pick now. As for your issue with |
(cherry picked from commit c920acf)
Example output of `otool -D libFLAC++.dylib` might be: libFLAC++.dylib (architecture arm64): @rpath/libFLAC++.10.dylib So just trim lines containing a colon and don't bother trying to match: * the path of the binary which might have chars like '+' which need escaping; * whether it contains "(architecture arm64)" in the output.
@JavierMatosD as per your request to demonstrate the issue, a test has been added that fails without and succeeds with this change. |
@JavierMatosD can something be done to move this ahead? |
execute_process( | ||
COMMAND "${install_name_tool_cmd}" -change "${adjusted_old_id}" "${adjusted_new_id}" "${macho_file}" | ||
OUTPUT_QUIET | ||
ERROR_VARIABLE change_id_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check exit code? (Or COMMAND_ERROR_IS_FATAL ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm new to using COMMAND_ERROR_IS_FATAL
- if I add COMMAND_ERROR_IS_FATAL ANY
would that make the check for change_id_error
potentially redundant or I should have both checks in-case install_name_tool
returns a 0
exit code but still prints something out to stderr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I haven't used COMMAND_ERROR_IS_FATAL
yet either 😅. I was just looking at the docs to make sure I understood what you were doing and saw that there.
I don't know what "${install_name_tool_cmd}"
's behavior with respect to stderr
and errors is; despite the name, stderr
does not always mean 'all errors go here'. It originates from big iron in the 70s where you would have machine readable bits on and input and output tape, and anything you wanted to show to a human operator would be sent to a separate physical printer.
My comment is more like 'some of these commands' exit code is being checked and some of them aren't being checked, it seems like they should all be checked unless there is a specific reason not to'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied nitpicks.patch
to f41bc39 (one of the nitpicks already applied via code suggestion in another comment).
Would you be willing to apply this patch? I tried but PS D:\vcpkg> push-pr SBGSports:fix_osx_dynamic_dependent_install_names
ERROR: Permission to SBGSports/vcpkg.git denied to BillyONeal.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists. |
Co-authored-by: Billy O'Neal <[email protected]>
…vcpkg_fixup_macho_rpath_in_dir
applied Thanks! |
Thanks and sorry that took so long to review! |
Thank you for the review @BillyONeal and for the work @derekcyruschow-catapult ! |
Extending #39313 to fix issues such as #30805 with openssl where libssl wasn't pointing to the rpath fixed id of libcrypto.
Fixes #30805