-
Notifications
You must be signed in to change notification settings - Fork 81
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
support Bazel 7 cc_shared_library #2232
Conversation
Adds support for CcSharedLibraryInfo provider
a0fa33f
to
f81a832
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably refactor the testing code so that cc_shared_library is used iff we are on Bazel 7 or greater.
Perhaps bazel_features can help. It doesn't seem to list a feature for cc_shared_library, yet. But, we could add one.
A more correct implementation would need to use aspects and might require the use of a separate attribute (dynamic_deps).
I think that's true. Nonetheless, I think this is a good step forward.
Adding back the linker flags Here is the remaining diff between the two Bazel versions for the failing target: --- /tmp/linker_flags_bazel_6 2024-08-29 19:06:26.401508228 +0200
+++ /tmp/linker_flags_bazel_7 2024-08-29 19:06:34.941537070 +0200
@@ -1,5 +1,6 @@
--optl-undefined
--optldynamic_lookup
+-optl-mmacosx-version-min=10.11
+-optl-no-canonical-prefixes
+-optl-fobjc-link-runtime
-optl-headerpad_max_install_names
-optl-lc++
-optl-lm Those might be responsible.. The latter two come from here, as far as I can tell: https://cs.opensource.google/bazel/bazel/+/refs/tags/7.3.1:tools/cpp/unix_cc_toolchain_config.bzl;l=1412-1427 |
2df4be8
to
509bfbd
Compare
I think that issue occurs when using an automatically configured local cc toolchain which is picking up a C compiler from inside a nix shell. The nix C compiler requires some environment variables to be set in order to locate the frameworks and these are not available inside of the Bazel actions (that is why we write these flags to files in the The failing test uses |
So a better fix would be to add the nixpkgs cc toolchain to that WORKSPACE, correct? |
273627a
to
8c02e49
Compare
Yes. 🤞 |
Only use on Bazel >= 7
4b20e34
to
fc6d7dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thank you! ❤️
This change is based on #2180 and adds support for
cc_shared_library
. This is needed for macOS and Bazel 7+, sincecc_library
no longer emits dynamic libraries on macOS (feature removal, linker flag removal).This implementation differs from the one in
cc_binary
in some ways:cc_binary
uses a separate attributedynamic_deps
forCcSharedLibraryInfo
, whilehaskell_library
/haskell_binary
usedeps
for bothCcInfo
andCcSharedLibraryInfo
.We also do not implement the same deduplication logic as
cc_binary
where we filterCcInfo
deps which are already provided by aCcSharedLibraryInfo
. This may lead to duplicate symbols. A more correct implementation would need to use aspects and might require the use of a separate attribute (dynamic_deps
).Open issues:
//tests/recompilation:recompilation_test_nixpkgs_bazel_7
, which depends on the macOSFoundation
framework.Probably broken by the removal ofwas broken by--undefined dynamic_lookup
-fobjc-link-runtime
but only worked accidentally (needed nixpkgs cc_toolchain).//tests/repl-targets:hs_lib_repl_test_nixpkgs_bazel_6
work again)Supersedes #2107 and #2180