Fix versioned shared libraries for macOS toolchain #20847

oquenchil · 2024-01-10T21:26:48Z

The toolchain was passing -l:name. This mechanism doesn' t exist on macOS, instead the full path to the shared library should be passed.

Mainline commit: f0ade80

As it says on that mainline commit, it cannot be checked in with a test unlike this PR, because the macOS toolchain was moved outside of Bazel. Therefore, the change must go in, the macOS toolchain needs to be updated, then the test can be added. This change does go in with a test because the toolchain is still embedded in Bazel for 6.5.

Fixes #20487

Wyverald · 2024-01-10T21:40:02Z

Is this cherry-picking a mainline commit? If so, could you link to it in the PR description?

oquenchil · 2024-01-10T21:47:31Z

Not checked in yet in mainline. I will do that first and add it to the description.

tools/cpp/unix_cc_toolchain_config.bzl

The toolchain was passing -l:name. This mechanism doesn' t exist on macOS, instead the full path to the shared library should be passed. To add a proper test like in #20847, this change must first be checked in, the Apple toolchain needs to be updated and then the test can be added. RELNOTES:none PiperOrigin-RevId: 597366229 Change-Id: Ib7cf237cb6c58a69e781197408d3f703010909da

keith

it's probably not worth changing but this bug is also in the legacy toolchain features

oquenchil · 2024-01-11T11:21:16Z

I agree with your assessment about it not being worth it.

Thank you very much for the guidance and review Keith.

oquenchil · 2024-01-11T15:40:43Z

@keith I bumped into an issue with just listing the path that I don' t know how to work around.

If a library is linked by just listing the path, the loader_path doesn't seem to be added properly.

See here:

$ otool -L bazel-bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/libfoo_so.dylib
bazel-bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/libfoo_so.dylib:
	bazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/libfoo_so.dylib (compatibility version 0.0.0, current version 0.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1500.65.0)
	@loader_path/../../../../../../../../_solib_darwin_x86_64/_Usrc_Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary/libbar_so.dylib (compatibility version 0.0.0, current version 0.0.0)
	@loader_path/../../../../../../../../_solib_darwin_x86_64/_U_S_Ssrc_Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary_Cprebuilt___Usrc_Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary/libdirect_so_file.dylib (compatibility version 0.0.0, current version 0.0.0)
	@loader_path/../../../../../../../../_solib_darwin_x86_64/_Usrc_Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary3/libdiff_pkg_so.dylib (compatibility version 0.0.0, current version 0.0.0)
	@loader_path/../../../../../../../../_solib_darwin_x86_64/_Usrc_Smain_Sstarlark_Stests_Sbuiltins_Ubzl_Scc_Scc_Ushared_Ulibrary_Stest_Ucc_Ushared_Ulibrary/libprivate_lib_so.dylib (compatibility version 0.0.0, current version 0.0.0)
	bazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/renamed_so_file_2.so (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1971.0.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)

There is no entry for renamed_so_file_2.so with @loader_path in front, it's just the path that was used for linking, so when we run a binary that loads that dynamic lib it won't find it in runfiles.

dyld[25817]: Library not loaded: bazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/renamed_so_file_2.so
  Referenced from: <D3814B9B-BF69-34D8-9B3F-9E45B5D0D8F8> {mytmpdir}/fceaa1243897725196f169627f6cc0db/execroot/io_bazel/bazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/libfoo_so.dylib
  Reason: tried: 'bazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/renamed_so_file_2.so' (no such file), '/System/Volumes/Preboot/Cryptexes/OSbazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/renamed_so_file_2.so' (no such file), 'bazel-out/darwin-fastbuild/bin/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/renamed_so_file_2.so' (no such file), '/usr/local/lib/renamed_so_file_2.so' (no such file), '/usr/lib/renamed_so_file_2.so' (no such file, not in dyld cache)
Abort trap: 6

I tried a couple of things, like adding renamed_so_file_2.so behind every runtime library search directory, i.e:

                        flags = [
                            "-Xlinker",
                            "-rpath",
                            "-Xlinker",
                            "@loader_path/%{runtime_library_search_directories}/{libraries_to_link.name}",
                        ],

and adding -force_load too. Both strategies didn't help.

I'm out of ideas about how one is supposed to link a custom named dynamic library in macOS and have everything work. For such a basic thing, it feels like there must be a way.

If you want to try it out, you can clone this commit from my repository, then remove the suffix from every BUILD file in src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library{,2,3}/BUILD.builtin_test. Also the failing_targets directory.

/tmp/bazel_built_from_this_source run --experimental_cc_shared_library src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:binary

oquenchil · 2024-01-11T16:37:42Z

Someone pointed out to me that the problem is here:

bazel/tools/cpp/osx_cc_wrapper.sh.tpl

Line 39 in 8dd0998

elif [[ "$opt" =~ ^-l(.*)$ ]]; then

We won't have the full path library in LIBS anymore, therefore in the loop at the bottom where we call install_name_tool it won't be listed.

I will fix the script now to take into account this path as well.

keith · 2024-01-11T18:00:05Z

that script sounds right. alternatively we could try again to flip this flag https://github.com/bazelbuild/apple_support/blob/d87e8b07f3345e750834dbb6ce38c7c7d3b8b44b/crosstool/cc_toolchain_config.bzl#L2305-L2324 which I think would also solve it, but last I checked there was some sort of issue

keith · 2024-01-11T18:05:57Z

I tested with bazel run --experimental_cc_shared_library src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:binary --incompatible_macos_set_install_name and it doesn't immediately fix it, so that path would require some work too. theoretically to solve that script we could use -l: just as a sentinel value, although I don't love that

oquenchil · 2024-01-11T21:44:04Z

Running tests now but the failure is gone locally. I don't think we need the sentinel value. I added code to parse everything ending in .so or .dylib and then check for the file existence.

If everything passes, I think this is good enough. The fix to the bash script also needs to go in at head as far as I understand because apple_support is still using it, isn't it? But I wouldn't delay merging this PR for that.

keith · 2024-01-11T22:18:08Z

yes this script is also used by the default C++ toolchain on macOS even without the apple_support toolchain, so definitely needed at head as well. I can update the features in the apple_support toolchain assuming using that new variable is backwards compatible as long as it's unused (since no one must be using it today with this issue)

Mirrors bazelbuild/bazel#20847 This variable isn't supported until the linked change, but if anyone hit this codepath today it wouldn't work either so it's safe to use since it just changes the error message to something else.

keith · 2024-01-11T22:19:34Z

looks like it's fine to do now bazelbuild/apple_support#292

The commit f0ade80 was missing some required changes. The toolchain was passing -l:name. This mechanism doesn' t exist on macOS, instead the full path to the shared library should be passed. To add a proper test like in #20847, this change must first be checked in, the Apple toolchain needs to be updated and then the test can be added. RELNOTES:none PiperOrigin-RevId: 597810345 Change-Id: Ic88feaabdde05143ab145e997de1ef9487af83fd

Mirrors bazelbuild/bazel#20847 This variable isn't supported until the linked change, but if anyone hit this codepath today it wouldn't work either so it's safe to use since it just changes the error message to something else.

