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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

csmulhern
Copy link
Contributor

JUSTIFICATION:
--incompatible_macos_set_install_name is broken without this change. This cherry-pick applied cleanly without modifications.

ORIGINAL COMMIT:
b06f495

RELEASE NOTES:

  • Fixes --incompatible_macos_set_install_name not correctly setting install names.

OLD COMMIT MESSAGE:

#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

#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
@csmulhern csmulhern requested a review from lberki as a code owner July 13, 2021 21:29
@google-cla google-cla bot added the cla: yes label Jul 13, 2021
@csmulhern csmulhern changed the title Fixes incorrect install names on darwin platforms [4.2.0 Cherry-pick] Fixes incorrect install names on darwin platforms Jul 13, 2021
@csmulhern
Copy link
Contributor Author

Linking to the 4.2.0 Release Issue (#13558) for tracking purposes.

CC: @katre.

@katre katre requested review from katre and removed request for lberki July 13, 2021 21:37
@katre
Copy link
Member

katre commented Jul 13, 2021

Looks good, thanks. I'll merge this when the tests pass.

@katre
Copy link
Member

katre commented Jul 13, 2021

I spoke too soon: looks like the tests are failing. Please investigate and let me know when this is ready.

@csmulhern
Copy link
Contributor Author

The issue with the failing test is that a cc_library without linkshared=1 (libfoo in the test) is being dynamically linked, and then the dylib is not found at runtime due to an incorrect install name. It seems that:

  1. There was a change made after the cut that changed the linking behavior of libfoo in the test case.
  2. Install names are still not being set correctly in some cases.

Given the above, let's skip the cherry-pick.

@csmulhern csmulhern closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants