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

extend/ENV/shared: effective_arch as public API #16477

Merged
merged 1 commit into from Jan 15, 2024

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Jan 13, 2024

  • 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 typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Opening this to discuss possibility of exposing effective_arch information.

Main reason is that we currently use Hardware.oldest_cpu in formulae but this may not be accurate if user wants to target different arch.

Would need some input if anything else needs to change to get this information available to be used by formulae that are not capable of relying on existing shim arch selection.

@cho-m cho-m added the discussion Input solicited from others label Jan 13, 2024
@MikeMcQuaid
Copy link
Member

Thanks for the PR @cho-m!

Main reason is that we currently use Hardware.oldest_cpu in formulae but this may not be accurate if user wants to target different arch.

Can you elaborate on some formulae that might use that, preferably in homebrew-core?

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Seems like a good idea to me.

@Bo98
Copy link
Member

Bo98 commented Jan 15, 2024

Can you elaborate on some formulae that might use that, preferably in homebrew-core?

Probably every existing call to Hardware.oldest_cpu would be replaced with ENV.effective_arch, which will work better for from-source builds that typically use native CPU rather than oldest CPU and aligns these references with what the shim does under the hood.

@MikeMcQuaid
Copy link
Member

Probably every existing call to Hardware.oldest_cpu would be replaced with ENV.effective_arch, which will work better for from-source builds that typically use native CPU rather than oldest CPU and aligns these references with what the shim does under the hood.

Ah, I thought it might be this. Note that we intentionally removed/changed this in the past so that from-source builds would more closely align with bottles because:

  1. we don't support from-source builds so don't really want to provide incentives for people to use them e.g. better performance
  2. we don't want to introduce potential e.g. compilation failures that only apply when building from source (an unsupported configuration) so better to do the same thing when bottling/not

It would probably be good to write this policy down somewhere, assuming we're agreed on sticking with it.

@cho-m
Copy link
Member Author

cho-m commented Jan 15, 2024

Can you elaborate on some formulae that might use that, preferably in homebrew-core?

I think zig-based formulae are the most common right now, e.g. ncdu https://github.com/Homebrew/homebrew-core/blob/master/Formula/n/ncdu.rb#L32-L37

    # Fix illegal instruction errors when using bottles on older CPUs.
    # https://github.com/Homebrew/homebrew-core/issues/92282
    cpu = case Hardware.oldest_cpu
    when :arm_vortex_tempest then "apple_m1" # See `zig targets`.
    else Hardware.oldest_cpu
    end

julia is another example - https://github.com/Homebrew/homebrew-core/blob/master/Formula/j/julia.rb#L93-L95

    # Set MARCH and JULIA_CPU_TARGET to ensure Julia works on machines we distribute to.
    # Values adapted from https://github.com/JuliaCI/julia-buildbot/blob/master/master/inventory.py
    args << "MARCH=#{Hardware.oldest_cpu}" if Hardware::CPU.intel?

Not sure on gmp usage. May need to check on what happens when native on Linux https://github.com/Homebrew/homebrew-core/blob/master/Formula/g/gmp.rb#L45-L49

    cpu = Hardware::CPU.arm? ? "aarch64" : Hardware.oldest_cpu
    if OS.mac?
      args << "--build=#{cpu}-apple-darwin#{OS.kernel_version.major}"
    else
      args << "--build=#{cpu}-linux-gnu"

openblas does target a CPU, but it also has ENV.runtime_cpu_detection so needs further details - https://github.com/Homebrew/homebrew-core/blob/master/Formula/o/openblas.rb#L39-L44

    ENV["TARGET"] = case Hardware.oldest_cpu
    when :arm_vortex_tempest
      "VORTEX"
    else
      Hardware.oldest_cpu.upcase.to_s
    end

As note, changes shouldn't impact Homebrew's bottling, but is mainly for users who want to prepare their own bottles perhaps optimized for different CPU. I think best for Linux users (given we only optimize for old Core2) or macOS Intel users (given we do not provide AVX due to Rosetta limitation).

@MikeMcQuaid
Copy link
Member

I think zig-based formulae are the most common right now, e.g. ncdu https://github.com/Homebrew/homebrew-core/blob/master/Formula/n/ncdu.rb#L32-L37

Yeh, this seems like a good idea.

but is mainly for users who want to prepare their own bottles perhaps optimized for different CPU

I don't think this is a path we should be working at supporting better as we don't support it and bottle usage outside the Homebrew org is fairly minimal.

@Bo98
Copy link
Member

Bo98 commented Jan 15, 2024

Note that we intentionally removed/changed this in the past so that from-source builds would more closely align with bottles

Sorry, I should have also clarified the native CPU logic is Linux-specific:

def effective_arch
if @build_bottle && @bottle_arch
@bottle_arch.to_sym
elsif @build_bottle
Hardware.oldest_cpu
else
:native
end
end

This behaviour already applies to the shims today even without this pull request.

On macOS there's basically no difference between ENV.effective_arch and Hardware.oldest_cpu except for --bottle-arch which is only a concern for third-party taps.

ENV.effective_arch is basically the indicator of what architecture we're compiling for. Any non-alignment with bottles wouldn't be because of this being public API as this already applies to almost all formulae except the very few listed above.

@MikeMcQuaid
Copy link
Member

Sorry, I should have also clarified the native CPU logic is Linux-specific:

Hah, yes, I forgot about this compromise 😅

On macOS there's basically no difference between ENV.effective_arch and Hardware.oldest_cpu except for --bottle-arch which is only a concern for third-party taps.

Does this API handle --bottle-arch as expected? I guess given that's an officially supported option: I'm not inclined to ✅ this assuming it does.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks @cho-m!

@Bo98
Copy link
Member

Bo98 commented Jan 15, 2024

Does this API handle --bottle-arch as expected?

Yes. ENV.effective_arch would return what you set --bottle-arch to.

@MikeMcQuaid MikeMcQuaid merged commit cea60ca into Homebrew:master Jan 15, 2024
25 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Feb 15, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2024
@cho-m cho-m deleted the effective-arch-non-private branch March 5, 2024 02:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion Input solicited from others outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants