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

support Bazel 7 cc_shared_library #2232

Merged
merged 9 commits into from
Aug 30, 2024
Merged

Conversation

malt3
Copy link
Contributor

@malt3 malt3 commented Aug 28, 2024

This change is based on #2180 and adds support for cc_shared_library. This is needed for macOS and Bazel 7+, since cc_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 attribute dynamic_deps for CcSharedLibraryInfo, while haskell_library / haskell_binary use deps for both CcInfo and CcSharedLibraryInfo.
We also do not implement the same deduplication logic as cc_binary where we filter CcInfo deps which are already provided by a CcSharedLibraryInfo. 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:

  • Fix //tests/recompilation:recompilation_test_nixpkgs_bazel_7, which depends on the macOS Foundation framework. Probably broken by the removal of --undefined dynamic_lookup was broken by -fobjc-link-runtime but only worked accidentally (needed nixpkgs cc_toolchain).
  • Make tests work on Bazel 6 and 7 (use cc_shared_library only conditionally and make //tests/repl-targets:hs_lib_repl_test_nixpkgs_bazel_6 work again)

Supersedes #2107 and #2180

@malt3 malt3 mentioned this pull request Aug 28, 2024
@malt3 malt3 changed the title support bazel 7 cc_shared_library support Bazel 7 cc_shared_library Aug 28, 2024
avdv and others added 2 commits August 28, 2024 17:39
@malt3 malt3 force-pushed the mp/bazel-7-cc_shared_library branch 3 times, most recently from a0fa33f to f81a832 Compare August 29, 2024 13:27
Copy link
Member

@aherrmann aherrmann left a 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.

@malt3
Copy link
Contributor Author

malt3 commented Aug 29, 2024

Adding back the linker flags --undefined dynamic_lookup did not fix the missing Foundation framework at link time. I'll try to investigate why we depend on the framework in the first place and what other things might have changed.

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

@malt3 malt3 force-pushed the mp/bazel-7-cc_shared_library branch 3 times, most recently from 2df4be8 to 509bfbd Compare August 30, 2024 08:35
@avdv
Copy link
Member

avdv commented Aug 30, 2024

Adding back the linker flags --undefined dynamic_lookup did not fix the missing Foundation framework at link time.

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 nix-support directory for the cc toolchain from rules_nixpkgs: https://github.com/tweag/rules_nixpkgs/blob/a11818b2173100ab8122712ae3694b711f956cbd/toolchains/cc/cc.nix#L30C1-L50C7).

The failing test uses haskell_register_ghc_nixpkgs but does not configure a nixpkgs cc toolchain. That is probably not something that should be supported, but it worked with Bazel 6 / older rules_cc versions.

@malt3
Copy link
Contributor Author

malt3 commented Aug 30, 2024

The failing test uses haskell_register_ghc_nixpkgs but does not configure a nixpkgs cc toolchain. That is probably not something that should be supported, but it worked with Bazel 6 / older rules_cc versions.

So a better fix would be to add the nixpkgs cc toolchain to that WORKSPACE, correct?

@malt3 malt3 force-pushed the mp/bazel-7-cc_shared_library branch from 273627a to 8c02e49 Compare August 30, 2024 09:03
@avdv
Copy link
Member

avdv commented Aug 30, 2024

So a better fix would be to add the nixpkgs cc toolchain to that WORKSPACE, correct?

Yes. 🤞

@malt3 malt3 force-pushed the mp/bazel-7-cc_shared_library branch from 4b20e34 to fc6d7dc Compare August 30, 2024 11:00
@malt3 malt3 marked this pull request as ready for review August 30, 2024 11:01
@malt3 malt3 requested a review from avdv as a code owner August 30, 2024 11:01
Copy link
Member

@avdv avdv left a 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! ❤️

@malt3 malt3 added the merge-queue merge on green CI label Aug 30, 2024
@mergify mergify bot merged commit 15ad309 into master Aug 30, 2024
49 checks passed
@mergify mergify bot deleted the mp/bazel-7-cc_shared_library branch August 30, 2024 14:44
@mergify mergify bot removed the merge-queue merge on green CI label Aug 30, 2024
@malt3 malt3 mentioned this pull request Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants