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

Make Homebrew work with Xcode 12 Beta 2 on Apple Silicon #7945

Merged
merged 1 commit into from Jul 8, 2020
Merged

Make Homebrew work with Xcode 12 Beta 2 on Apple Silicon #7945

merged 1 commit into from Jul 8, 2020

Conversation

claui
Copy link
Contributor

@claui claui commented Jul 8, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Starting with Xcode 12 Beta 2, builds that used to work on Apple Silicon now break due to Hardware#oldest_cpu returning :nehalem.

This PR is the first in a series of improvements to Hardware#oldest_cpu. It resolves the Xcode 12 Beta 2 issue for now.

Update: Confirmed to work on Xcode 12 Beta 1 (Apple Silicon), Xcode 12 Beta 2 (Apple Silicon) and Intel Macs.

Starting with Xcode 12 Beta 2, builds that used to work on Apple Silicon
now break due to `Hardware#oldest_cpu` returning `:nehalem` [1].

This commit is the first in a series of improvements to
`Hardware#oldest_cpu`. It resolves the Xcode 12 Beta 2 issue for now.

[1]: #7857 (comment)
@claui claui changed the title Make Hardware.oldest_cpu depend on architecture Make Homebrew work with Xcode 12 Beta 2 on Apple Silicon Jul 8, 2020
@claui claui requested review from Bo98 and sjackman July 8, 2020 20:03
nehalem: "-march=nehalem",
core2: "-march=core2",
core: "-march=prescott",
arm_vortex_tempest: "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine leaving this blank now. We can revisit it later if need be.

Comment on lines +46 to +48
unless Hardware::CPU.intel?
raise "Unexpected architecture: #{Hardware::CPU.arch}. This only works with Intel architecture."
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an interesting one this. I spent a few minutes debating with myself wheether this should instead return false.

In some cases raising makes sense as the formula might need some intention, in other cases returning false might actually make some formulae just work out-of-the-box.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. My rationale was that I’ve found a couple of formulae on homebrew-core doing stuff like: https://github.com/Homebrew/homebrew-core/blob/9e6f3fd50144b07f4bda3fdfed77f594c207a59e/Formula/john-jumbo.rb#L46

(requires_sse4? is an alias to requires_nehalem_cpu?.)

All similar usages I’ve found on homebrew-core do something unless requires_sse4?, just like in the example. So I thought we’d rather want those to fail safely.

@claui claui merged commit 0735915 into Homebrew:master Jul 8, 2020
@claui claui deleted the oldest_cpu branch July 8, 2020 20:29
@claui
Copy link
Contributor Author

claui commented Jul 8, 2020

Thanks @Bo98 for the feedback!

@MikeMcQuaid
Copy link
Member

Thanks for the fix @claui!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 25, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants