Skip to content

Conversation

@carlocab
Copy link
Member

@carlocab carlocab commented Mar 20, 2025

  • 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?

We need to allow -mcpu and -mtune flags to avoid breaking builds on
arm64 macOS. See, for example, Homebrew/homebrew-core#212051.

Also, let's reinstate adding our own optimisation flags if the compiler
was not invoked with a conflicting optimisation flag.

@carlocab carlocab requested review from Bo98, cho-m and Copilot March 20, 2025 06:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the handling of optimization flags to support runtime CPU detection builds on arm64 macOS by adjusting how flags such as -mcpu and -mtune are processed.

  • Removed -mtune and -mcpu flag matching from the first case and placed them in a separate condition for CPU detection.
  • Replaced a blanket concatenation of optflags with an iteration that checks for conflicts with existing flags.
Comments suppressed due to low confidence (1)

Library/Homebrew/shims/super/cc:204

  • Confirm that moving the -mtune and -mcpu flags here maintains the intended flag precedence and does not affect builds on platforms other than arm64.
when /^-march=.+/, /^-mtune=.+/, /^-mcpu=.+/

@Bo98
Copy link
Member

Bo98 commented Mar 20, 2025

We need to allow -mcpu and -mtune flags to avoid breaking builds on
arm64 macOS. See, for example, Homebrew/homebrew-core#212051.

Why? nss does not pass -mcpu or -mtune

@carlocab
Copy link
Member Author

Why? nss does not pass -mcpu or -mtune

Not really sure, but dropping the -mcpu/-mtune filtering fixes this failure: https://github.com/Homebrew/homebrew-core/actions/runs/13962241797/job/39085667674#step:3:1173

@Bo98
Copy link
Member

Bo98 commented Mar 20, 2025

Does it? Looks like it's actually our stripping of -march that actually made it work in the first place.

It's compiling as aarch32 because this is wrong: https://github.com/nss-dev/nss/blob/b6a1be96fba1c400d77698bde71ccaec6796e86d/coreconf/Darwin.mk#L18 + https://github.com/nss-dev/nss/blob/b6a1be96fba1c400d77698bde71ccaec6796e86d/coreconf/Darwin.mk#L34.

You can override CPU_ARCH in the formula fortunately to aarch64

@carlocab
Copy link
Member Author

You can override CPU_ARCH in the formula fortunately to aarch64

Won't that make the build assume a PowerPC build?

https://github.com/nss-dev/nss/blob/b6a1be96fba1c400d77698bde71ccaec6796e86d/coreconf/Darwin.mk#L35-L38

…ction builds

Let's reinstate adding our own optimisation flags if the compiler
was not invoked with a conflicting optimisation flag.
@carlocab carlocab force-pushed the runtime-cpu-detect-optflags branch from e285a2a to 351f7f8 Compare March 20, 2025 07:43
@carlocab
Copy link
Member Author

Dropped the -mtune and -mcpu part of the flags for now.

@Bo98
Copy link
Member

Bo98 commented Mar 20, 2025

Won't that make the build assume a PowerPC build?

Ah right yeah, probably needs something like:

diff --git a/coreconf/Darwin.mk b/coreconf/Darwin.mk
index 0569e1819..d1893adb4 100644
--- a/coreconf/Darwin.mk
+++ b/coreconf/Darwin.mk
@@ -31,13 +31,19 @@ override CPU_ARCH	= x86
 endif
 else
 ifeq (arm,$(CPU_ARCH))
-# Nothing set for arm currently.
+ifdef USE_64
+CC              += -arch arm64
+CCC             += -arch arm64
+override CPU_ARCH	= aarch64
+endif
 else
+ifeq (ppc,$(CPU_ARCH))
 OS_REL_CFLAGS	= -Dppc
 CC              += -arch ppc
 CCC             += -arch ppc
 endif
 endif
+endif
 
 ifneq (,$(MACOS_SDK_DIR))
     GCC_VERSION_FULL := $(shell $(CC) -dumpversion)

or I guess inreplaceing the arm away also works as a quick fix since the -arch flag really doesn't matter

@carlocab carlocab added this pull request to the merge queue Mar 20, 2025
Merged via the queue into master with commit f3bd91d Mar 20, 2025
36 checks passed
@carlocab carlocab deleted the runtime-cpu-detect-optflags branch March 20, 2025 08:16
@michaelsproul
Copy link

Hi @carlocab, apologies if this is the wrong place to post.

I'm debugging a formula build failure in this PR:

I think I've tracked the failure down to the cc shim's handling of the -mno-avx flag, and noticed that you recently made changes to the shim, and therefore must understand how it works 😁

Is it possible the shim would filter out the -mno-avx flag during the "flag detection" compile that the Rust build system runs – leading Rust's cc crate to think that the -mno-avx flag is supported when it actually is not?

Again, sorry if I'm barking up the wrong tree. I'm not very experienced in Homebrew's workings, nor Ruby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants