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

Move Apple toolchain setup to apple_support #16619

Conversation

keith
Copy link
Member

@keith keith commented Nov 1, 2022

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 #15041

@keith
Copy link
Member Author

keith commented Nov 1, 2022

This is failing because rules_cc references the primary file we're deleting here

@katre
Copy link
Member

katre commented Nov 1, 2022

This looks reasonable. Can we migrate/update rules_cc to fix this? Ideally rules_cc wouldn't be mac-specific either.

@keith
Copy link
Member Author

keith commented Nov 1, 2022

Yea I definitely can, the problem will be doing that in lockstep, but I guess it's ok since it's breaking either way

@keith
Copy link
Member Author

keith commented Nov 1, 2022

bazelbuild/rules_cc#154

@keith
Copy link
Member Author

keith commented Nov 1, 2022

I think the order of operations has to be:

  1. remove from rules_cc
  2. create rules_cc release, update it in bazel
  3. remove here
  4. merge into apple_support

@keith
Copy link
Member Author

keith commented Nov 1, 2022

@katre ok I moved out all of the apple crosstool setup that I can, and moved it into bazelbuild/apple_support#113

one thing I'm unclear on currently is that I currently have the apple_support using local_config_cc, and falling back in the case the Apple toolchain doesn't apply. It seems ideally I would use local_config_apple_cc or something, but currently that would require we force a --crosstool_top argument that I don't think we want to. Based on https://github.com/grailbio/bazel-toolchain, this might change if we were using --incompatible_enable_cc_toolchain_resolution, but I believe that's still blocked on this detail #7260 (comment)

@katre
Copy link
Member

katre commented Nov 2, 2022

The right answer is for non-toolchains users to use --crosstool_top=@rules_apple//whatever, and for toolchains users to pick it up in the WORKSPACE.

That sequencing looks right but we should confirm with @comius.

@keith
Copy link
Member Author

keith commented Nov 2, 2022

I wonder if adding that in our transition would be enough instead of requiring that change on folks before we transition to the workspace way

@keith
Copy link
Member Author

keith commented Nov 2, 2022

it seems like if we could change the default of apple_crosstool_top to the new toolchain when it exists users wouldn't have to know about this. we could do that always in rules_apple, but that also is used in a transition in this repo, and defaults to the default CC toolchain

keith added a commit to keith/rules_cc that referenced this pull request Nov 3, 2022
@keith keith force-pushed the ks/move-apple-toolchain-setup-to-apple_support branch 2 times, most recently from 77b53f5 to 6d3b963 Compare January 6, 2023 20:43
keith added a commit to keith/rules_cc that referenced this pull request Jan 9, 2023
@keith keith force-pushed the ks/move-apple-toolchain-setup-to-apple_support branch 4 times, most recently from dfb18e0 to e41a264 Compare January 9, 2023 19:17
@keith
Copy link
Member Author

keith commented Jan 10, 2023

I think this is pretty close to ready, but I can't make CI green until we have a new release of rules_cc, it seems too difficult to correctly override all the tests cases for it at the moment.

@keith keith force-pushed the ks/move-apple-toolchain-setup-to-apple_support branch from 15ba30c to 6cbc6be Compare January 10, 2023 01:18
keith added a commit to keith/rules_cc that referenced this pull request Jan 10, 2023
@keith keith force-pushed the ks/move-apple-toolchain-setup-to-apple_support branch 3 times, most recently from 7b9b078 to 8a12270 Compare January 10, 2023 22:56
@googlewalt
Copy link
Contributor

We see a bunch of failures: https://buildkite.com/bazel/google-bazel-presubmit/builds/64142. We're fixing it in the internal import patch, but I want to check whether this should have been caught by bazel presubmits earlier?

@keith
Copy link
Member Author

keith commented Mar 6, 2023

i imagine something broke since i rebased last?

@keith
Copy link
Member Author

keith commented Mar 6, 2023

let me know if you want me to fix on this branch!

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 10, 2023
@keith keith deleted the ks/move-apple-toolchain-setup-to-apple_support branch March 10, 2023 17:23
@keith
Copy link
Member Author

keith commented Mar 10, 2023

