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.

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

BREAKING: Modify path layout to fix relative RPATH lookups #104

Merged
merged 14 commits into from
Jul 24, 2024

Conversation

fmeum
Copy link
Owner

@fmeum fmeum commented Jul 23, 2024

The executable of an executable transitioned rule is now created in a subdirectory of the target's package, which allows it to match the path depth under the execroot of the original executable and thus gets relative RPATH entries to resolve.

Also copy any DLLs next to the new executable on Windows.

The new path layout can be disabled via --no@with_cfg.bzl//:incompatible_same_depth_path_layout, e.g. if users hardcoded rlocationpaths and don't want to update them now. It is recommended to avoid this and use $(rlocationpath ...) instead.

Fixes #85

@fmeum fmeum force-pushed the tests-dynamic-deps branch 4 times, most recently from 75830d0 to e494d60 Compare July 24, 2024 09:56
@fmeum fmeum changed the title Test cc_binary dynamic_deps runfiles Modify path layout to fix relative RPATH lookups Jul 24, 2024
@fmeum fmeum changed the title Modify path layout to fix relative RPATH lookups BREAKING: Modify path layout to fix relative RPATH lookups Jul 24, 2024
@fmeum
Copy link
Owner Author

fmeum commented Jul 24, 2024

@keith This PR should fix the problems with shared deps on Linux, but it runs into a surprising failure on macOS: Despite a matching LC_RPATH entry at https://github.com/fmeum/with_cfg.bzl/actions/runs/10075507430/job/27853944169?pr=104#step:5:356, the test fails and (seemingly) never searches in that location. Do you have an idea why?

@fmeum
Copy link
Owner Author

fmeum commented Jul 24, 2024

I can reproduce this failure on test_cc_binary_tool_with_dynamic_deps in Bazel (see bazelbuild/bazel#23085, in which I enable more tests on macOS). I'm confused by what I'm seeing: Why doesn't Bazel set @rpath/libname.dylib as LC_LOAD_DYLIB and then adds appropriate LC_RPATH entries (usually multiple) as on Linux? As it stands, it looks like the LC_RPATH entries are simply ignored.

@fmeum
Copy link
Owner Author

fmeum commented Jul 24, 2024

Can confirm that this patch to Bazel fixes the problem:

diff --git a/tools/cpp/osx_cc_wrapper.sh.tpl b/tools/cpp/osx_cc_wrapper.sh.tpl
index e40a98bb1a601d..3d5e3bdfb3beed 100644
--- a/tools/cpp/osx_cc_wrapper.sh.tpl
+++ b/tools/cpp/osx_cc_wrapper.sh.tpl
@@ -106,7 +106,7 @@ function get_otool_path() {
 
 function call_install_name() {
     /usr/bin/xcrun install_name_tool -change $(get_otool_path "$1") \
-        "@loader_path/$2/$3" "${OUTPUT}"
+        "@rpath/$3" "${OUTPUT}"
 }
 
 # Do replacements in the output

@fmeum
Copy link
Owner Author

fmeum commented Jul 24, 2024

Sorry for the stream of messages here, I just now learned about --incompatible_macos_set_install_name. Looks like flipping that is required for correct dylib discovery, but the issue about it has gone stale (bazelbuild/bazel#12370)? Do you see a reason not to flip this?

@fmeum
Copy link
Owner Author

fmeum commented Jul 24, 2024

TLDR is that this is broken in Bazel without bazelbuild/bazel#23089. I disabled the test on macOS for now.

@fmeum fmeum merged commit fdcaa33 into main Jul 24, 2024
11 checks passed
@fmeum fmeum deleted the tests-dynamic-deps branch July 24, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle shared library dependencies of cc_binary
1 participant