Baseline: 50b61e3 Release Notes: + Fix tree file materialized as symlink to another file when building without the bytes. (#20409) + Don't pass --add-opens= to javac (#20472) + Flip --incompatible_visibility_private_attributes_at_definition (#20520) + Fix extraction of tar archives containing sparse files. (#20531) + RemoteSpawnRunner: record inbetween phases in timing profile (#20550) + Add profiling to `remoteActionBuildingSemaphore.acquire()` (#20549) + The label API shakeup & docs cleanup (#20590) + Disable rewriter test (#20758) + Disable PyTest.testSmoke on macOS (#20729) + Upgrade abseil-cpp to fix build on macos_arm64 (#20785) + Ignore read-only errors when updating the `mtime` of the `install_base` (#20568) + Restart at most once when prepopulating repository rule environment (#20667) + Fix bootstrapped Bazel binary (#20804) + Add flag `experimental_throttle_remote_action_building` (#20861) + Fix versioned shared libraries for macOS toolchain (#20847) + Proto toolchainisation cherrypicks (#20925) Acknowledgements: This release contains contributions from many people at Google, as well as bazel.build machine account, Brentley Jones, Fabian Meumertzheim, Jordan Mele, Mai Hussien, oquenchil, Rahul Butani, Son Luong Ngoc, Xùdōng Yáng.

Fix versioned shared libraries for macOS toolchain

0514564

oquenchil requested review from keith and meteorcloudy January 10, 2024 21:26

oquenchil requested a review from a team as a code owner January 10, 2024 21:26

oquenchil removed request for a team, keith and meteorcloudy January 10, 2024 21:28

oquenchil self-assigned this Jan 10, 2024

oquenchil added the awaiting-user-response Awaiting a response from the author label Jan 10, 2024

Fix whitespaces

a962fed

oquenchil requested a review from keith January 10, 2024 21:35

Fix runfiles test

0e66103

keith reviewed Jan 10, 2024

View reviewed changes

tools/cpp/unix_cc_toolchain_config.bzl Show resolved Hide resolved

oquenchil added 2 commits January 10, 2024 23:26

Fix LibraryToLinkValueTest

c993b3f

Add flag change to osx_cc_toolchain_config.bzl

bc49b7c

oquenchil removed the awaiting-user-response Awaiting a response from the author label Jan 10, 2024

keith approved these changes Jan 10, 2024

View reviewed changes

sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Jan 11, 2024

oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 11, 2024

oquenchil added 2 commits January 11, 2024 14:02

Fix bug in default features in Java code

3395e13

Modify tools/osx

77246a0

keertk mentioned this pull request Jan 11, 2024

[6.5.0] Bazel 6.4.0 fails to build tensorflow-official on MacOS #20488

Closed

iancha1992 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 11, 2024

iancha1992 enabled auto-merge January 11, 2024 21:18

iancha1992 disabled auto-merge January 11, 2024 21:19

iancha1992 requested a review from Wyverald January 11, 2024 21:19

Modify tools/cpp/osx_cc_wrapper.sh.tpl

df4778f

keith approved these changes Jan 11, 2024

View reviewed changes

keith mentioned this pull request Jan 11, 2024

Replace -l: with path to library bazelbuild/apple_support#292

Merged

iancha1992 requested a review from meteorcloudy January 11, 2024 23:23

meteorcloudy approved these changes Jan 12, 2024

View reviewed changes

meteorcloudy added this pull request to the merge queue Jan 12, 2024

Merged via the queue into bazelbuild:release-6.5.0 with commit 29bbe31 Jan 12, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix versioned shared libraries for macOS toolchain #20847

Fix versioned shared libraries for macOS toolchain #20847

oquenchil commented Jan 10, 2024 •

edited by meteorcloudy

Wyverald commented Jan 10, 2024

oquenchil commented Jan 10, 2024

keith left a comment

oquenchil commented Jan 11, 2024

oquenchil commented Jan 11, 2024

oquenchil commented Jan 11, 2024

keith commented Jan 11, 2024

keith commented Jan 11, 2024

oquenchil commented Jan 11, 2024

keith commented Jan 11, 2024

keith commented Jan 11, 2024

Fix versioned shared libraries for macOS toolchain #20847

Fix versioned shared libraries for macOS toolchain #20847

Conversation

oquenchil commented Jan 10, 2024 • edited by meteorcloudy

Wyverald commented Jan 10, 2024

oquenchil commented Jan 10, 2024

keith left a comment

Choose a reason for hiding this comment

oquenchil commented Jan 11, 2024

oquenchil commented Jan 11, 2024

oquenchil commented Jan 11, 2024

keith commented Jan 11, 2024

keith commented Jan 11, 2024

oquenchil commented Jan 11, 2024

keith commented Jan 11, 2024

keith commented Jan 11, 2024

oquenchil commented Jan 10, 2024 •

edited by meteorcloudy