[vcpkg] Fix Mach-O RPATH duplicate or empty values on macOS #41146
Conversation
FrankXie05
added
the
category:tool-update
|
I'm kind of confused by the changes but that could be because I know very little CMake syntax and have only a vague idea of what RPATHs are. But assuming the original code was correct, the idea was to call Then the optimized code aimed to call In the proposed changes I see: What if The more I study it, I wonder if the optimized version was fine, with just a small mistake causing the warnings as described in #41122. Specifically here probably: If I write some simple test code like: It prints Or however else you're supposed to do it in CMake. Perhaps @m-kuhn or @autoantwort have some thoughts since they've worked on this part before? |
|
I would appreciate to see a specific test. |
|
@jdpurcell Clarification on the empty rpath_list: You’re right — in the original logic, if the Handling of empty rpath_args: I also addressed the issue where empty Optimized install_name_tool invocation: As you mentioned, the optimized version aimed to batch the |
|
We can do a simulation to help observe and reason about the behavior. CMakeLists.txt: set(macho_files "a;b;c;d;e;f")
foreach(macho_file IN LISTS macho_files)
# The correct/target rpath value
set(new_rpath "correct")
# The rpath values currently present on macho_file
if (macho_file STREQUAL "a")
set(rpath_list "")
elseif (macho_file STREQUAL "b")
set(rpath_list "wrong1")
elseif (macho_file STREQUAL "c")
set(rpath_list "correct")
elseif (macho_file STREQUAL "d")
set(rpath_list "wrong1" "wrong2")
elseif (macho_file STREQUAL "e")
set(rpath_list "wrong1" "correct")
elseif (macho_file STREQUAL "f")
set(rpath_list "correct" "wrong1")
endif()
# TODO: Compute arguments
execute_process(
COMMAND ./print_args.sh ${rpath_args} "${macho_file}"
)
endforeach()print_args.sh: #!/bin/bash
echo "Arguments: $@"Make sure we can execute it: Now let's replace the list(FIND rpath_list "${new_rpath}" has_new_rpath)
if(NOT has_new_rpath EQUAL -1)
list(REMOVE_AT rpath_list ${has_new_rpath})
set(rpath_args)
else()
set(rpath_args -add_rpath "${new_rpath}")
endif()
foreach(rpath IN LISTS rpath_list)
list(APPEND rpath_args "-delete_rpath" "${rpath}")
endforeach()
if(rpath_args STREQUAL "")
continue()
endif()Run it: And observe the output: Everything looks correct to me, with the exception of file "c", which should have skipped executing the command because no rpath changes were needed. Simply adjusting the final if("${rpath_args}" STREQUAL "")Produces the output I would expect, with the processing of file "c" skipped: If the other changes in this PR are run in the simulation, several issues are revealed:
So I don't think any changes are needed aside from correcting the one # Compute arguments
set(rpath_args "")
set(found_new_rpath FALSE)
foreach(rpath IN LISTS rpath_list)
if (rpath STREQUAL new_rpath)
set(found_new_rpath TRUE)
else()
list(APPEND rpath_args "-delete_rpath" "${rpath}")
endif()
endforeach()
if(NOT found_new_rpath)
list(APPEND rpath_args "-add_rpath" "${new_rpath}")
endif()
if("${rpath_args}" STREQUAL "")
continue()
endif()This is a few lines longer than the code it would replace, but I find it a bit easier to read and understand. Running through the simulation, it produces the expected output, just with the arguments in a different order (i.e. the adds come after the removes), which should work fine. |
|
@jdpurcell Thanks again for your help! :) |
JavierMatosD
added
the
requires:vcpkg-team-review
|
@BillyONeal, @ras0219-msft , @AugP, @data-queue and I discussed this today. This looks reasonable, but it needs a test that shows it fails today and not with these changes.
|
I think this will need a different kind of test, similar to |
Test results: |
|
what I meant is: to test this fix, it will require a test which builds binaries that show this problem (and not "just" a test that checks some strings produced by helper cmake functions) Some affected ports
|
|
The code is more or less the same as before, but it fixes the following bug in the existing code: set(test_var)
if (test_var STREQUAL "")
message(STATUS "test_var is empty")
else()
message(STATUS "test_var is not empty")
endif()outputs "test_var is not empty". So it would be enough to change if(rpath_args STREQUAL "")to if(NOT rpath_args) |
|
I'm closing this because I merged the competing fix #41443 . Thanks for your contribution to vcpkg! |
I think this problem should be reducible to string manipulation testing. However @autoantwort's 1 line fix seemed like a much more targeted fix so I merged it despite not much testing. |
Fix #41122
SHA512s are updated for each updated download.The "supports" clause reflects platforms that may be fixed by this new version.Any fixed CI baseline entries are removed from that file.Any patches that are no longer applied are deleted from the port's directory.When updating the upstream version, the"port-version"is reset (removed fromvcpkg.json).The version database is fixed by rerunning./vcpkg x-add-version --alland committing the result.Only one version is added to each modified port's versions file.This PR fixes an issue where
install_name_tooltried to add an RPATH that already existed in some Mach-O files or was empty. This issue caused warnings in thevcpkg_fixup_macho_rpathstep when processing files likecldr-plurals.Changes:
install_name_toolif there is an RPATH to remove, avoiding unnecessary operations. Avoid empty warnings.cc @autoantwort