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

Add -install_name when linking shared libraries on macOS #12304

Closed
wants to merge 10 commits into from

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Oct 19, 2020

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).
@google-cla google-cla bot added the cla: yes label Oct 19, 2020
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Yannic
Copy link
Contributor Author

Yannic commented Oct 20, 2020

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
Copy link
Contributor Author

Yannic commented Oct 20, 2020

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

@oquenchil
Copy link
Contributor

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.

keith added a commit to keith/bazel that referenced this pull request Oct 27, 2020
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 Yannic marked this pull request as ready for review October 28, 2020 13:55
@Yannic Yannic requested a review from lberki as a code owner October 28, 2020 13:55
@Yannic
Copy link
Contributor Author

Yannic commented Oct 28, 2020

@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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bazel-io pushed a commit that referenced this pull request Nov 5, 2020
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
Copy link
Contributor Author

Yannic commented Nov 11, 2020

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

@@ -6228,6 +6250,7 @@ def _impl(ctx):
supports_dynamic_linker_feature,
objcopy_embed_flags_feature,
dynamic_linking_mode_feature,
set_install_name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should, done. PTAL

Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Yannic commented Nov 12, 2020

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

@bazel-io bazel-io closed this in cf47f90 Nov 12, 2020
@keith
Copy link
Member

keith commented Nov 12, 2020

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

@oquenchil
Copy link
Contributor

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.

bazel-io pushed a commit that referenced this pull request Jun 4, 2021
#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
katre pushed a commit that referenced this pull request Jul 12, 2021
#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
fmeum added a commit to CodeIntelligenceTesting/rules_go that referenced this pull request May 12, 2022
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.
linzhp pushed a commit to bazel-contrib/rules_go that referenced this pull request Jun 10, 2022
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants