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

iOS platform constraints locations #15041

Closed
keith opened this issue Mar 15, 2022 · 17 comments
Closed

iOS platform constraints locations #15041

keith opened this issue Mar 15, 2022 · 17 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: support / not a bug (process)

Comments

@keith
Copy link
Member

keith commented Mar 15, 2022

In order to use bazel toolchains with Apple platforms, we have to define the core set of platforms it supports. This is mostly done by https://github.com/bazelbuild/bazel/blob/96c8a9073807c9e97635ddafe2ed0365a9318d6f/tools/osx/crosstool/BUILD.toolchains

The issue is that since M1 macs were released there are now overlapping OS + CPU combos, like ios + arm64 that need more specific information for whether or not the build is for a physical device, or the simulator. To support this, these custom constraints exist: https://github.com/bazelbuild/apple_support/blob/2ec0ef3eb2954371596fbb1a225fb244fcde31a9/constraints/BUILD#L12-L26

Since they live in apple_support today, and bazel does not take them into account, as far as I can tell this setup isn't able to do what we want. It seems to me that we either need to move these constraints into bazel, and annotate the toolchains here, or move the toolchain definitions out of bazel and into apple_support (or another location?).

I'm hoping we can use this issue to decide on this, which will help make some more progress on toolchain support for Apple platforms.

@keith
Copy link
Member Author

keith commented Mar 15, 2022

In the short term if you want to try to use this, I believe you'd have to manually override local_config_cc_toolchains and define some valid toolchains yourself.

@keith
Copy link
Member Author

keith commented Mar 15, 2022

Related #14844

@gregestren gregestren added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged type: support / not a bug (process) labels Mar 15, 2022
@gregestren
Copy link
Contributor

As next step our team should discuss.

Since these definitions are "special" - owned by Apple but intermingled with C++, my intuition is the most practical way to continue supporting now is to move whatever extra constraints we need into Bazel, I suppose next to https://github.com/bazelbuild/bazel/blob/96c8a9073807c9e97635ddafe2ed0365a9318d6f/tools/osx/crosstool/BUILD.toolchains.

i.e. cleanly breaking Apple toolchain ownership from core Bazel is its own issue, with its own scope. For better or worse it's in Bazel now. If we need to support Apple toolchains, that's where we need to support them. Moving everything sounds like biting off a much larger commitment.

@gregestren
Copy link
Contributor

Update: I expect us to discuss this on Wednesday.

@gregestren
Copy link
Contributor

We chatted today and support the opinion at #15041 (comment).

I'd be happy to review a PR moving those constraints into tools/osx/crosstool. If anyone has the energy to practically migrate everything out of the Bazel repo, that's also welcome. But I'm guessing it's no one's top priority just yet. 😄

@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Mar 23, 2022
@keith
Copy link
Member Author

keith commented Mar 23, 2022

I'm curious about what our options are for moving things out. I think it makes sense in general for these apple platform specifics to be more closely tied to the apple related rules that you are likely (besides in the case of C++ binaries) already using if you're building for apple platforms. I think if we just wanted to move the toolchains and their setup out, we could do something like this bazelbuild/apple_support#113 and remove the related logic from bazel itself. The one thing I'm not sure of is if this will cause long term problems with tests in bazel itself around specific platforms, specifically for C++ and Objective-C logic

@gregestren
Copy link
Contributor

@trybka
Copy link
Contributor

trybka commented Mar 28, 2022

Concretely, I think it would mean that we couldn't build C++ on Apple platforms?

I think there is an argument for reducing the supported surface for Apple platforms without rules_apple, however I think we still want to maintain bare minimum C++ support for darwin_* which does not require (AFAIK) xcode, for instance.

I think it is more acceptable to say that anything requiring Xcode requires rules_apple, but I don't think it would be a great idea to remove this entirely.

I could be convinced otherwise, if, for example, Bazel on a Mac automatically fetched rules_apple for instance. But I think out of the box, I should be able to run a cc_test on a Mac.

@keith
Copy link
Member Author

keith commented Mar 29, 2022

The route I'm suggesting would still have a C++ toolchain setup for macOS through the same codepath as if you don't have Xcode installed today, specifically here:

_generate_cpp_only_build_file(repository_ctx, cpu_value, paths)

Using these toolchain configurations https://github.com/bazelbuild/bazel/blob/be21f194ae00d1a21e8e36a6fbb30be9e449086c/tools/cpp/BUILD.toolchains.tpl

I'm not sure what this would mean for the case you had both the Xcode toolchains, and this default setup, I'm not sure which toolchain configuration would win. But at least this wouldn't mean entirely removing the default macOS support.

@katre
Copy link
Member

katre commented Oct 28, 2022

We keep coming back to this, sorry.

After some discussion with the cc toolchain owners, I think we've come to a consensus on an appropriate solution, at least:

  1. Bazel's @bazel_tools repo needs some degree of minimal cc toolchain for macos users, so that host tools can be built as needed. However, Bazel doesn't (and shouldn't) define cc toolchains for building ios apps, and so we should rmeove these. This removes the need for the device and simulator constraints. We still need to support macos/intel and macos/m1, of course.
  2. Support for ios cc toolchains should be moved into the Apple rules repo. The device/simulator constraints should probably be in Apple support (is that where they are currently?). This means that anyone currently relying on these cc toolchains needs to import Apple rules, but I don't suspect there are a lot of users trying to build C++ code for ios without using Apple rules.

Does this make sense? @keith, is this something you can help work on? I'm happy to review PRs for changes to Bazel's version of cc toolchains for this.

@keith
Copy link
Member Author

keith commented Oct 28, 2022

Apple support (is that where they are currently?).

yes https://github.com/bazelbuild/apple_support/blob/master/constraints/BUILD

So if I understand what you're suggesting, I think this PR bazelbuild/apple_support#113 mostly shows an example of how this would work, with the slight difference that we might also want to move some other things that are in tools/ such as tools/cpp/osx_cc_configure.bzl and related files. At which point the standard unix toolchain would be used on macOS by default in the case that the user didn't pull in the apple toolchains explicitly. Does this sound right? Does moving as much of that setup as possible make sense?

@katre
Copy link
Member

katre commented Oct 28, 2022

I think so. I think the darwin_foo variants should still be in Bazel (so that host tools build), but aside from that it seems right.

@keith
Copy link
Member Author

keith commented Oct 28, 2022

Do we need explicit Darwin constraints vs the ones that are automatically picked up in the case you force the non-Xcode toolchain today?

@katre
Copy link
Member

katre commented Oct 28, 2022

That's a good question, I'm not sure offhand. We can experiment and see.

@keith
Copy link
Member Author

keith commented Nov 1, 2022

I think that is the case, at least if we don't care about the ability to cross compile even though it is supported by the command line tools only, to retain the cross compilation feature it's probably more complicated

In which case I think #16619 would be the minimum set of changes here. In my apple_support PR I currently use a different repository name for the toolchains (local_config_apple_cc_toolchains), but AFAIUI this is fine as long as long as everything is registered? It just means that both the C++ only, and the Apple toolchains would be registered in some cases

@katre
Copy link
Member

katre commented Nov 1, 2022

This looks good, commented on the PR.

keith added a commit to keith/bazel that referenced this issue Jan 6, 2023
keith added a commit to keith/bazel that referenced this issue Jan 9, 2023
keith added a commit to keith/bazel that referenced this issue Jan 10, 2023
keith added a commit to keith/bazel that referenced this issue Jan 11, 2023
keith added a commit to keith/bazel that referenced this issue Jan 12, 2023
@keith
Copy link
Member Author

keith commented Jan 12, 2023

The PR is now ready for review #16619, the only blocker is getting a rules_cc update into bazel

keith added a commit to keith/bazel that referenced this issue Jan 17, 2023
keith added a commit to keith/bazel that referenced this issue Jan 20, 2023
keith added a commit to keith/bazel that referenced this issue Jan 20, 2023
keith added a commit to keith/bazel that referenced this issue Feb 12, 2023
keith added a commit to keith/bazel that referenced this issue Feb 15, 2023
keith added a commit to keith/bazel that referenced this issue Feb 16, 2023
keith added a commit to keith/bazel that referenced this issue Feb 17, 2023
keith added a commit to keith/bazel that referenced this issue Feb 21, 2023
keith added a commit to keith/bazel that referenced this issue Feb 21, 2023
keith added a commit to keith/bazel that referenced this issue Feb 21, 2023
keith added a commit to keith/bazel that referenced this issue Feb 22, 2023
keith added a commit to keith/bazel that referenced this issue Feb 22, 2023
keith added a commit to keith/bazel that referenced this issue Feb 23, 2023
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
This moves the CC toolchain for building Apple platforms besides macOS to the apple_support repo bazelbuild/apple_support#113

The default unix toolchain is now used if someone wants to build for macOS without the apple_support toolchain, but it doesn't handle as many platform specific features as the previous toolchain.

Fixes bazelbuild#15041

Closes bazelbuild#16619.

PiperOrigin-RevId: 515546196
Change-Id: Ia54b53e7093c1edbfe8276730aaed5a11a94a027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: support / not a bug (process)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants