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

Configure LLVM cc_toolchain included with Swift releases #1113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gferon
Copy link
Contributor

@gferon gferon commented Sep 18, 2023

On Linux, and to some extent on macOS, Swift ships with its own version of clang.

Our problem: we have a tree with a lot of targets for different platforms, and we need to make sure that Swift built binaries are using the Apple-shipped clang if we decide to configure another default C++ toolchain for the rest of the build.

This is currently not possible, because rules_swift assumes the use of @bazel_tools//tools/cpp:toolchain_type for its cc_toolchain.

  • On macOS, this PR is essentially doing nothing, as we can alias @local_config_apple_cc//:toolchain
  • On Linux, it's a little bit more involved, as you need to run CC toolchain detection on the clang provided by Apple from https://www.swift.org/download/. Fortunately, the relevant functions are part of the public API of @bazel_tools.
  • On Windows, we do the same as on Linux, except we use the Windows functions from @bazel_tools.

This means that we can remove the check for CC=clang entirely and leave this choice to the user (which they might need if they have native code that only compiles with clang for example). It is also a prerequisite for downloading Swift toolchains (except on macOS) directly in Bazel, which should now be possible.

@gferon gferon force-pushed the use-cc-toolchain-config-for-swift branch from e1402a4 to 36ad5aa Compare September 18, 2023 15:29
@keith
Copy link
Member

keith commented Sep 18, 2023

I think the general assumption today to solve this is by doing CC=clang in the env when you exec bazel that the clang in that statement is the one from Swift, so the standard configuration picks up the tools that were installed with Swift instead of the system tools (that might not apply for llvm-ar etc unless those are symlinked to just ar)

@gferon
Copy link
Contributor Author

gferon commented Sep 19, 2023

I think the general assumption today to solve this is by doing CC=clang

Yes, and this is the problem I'm trying to solve. You want to make sure rules_swift picks up those tools, but not necessarily the rest of the build. With this patch, you can stop doing this.

There's even a TODO here (that I forgot to remove in this PR)

# TODO(allevato): This is expedient and fragile. Use the
explaining what I'm trying to achieve.

@gferon gferon force-pushed the use-cc-toolchain-config-for-swift branch 2 times, most recently from 737b285 to abde801 Compare September 21, 2023 08:29
@gferon gferon marked this pull request as ready for review September 21, 2023 08:54
@gferon gferon changed the title Use cc_toolchain_config for LLVM (clang) included with Swift releases Use cc_toolchain_config with LLVM included with Swift releases Sep 21, 2023
@gferon gferon changed the title Use cc_toolchain_config with LLVM included with Swift releases Configure LLVM cc_toolchain included with Swift releases Sep 21, 2023
@gferon gferon force-pushed the use-cc-toolchain-config-for-swift branch 2 times, most recently from 3570275 to f0afe6e Compare September 21, 2023 09:13
@luispadron
Copy link
Contributor

@gferon I fixed the merge conflict. Did we want to merge this @keith @thii?

@keith
Copy link
Member

keith commented Jan 18, 2024

I took this for a spin and I noticed that right now this works via using the old toolchain setup, which is a side effect of us not migrating to the new way sooner. Theoretically we should replace the _cc_toolchain attribute with https://github.com/bazelbuild/rules_apple/blob/c74704ac9a41c6998191d26bf418152bda9d2f26/apple/cc_toolchain_forwarder.bzl#L134, which results in this no longer working correctly. Similar on the consuming side instead of doing ctx.attr._cc_toolchain we should be using https://github.com/bazelbuild/rules_apple/blob/c74704ac9a41c6998191d26bf418152bda9d2f26/apple/internal/watchos_rules.bzl#L158

If we make these changes this no longer works because the newly created toolchains here aren't registered. I think the ideal solution would be that we would create these toolchains as you are now, register them with the normal infra, and then bazel would discover them correctly. The potential problem with that is I think we need another constraint of some sort on the toolchain to make sure bazel picks the right one in that case, and I'm not sure if there's a way to do that while still calling configure_unix_toolchain.

I noticed this because on macOS this change is actually a no-op for the wrong reasons, since we're using the new API already

cc_toolchain = find_cpp_toolchain(ctx)
and the result is the default CC toolchain is chosen instead of the one being aliased and specified here

@gferon gferon force-pushed the use-cc-toolchain-config-for-swift branch 2 times, most recently from bf4fc61 to 0618c50 Compare February 13, 2024 11:09
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.

None yet

4 participants