Thanks all! @googlewalt just so I know for the future, are the files you ended up keeping libtool.sh / make_hashed_objlist.py still there because they're used in those paths internally? (since i don't see any external references)

@meteorcloudy
Copy link
Member

image
https://buildkite.com/bazel/bazel-bazel/builds?branch=master&page=3
https://buildkite.com/bazel/bazel-bazel/builds/22421#0186ca61-268c-4135-b44d-f1123f11d11c

It looks like this PR is causing a lot of tests to time out in Bazel's postsubmit. @keith Can you take a look?

@meteorcloudy
Copy link
Member

I guess we need to increase the test size of those timing out tests, e.g. https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/bazel/BUILD;l=937

But this does indicate that there is a potential performance regression?

@googlewalt
Copy link
Contributor

Thanks all! @googlewalt just so I know for the future, are the files you ended up keeping libtool.sh / make_hashed_objlist.py still there because they're used in those paths internally? (since i don't see any external references)

Yes. I suspect we should get rid of that dependency but left the cleanup for another day.

@keith
Copy link
Member Author

keith commented Mar 13, 2023

@meteorcloudy I'll look at this today

keith added a commit to keith/bazel that referenced this pull request Mar 13, 2023
Using this feature isn't supported by the Xcode based toolchain on
macOS, and generating a modulemap for the entire macOS SDK takes a huge
amount of time. This disables this feature on macOS until there's a
better solution to those performance issues.

Fixes bazelbuild#16619 (comment)
@keith
Copy link
Member Author

keith commented Mar 13, 2023

#17763

@meteorcloudy
Copy link
Member

This change was rollbacked at a50cca5, @zhengwei143 can you provide more details about what was broken?

@brentleyjones
Copy link
Contributor

I believe @googlewalt is working on a roll forward.

@keith
Copy link
Member Author

keith commented Mar 21, 2023

@googlewalt how's the forward fix looking?

@googlewalt
Copy link
Contributor

I just fixed one small remaining issue. Awaiting review @oquenchil.

@googlewalt
Copy link
Contributor

Submitted as 699e403.

@keith
Copy link
Member Author

keith commented Mar 22, 2023

Thanks!

keith added a commit to keith/bazel that referenced this pull request Mar 22, 2023
Using this feature isn't supported by the Xcode based toolchain on
macOS, and generating a modulemap for the entire macOS SDK takes a huge
amount of time. This disables this feature on macOS until there's a
better solution to those performance issues.

Fixes bazelbuild#16619 (comment)
copybara-service bot pushed a commit that referenced this pull request Mar 24, 2023
Followup to #16619.

PiperOrigin-RevId: 519183744
Change-Id: I13e4d00d679e1c1150c309264dc5fd3a980eb221
keith added a commit to keith/bazel that referenced this pull request May 4, 2023
Using this feature isn't supported by the Xcode based toolchain on
macOS, and generating a modulemap for the entire macOS SDK takes a huge
amount of time. This disables this feature on macOS until there's a
better solution to those performance issues.

Fixes bazelbuild#16619 (comment)
copybara-service bot pushed a commit that referenced this pull request May 15, 2023
Using this feature isn't supported by the Xcode based toolchain on macOS, and generating a modulemap for the entire macOS SDK takes a huge amount of time. This disables this feature on macOS until there's a better solution to those performance issues.

Fixes #16619 (comment)

Closes #17763.

PiperOrigin-RevId: 532080490
Change-Id: I96b77eff884fa965ed20084b90b1e0af5b80b082
fweikert pushed a commit to fweikert/bazel that referenced this pull request 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
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
Followup to bazelbuild#16619.

PiperOrigin-RevId: 519183744
Change-Id: I13e4d00d679e1c1150c309264dc5fd3a980eb221
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
Using this feature isn't supported by the Xcode based toolchain on macOS, and generating a modulemap for the entire macOS SDK takes a huge amount of time. This disables this feature on macOS until there's a better solution to those performance issues.

Fixes bazelbuild#16619 (comment)

Closes bazelbuild#17763.

PiperOrigin-RevId: 532080490
Change-Id: I96b77eff884fa965ed20084b90b1e0af5b80b082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS platform constraints locations
7 participants