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.

Sign up for GitHub

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

Jump to bottom

Remove 'darwin' CPU value #17547

Closed
wants to merge 1 commit into from
Closed

Remove 'darwin' CPU value #17547

wants to merge 1 commit into from

Conversation

keith
Copy link
Member

@keith keith commented Feb 21, 2023 •
edited by meteorcloudy
Loading

This is another attempt to remove the legacy 'darwin' CPU string which actually means darwin_x86_64. The previous attempt was reverted here e96b8ca

RELNOTES[INC]: Remove 'darwin' as a CPU value, use 'darwin_x86_64' instead

@keith
Copy link
Member Author

keith commented Feb 21, 2023

@meteorcloudy can you run the downstream pipeline on this again? I want to see where this is at

@keith
Copy link
Member Author

keith commented Feb 21, 2023 •
edited
Loading

A few things have changed since we reverted this:

  • things have gotten better support for M1 machines, meaning they've likely messed with their select() statements that were relying on this
  • LTS releases have been around longer so a breaking change like this might be more acceptable now than it was then
  • we've gotten closer to caring about this CPU for the platforms transition, where supporting the old one likely doesn't make sense

so i'm interested to see if this looks more feasible now. I started looking at this again because with #16619 the unix toolchain would have to inherit the hacks aliasing these toolchains together

@meteorcloudy
Copy link
Member

Looks like there are already many tests failure in presubmit.

@meteorcloudy
Copy link
Member

Creating a downstream run here: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2877

@keith
Copy link
Member Author

keith commented Feb 22, 2023

ah yea sorry I should have checked back here before I asked you to do that, the change was just invalid at that point, so that downstream run is invalid

@meteorcloudy
Copy link
Member

No worries, feel free to ping me again when it's ready

@keith
Copy link
Member Author

keith commented Feb 22, 2023

@meteorcloudy ok ready for another attempt

@keith
Copy link
Member Author

keith commented Feb 22, 2023

oh turns out I can trigger these myself now nvm

@keith
Copy link
Member Author

keith commented Feb 22, 2023

https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2881#01867a63-f606-49d5-8007-5995508c159e

@keith
Copy link
Member Author

keith commented Feb 23, 2023

Ok so there are a few small issues with the downstream pipeline in the case people were selecting on only the old value, but PRs are out for everything there. I think given how simple the change is to fix this case (either replace darwin with darwin_x86_64, or add darwin_x86_64 if you want to support older bazels as well) it's probably reasonable to consider now, wdyt @meteorcloudy

@meteorcloudy
Copy link
Member

Sounds good!

but PRs are out for everything there.

Can we have the list of broken projects and probably the relevant PRs here, so we'll know how to track the progress of fixing those breakages.

/cc @oquenchil should also take a look.

@keith keith marked this pull request as ready for review February 23, 2023 16:30
@keith
Copy link
Member Author

keith commented Feb 23, 2023 •
edited
Loading

here are the remaining ones:

importantly the last 2 are dependencies of tensorflow, so once those merge they'll have to be bumped there, but that hasn't been a problem for me in the past

also worth noting that ubp and tensorflow are broken in the downstream job regardless of this change

@sgowroji sgowroji added team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website awaiting-review PR is awaiting review from an assigned reviewer labels Feb 23, 2023
@drraghavendra
Copy link

Macos Information needs to be included
":macos_x86_64_legacy": COMMON_SRCS + X86_SRCS + MACH_SRCS + MACH_X86_SRCS,

@oquenchil
Copy link
Contributor

Keith can this be merged?

@meteorcloudy meteorcloudy added team-Rules-CPP Issues for C++ rules and removed team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website labels Feb 27, 2023
@keith
Copy link
Member Author

keith commented Feb 27, 2023

Yes. But I would prefer to merge the toolchain deletion first and then rebase this one since they do potentially interact and this one is lower priority

@keith
Copy link
Member Author

keith commented Mar 2, 2023

@drraghavendra which PR do you mean? can you comment directly on that one?

@buildbreaker2021 buildbreaker2021 self-assigned this Mar 6, 2023
@buildbreaker2021 buildbreaker2021 added the awaiting-user-response Awaiting a response from the author label Mar 6, 2023
This was referenced Mar 14, 2023
Closed
Closed
@keith keith force-pushed the ks/remove-darwin-cpu-value branch from 6d3a631 to c1f55a4 Compare March 22, 2023 17:26
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Mar 22, 2023
@thomasvl thomasvl removed their request for review March 22, 2023 17:29
@allevato allevato removed their request for review March 22, 2023 18:02
@keith
Copy link
Member Author

keith commented Mar 22, 2023

@buildbreaker2021 I think we can land this now

@dmaclach dmaclach removed their request for review March 22, 2023 20:32
@keith
Copy link
Member Author

keith commented Mar 28, 2023

@buildbreaker2021 friendly bump 🙏🏻

@keith
Remove 'darwin' CPU value
e4efa6c 
This is another attempt to remove the legacy 'darwin' CPU string which
actually means darwin_x86_64. The previous attempt was reverted here
bazelbuild@e96b8ca

RELNOTES: Remove 'darwin' as a CPU value, use 'darwin_x86_64' instead
@keith keith force-pushed the ks/remove-darwin-cpu-value branch from c1f55a4 to e4efa6c Compare March 28, 2023 16:02
@buildbreaker2021 buildbreaker2021 added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Mar 29, 2023
@drraghavendra
Copy link

sir please merge the PR

@sgowroji
Copy link
Member

Hi @drraghavendra, Its internal in progress. Will update once imported!

copybara-service bot pushed a commit that referenced this pull request Mar 29, 2023
@keith @sgowroji
Remove 'darwin' CPU value
02846a8 
This is another attempt to remove the legacy 'darwin' CPU string which
actually means darwin_x86_64. The previous attempt was reverted here
e96b8ca

RELNOTES: Remove 'darwin' as a CPU value, use 'darwin_x86_64' instead

Partial commit for third_party/*, see #17547.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
@sgowroji
Copy link
Member

Thirdparty changes are merged at 02846a8

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 29, 2023
@keith keith mentioned this pull request May 3, 2023
Open
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@keith @fweikert
Remove 'darwin' CPU value
057e67f 
This is another attempt to remove the legacy 'darwin' CPU string which
actually means darwin_x86_64. The previous attempt was reverted here
bazelbuild@e96b8ca

RELNOTES: Remove 'darwin' as a CPU value, use 'darwin_x86_64' instead

Partial commit for third_party/*, see bazelbuild#17547.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@keith @fweikert
Remove 'darwin' CPU value
ad918ec 
This is another attempt to remove the legacy 'darwin' CPU string which actually means darwin_x86_64. The previous attempt was reverted here bazelbuild@e96b8ca

RELNOTES[INC]: Remove 'darwin' as a CPU value, use 'darwin_x86_64' instead

Closes bazelbuild#17547.

PiperOrigin-RevId: 520324475
Change-Id: I1de3dcfb4fc8d3920ef507ec12960058d24bcdcb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buildbreaker2021 buildbreaker2021 buildbreaker2021 approved these changes

@lberki lberki Awaiting requested review from lberki lberki is a code owner

@oquenchil oquenchil Awaiting requested review from oquenchil

@gregestren gregestren Awaiting requested review from gregestren gregestren is a code owner

@fweikert fweikert Awaiting requested review from fweikert fweikert is a code owner

@philomathing philomathing Awaiting requested review from philomathing

@DavidGoldman DavidGoldman Awaiting requested review from DavidGoldman DavidGoldman is a code owner

Assignees

@buildbreaker2021 buildbreaker2021

Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@keith @meteorcloudy @drraghavendra @oquenchil @buildbreaker2021 @sgowroji