Add -install_name when linking shared libraries on macOS by Yannic · Pull Request #12304 · bazelbuild/bazel

Yannic · 2020-10-19T19:01:42Z

This change exposes the name of the solib symlink available at runtime
to link actions and uses that name to set the -install_name of shared
libraries on macOS. Exposing the runtime solib name to link actions
allows us to remove the need for the cc_wrapper script that patched the
binary on macOS (which relied on the fact that the runtime solibs are
symlinks locally, which is not true in all cases).

Updates #7415

This change exposes the name of the solib symlink available at runtime to link actions and uses that name to set the `-install_name` of shared libraries on macOS. Exposing the runtime solib name to link actions allows us to remove the need for the cc_wrapper script that patched the binary on macOS (which relied on the fact that the runtime solibs are symlinks locally, which is not true in all cases).

keith

this is awesome, removing these wrappers is a huge win!

Yannic · 2020-10-20T09:08:08Z

Downstream: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1712

Most projects are passing, except for rules_go. It seems like cgo relies on the default value for -install_name (which seems to be that the value of -o is used), so we may have to roll this out as incompatible change.

Yannic · 2020-10-20T09:12:46Z

@oquenchil Could we please get an early (high-level) review on this?

oquenchil · 2020-10-21T15:54:52Z

This looks like a very good clean up. Thank you very much. If rules_go indicates that it can break people, then I would say put it behind a flag.

Apart from that the code looks good to me. I'm a bit hesitant about the name runtime_solib_name, does the way the build variable is used make sense only for macOS or you see this potentially being used for unix too? If not, perhaps the link build variable name should indicate that with an osx_ prefix. Up to you.

As a more general thought but nothing for you to take care of, I wish we could come up with a more flexible way of adding build variables that doesn't involve hardcoding them in the Java code.

Currently in the macOS toolchain C++ linking goes through `cc_wrapper.sh`, not `wrapped_clang`. I didn't add the handling to `cc_wrapper` for this argument. These wrappers will likely go away soon, when they do we should probably make them route to wrapped_clang instead, in which case we could revert this. bazelbuild#12304

Yannic · 2020-10-28T14:00:24Z

@oquenchil PTAL, thanks!

This looks like a very good clean up. Thank you very much. If rules_go indicates that it can break people, then I would say put it behind a flag.

Done

Apart from that the code looks good to me. I'm a bit hesitant about the name runtime_solib_name, does the way the build variable is used make sense only for macOS or you see this potentially being used for unix too? If not, perhaps the link build variable name should indicate that with an osx_ prefix. Up to you.

The runtime solibs are not specific to macOS, so I don't think the variable should have an osx_ prefix. I can imagine the value being used on Unix/Windows as well.

keith · 2020-10-28T16:59:18Z

+                with_features = [with_feature_set(features = ["dynamic_linking_mode"])],
+            ),
+        ]
+        if ctx.fragments.cpp.do_not_use_macos_set_install_name:


instead of these conditions could we only inject the runtime_solib_name variable with the feature is enabled?

Currently in the macOS toolchain C++ linking goes through `cc_wrapper.sh`, not `wrapped_clang`. I didn't add the handling to `cc_wrapper` for this argument. These wrappers will likely go away soon, when they do we should probably make them route to wrapped_clang instead, in which case we could revert this. #12304 Closes #12356. PiperOrigin-RevId: 340817292

Yannic · 2020-11-11T17:34:34Z

@oquenchil Any chance you can take a look and we get this into 4.0?

keith · 2020-11-11T18:46:07Z

            supports_dynamic_linker_feature,
            objcopy_embed_flags_feature,
            dynamic_linking_mode_feature,
+            set_install_name,


shouldn't this feature apply to dylibs built for iOS as well?

Probably we should, done. PTAL

oquenchil

LGTM

oquenchil · 2020-11-12T11:55:51Z

I'm seeing this test failure:
https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/1b58eec1-10d5-4843-8af4-324e41f79556/src/test/shell/bazel/cpp_darwin_integration_test/attempt_1.log

Yannic · 2020-11-12T13:46:52Z

Looks like this broke because of #12265. Fixed by using -Wl,-install_name,@rpath/...-

keith · 2020-11-12T16:09:04Z

Are we going to have to wait until 5.0 to flip this and cleanup those wrappers?

oquenchil · 2020-11-16T14:56:38Z

For flipping and cleaning up those wrappers you can do so right after 4.0 is cut (very soon, not sure if today or in 1 day). The next minor releases will have 4.0 as baseline so the behavior won't be there by default without using the flag.

I was told you can even flip before 4.0, but since this is an incompatible change, it might be too rushed, getting everything in downstream to pass etc,... If you are concerned about is not having to use the flag, then you can try that. If you are just concerned about cleaning up the code you will be able to do that soon.

#12304 added support to bazel for setting install names for dynamic libraries on darwin platforms. This would set LC_ID_DYLIB to @rpath/{library_name}, so that RPATH would be used to locate these libraries at runtime. However, the code was using a utility method that assumed the library name was mangled, which is often not the case. Given that the output path should already have been determined with the mangled or unmangled name, we should be able to just use the base name of the artifact. The test that was added in #12304 has been updated to actually use dynamic libaries, and passes with the changes made in this commit. Closes #13427. PiperOrigin-RevId: 377504015

Based on bazelbuild/bazel#10254 (comment) and bazelbuild/bazel#12304 being fixed, this special handling of rpaths on macOS appears to be unnecessary. This cleanup ensures that Bazel sets the correct metadata for the exec location of Go libraries linked in c-shared mode, which in turn allows to not include them in the runfiles of all dependents - cc_* targets depending on them will now generate the correct rpath entries to find the libraries at runtime at the usual position.

* Remove unnecessary rpath special handling on macOS Based on bazelbuild/bazel#10254 (comment) and bazelbuild/bazel#12304 being fixed, this special handling of rpaths on macOS appears to be unnecessary. This cleanup ensures that Bazel sets the correct metadata for the exec location of Go libraries linked in c-shared mode, which in turn allows to not include them in the runfiles of all dependents - cc_* targets depending on them will now generate the correct rpath entries to find the libraries at runtime at the usual position. * Don't include non-executable go_binary in dependent's runfiles If a go_binary is built with a non-executable link mode such as `c-archive`, its dependents currently pick up a runfile dependency on it since its DefaultInfo specifies the resulting archive as an executable. This is unnecessary as the dependent should be able to decide whether to include the file (e.g. dynamic linking) or not (e.g. static linking). With this commit, the executable field of the DefaultInfo is only populated if the go_binary is built with an executable link mode. Follow-up to #3143

google-cla Bot added the cla: yes label Oct 19, 2020

keith approved these changes Oct 19, 2020

View reviewed changes

Yannic mentioned this pull request Oct 20, 2020

go_binary: Emit CcInfo directly instead of using cc_{import,library} bazel-contrib/rules_go#2682

Closed

meisterT requested a review from oquenchil October 21, 2020 11:32

meisterT added the team-Rules-CPP Issues for C++ rules label Oct 21, 2020

keith mentioned this pull request Oct 27, 2020

Remove C++ linking from oso_prefix feature for now #12356

Closed

keith reviewed Oct 27, 2020

View reviewed changes

Comment thread tools/cpp/osx_cc_wrapper.sh.tpl Outdated

Yannic added 5 commits October 28, 2020 12:57

Guard behind incompatible_flag and add test

e0b5d80

Merge branch 'master' into runtime_solib_name

927d0be

Remove unused load

1798e7d

Default to false and set hostopt

a280944

Fix typo in test

d2bf956

Yannic marked this pull request as ready for review October 28, 2020 13:55

Yannic requested a review from lberki as a code owner October 28, 2020 13:55

keith mentioned this pull request Oct 28, 2020

incompatible_macos_set_install_name: Set -install_name when linking dynamic libraries on macOS #12370

Closed

keith reviewed Oct 28, 2020

View reviewed changes

Yannic added 2 commits November 11, 2020 18:01

Add set_install_name feature

0bf6883

Fix formatting

211eea5

keith reviewed Nov 11, 2020

View reviewed changes

Always add set_install_name

1e5d216

oquenchil approved these changes Nov 12, 2020

View reviewed changes

Switch to -Wl,-install_name,@rpath/...

7640de5

Yannic force-pushed the runtime_solib_name branch from 1947bf5 to 7640de5 Compare November 12, 2020 13:23

bazel-io closed this in cf47f90 Nov 12, 2020

steeve mentioned this pull request Feb 14, 2021

Make go_binary expose CcInfo bazel-contrib/rules_go#2445

Merged

csmulhern mentioned this pull request May 5, 2021

Fixes incorrect install names on darwin platforms #13427

Closed

csmulhern mentioned this pull request Jul 13, 2021

[4.2.0 Cherry-pick] Fixes incorrect install names on darwin platforms #13678

Closed

pcjanzen mentioned this pull request Apr 21, 2022

Revert "Fixes incorrect install names on darwin platforms" #15261

Closed

Conversation

Yannic commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keith left a comment

Choose a reason for hiding this comment

Uh oh!

Yannic commented Oct 20, 2020

Uh oh!

Yannic commented Oct 20, 2020

Uh oh!

oquenchil commented Oct 21, 2020

Uh oh!

Uh oh!

Yannic commented Oct 28, 2020

Uh oh!

keith Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

Yannic commented Nov 11, 2020

Uh oh!

keith Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

Yannic Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

oquenchil commented Nov 12, 2020

Uh oh!

Yannic commented Nov 12, 2020

Uh oh!

keith commented Nov 12, 2020

Uh oh!

oquenchil commented Nov 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Yannic commented Oct 19, 2020 •

edited

Loading