Skip to content

Remove 'darwin' CPU value#17547

Closed
keith wants to merge 1 commit intobazelbuild:masterfrom
keith:ks/remove-darwin-cpu-value
Closed

Remove 'darwin' CPU value#17547
keith wants to merge 1 commit intobazelbuild:masterfrom
keith:ks/remove-darwin-cpu-value

Conversation

@keith
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

meteorcloudy commented Feb 22, 2023

Looks like there are already many tests failure in presubmit.

@meteorcloudy
Copy link
Copy Markdown
Member

meteorcloudy commented Feb 22, 2023

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

@keith
Copy link
Copy Markdown
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

@keith keith force-pushed the ks/remove-darwin-cpu-value branch from e0150aa to b43dab5 Compare February 22, 2023 16:25
@meteorcloudy
Copy link
Copy Markdown
Member

meteorcloudy commented Feb 22, 2023

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

@keith keith force-pushed the ks/remove-darwin-cpu-value branch from b43dab5 to e9d34b8 Compare February 22, 2023 17:16
@keith
Copy link
Copy Markdown
Member Author

keith commented Feb 22, 2023

@meteorcloudy ok ready for another attempt

@keith
Copy link
Copy Markdown
Member Author

keith commented Feb 22, 2023

oh turns out I can trigger these myself now nvm

@keith
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

meteorcloudy commented Feb 23, 2023

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
Copy Markdown
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

@keith keith force-pushed the ks/remove-darwin-cpu-value branch from b09e895 to 4b20fe3 Compare February 23, 2023 16:32
@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
Copy Markdown

drraghavendra commented Feb 26, 2023

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

@oquenchil
Copy link
Copy Markdown
Contributor

oquenchil commented Feb 27, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

drraghavendra commented Mar 29, 2023

sir please merge the PR

@sgowroji
Copy link
Copy Markdown
Member

sgowroji commented Mar 29, 2023

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
Copy Markdown
Member

sgowroji commented Mar 29, 2023

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

@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

1 more reviewer

@buildbreaker2021 buildbreaker2021 buildbreaker2021 approved these changes

Reviewers whose approvals may not affect merge requirements

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