Macos cc_wrapper: Use otool to read install name

[jlaxson]

When linking an executable on darwin, cc_wrapper.sh contains functionality to ensure rpath entries and shared library install names are relative and match the intended paths that will be used in downstream actions. This depends on being able to traverse symlinks from the _solibs path (like _solib_darwin_x86_64/_U_S_S_Cgenlib_Uimport___U/libgenlib.so) to the actual file (like bazel-out/darwin-opt-exec-31C2CD59/bin/libgenlib.so). This also assumes that the library has its internal install name equal to that latter path - this seems to be generally true right now, but is maybe a bit brittle.

This system breaks down when doing linking on a remote executor, because Bazel does not encode symlinks, and so the file shows up in _solib_darwin_x86_64 as a real file, and the wrapper picks the wrong install name. The resulting executable works when run directly from bazel-bin/bazel-out on a local machine, but will not work via bazel run, or when used in remote actions.

This change uses otool to read the install name of the target library, which is guaranteed to match what was embedded into the executable during link time. This result will be immune to the peculiarities of how symlinks are used and what install_name is encoded into the shared library during its creation.

[allevato]

I'm not terribly familiar with this script or its history; maybe @trybka has some insight.

[keith]

note this logic will have to be duplicated in https://github.com/bazelbuild/bazel/blob/master/tools/cpp/osx_cc_wrapper.sh

[trybka]

I'm not super familiar with this TBH, but the change looks reasonable to me.

[allevato]

I'm not super familiar with this TBH, but the change looks reasonable to me.

I'll take it!

[trybka]

@keith's point about duplicating the changes to the other script still stand though. :-)

[jlaxson]

Rebased and added the change to the non-tpl copy of the script

[keith]

works for our use case with swift-syntax, here's the diff of otool -L in that case:

 bazel-bin/tools/deadcode/deadcode:
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 905.6.0)
-       @rpath/lib_InternalSwiftSyntaxParser.dylib (compatibility version 1.0.0, current version 1300.0.31)
+       @loader_path/../../_solib_darwin_x86_64/_U@com_Ugithub_Ukeith_Uswift_Usyntax_S_S_Clibrary___U/lib_InternalSwiftSyntaxParser.dylib (compatibility version 1.0.0, current version 1300.0.31)

[keith]

@trybka can you take another look at this one?

[trybka]

Shouldn't the OTOOL var appear in the .sh as well as the .sh.tpl file?

[keith]

Suggested change

	${OTOOL} -D "$1" | awk 'NR==2{print $1}' | sed 's|^.*/bazel-out/|bazel-out/|'
	/usr/bin/otool -D "$1" | awk 'NR==2{print $1}' | sed 's|^.*/bazel-out/|bazel-out/|'

[sgowroji]

Hello @jlaxson, Could you please resolve the above branch conflicts and reply to the given review comments. Thanks!

[keith]

I submitted a rebased version here #16542

[keertk]

Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this since a new one has been submitted. Please feel free to reopen if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

[keith]

should be fine, #16542 should be landed instead