[macOS][gn] Use arm64 clang in copy_info_plist.py on arm64 Macs#186661
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request updates copy_info_plist.py to dynamically determine the host architecture (arm64 or x64) for the clang++ path and ensures subprocess.check_output returns a string. Review feedback recommends removing a redundant str() wrapper on the file path and adding a check to prevent an IndexError if the clang version output is empty.
This eliminates instances of clang host toolchain path that assumes clang-x64 on macOS hosts and instead uses the toolchain for the host architecture. Note that if the build is intentionally invoked under Rosetta 2 -- for example by running `arch -x86_64 BUILD_COMMAND`, `host_cpu` will evaluate to `x86_64`. As such, it is still possible to run builds using the x64 toolchain on demand, but the default behaviour is now to build consistently with the appropriate clang toolchain for the host CPU architecture. Issue: flutter#103386
| _src_root_dir, 'flutter', 'buildtools', f'mac-{arch}', 'clang', 'bin', 'clang++' | ||
| ) | ||
| version = subprocess.check_output([clang_executable, '--version']) | ||
| version = subprocess.check_output([clang_executable, '--version'], text=True) |
There was a problem hiding this comment.
See the commit description; this ensures we return a string rather than bytes, which in this case is what we want.
| def get_clang_version(): | ||
| clang_executable = str( | ||
| os.path.join(_src_root_dir, 'flutter', 'buildtools', 'mac-x64', 'clang', 'bin', 'clang++') | ||
| arch = 'arm64' if platform.machine() in ('arm64', 'aarch64') else 'x64' |
There was a problem hiding this comment.
According to this comment, it could also be something like arm64_v8a. Perhaps compare against x86_64 and then default to arm64?
flutter/packages/flutter_tools/lib/src/base/os.dart
Lines 268 to 270 in 54e3e36
There was a problem hiding this comment.
On macOS and iOS this is always arm64 but the architecture's "correct" name is aarch64 (Linux/Android/WSL all return that), so added it out of paranoia in case they ever decide to start returning the same as others. Given how incredibly painful it would be for Apple to change this (would require new compiler triples to be registered etc.) it's probably safe for us to just hardcode arm64 here. arm64_v8a is specific to the Android NDK so unlikely to be an issue on macOS/iOS.
uname runs on every POSIX system and across OSes there's a lot of variance, so in os.dart, we need to handle things generically, but for a given OS, changing this is a pretty big breaking change, so seems very unlikely to break. (As is, I'm probably being over-paranoid by adding aarch64 at all)
I could invert the check if you prefer, but we have the exact same issue with x86_64 vs x64 for intel; it seems equally unlikely anyone will budge there either though.
Preference?
There was a problem hiding this comment.
Did a paranoid check for arm64e as well, since it's at one point Apple actually made that distinction on 32-bit devices. Looks like every chip since A12 supports those additional instructions but Apple kept arm64 for those too and provided a sysctl to check for arm64e.
There was a problem hiding this comment.
TL;DR this is gross whichever way we check this. For Intel, macOS and Linux use x86_64, the BSDs and Microsoft tend to use amd64, a bunch of Apple and MS tooling use x64. Platforms are awful. 🤷
There was a problem hiding this comment.
I'm going to land-as is since this works for all known macOS/iOS versions and unlikely to break anytime soon, but I'm going to file an issue to consolidate and centralise the toolchain path resolution.
We probably really shouldn't be scattering this logic all over the build/scripts, and instead just declare it once in a host_toolchain_path or similar variable in a low-level .gni file and re-use that everywhere. I feel like the version 13 -> 23, etc. is 100% going to keep biting us otherwise.
I had initially written the ASAN patch that way, but tempting as it was, the refactoring is kind of orthogonal to the de-rosettification.
There was a problem hiding this comment.
Err, nevermind, I filed that last night right as I was sharding these patches out of a larger patch, ran for dinner, and promptly forgot about it: #186668
flutter/flutter@3598686...259aeae 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 967ddb1aa561 to f1b406860c5e (2 revisions) (flutter/flutter#186731) 2026-05-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 5Ki-dBY4SpWdQMF_3... to -F9Ci3Opxt06MixDl... (flutter/flutter#186727) 2026-05-19 116356835+AbdeMohlbi@users.noreply.github.com Remove unused field in `ResourceExtractor` (flutter/flutter#186629) 2026-05-19 mbrase@google.com Update Fuchsia tests to use realm_builder_server as a subpackage (flutter/flutter#186409) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from cebf49d034b8 to 967ddb1aa561 (4 revisions) (flutter/flutter#186720) 2026-05-19 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186721) 2026-05-19 chris@bracken.jp [macOS][gn] support both x64/arm64 macOS host clang toolchains for ASAN (flutter/flutter#186669) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in generate_coverage.py on arm64 Macs (flutter/flutter#186662) 2026-05-19 awolff@google.com Fix broken link in impeller golden_tests readme (flutter/flutter#186470) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 27f7bba22600 to cebf49d034b8 (37 revisions) (flutter/flutter#186699) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in verify_exported.dart on arm64 Macs (flutter/flutter#186664) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in sanitizer_suppressions.sh on arm64 Macs (flutter/flutter#186663) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in copy_info_plist.py on arm64 Macs (flutter/flutter#186661) 2026-05-19 58529443+srujzs@users.noreply.github.com Complete completer only once in hot restart tests (flutter/flutter#186702) 2026-05-18 30870216+gaaclarke@users.noreply.github.com Testing autosubmit bot -- updating testowners (flutter/flutter#185226) 2026-05-18 chris@bracken.jp [ios] Correct handling for CADisplayLink paused-to-unpaused transitions (flutter/flutter#186457) 2026-05-18 chris@bracken.jp [Android][macOS][gn] support both x64/arm64 macOS host clang toolchains (flutter/flutter#186660) 2026-05-18 engine-flutter-autoroll@skia.org Roll Packages from 32c84d6 to b9bdd37 (2 revisions) (flutter/flutter#186683) 2026-05-18 jesswon@google.com [AGP 9] Upgrade Flutter Test Apps to AGP 9 (flutter/flutter#186200) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#11737) flutter/flutter@3598686...259aeae 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 967ddb1aa561 to f1b406860c5e (2 revisions) (flutter/flutter#186731) 2026-05-19 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 5Ki-dBY4SpWdQMF_3... to -F9Ci3Opxt06MixDl... (flutter/flutter#186727) 2026-05-19 116356835+AbdeMohlbi@users.noreply.github.com Remove unused field in `ResourceExtractor` (flutter/flutter#186629) 2026-05-19 mbrase@google.com Update Fuchsia tests to use realm_builder_server as a subpackage (flutter/flutter#186409) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from cebf49d034b8 to 967ddb1aa561 (4 revisions) (flutter/flutter#186720) 2026-05-19 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#186721) 2026-05-19 chris@bracken.jp [macOS][gn] support both x64/arm64 macOS host clang toolchains for ASAN (flutter/flutter#186669) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in generate_coverage.py on arm64 Macs (flutter/flutter#186662) 2026-05-19 awolff@google.com Fix broken link in impeller golden_tests readme (flutter/flutter#186470) 2026-05-19 engine-flutter-autoroll@skia.org Roll Skia from 27f7bba22600 to cebf49d034b8 (37 revisions) (flutter/flutter#186699) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in verify_exported.dart on arm64 Macs (flutter/flutter#186664) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in sanitizer_suppressions.sh on arm64 Macs (flutter/flutter#186663) 2026-05-19 chris@bracken.jp [macOS][gn] Use arm64 clang in copy_info_plist.py on arm64 Macs (flutter/flutter#186661) 2026-05-19 58529443+srujzs@users.noreply.github.com Complete completer only once in hot restart tests (flutter/flutter#186702) 2026-05-18 30870216+gaaclarke@users.noreply.github.com Testing autosubmit bot -- updating testowners (flutter/flutter#185226) 2026-05-18 chris@bracken.jp [ios] Correct handling for CADisplayLink paused-to-unpaused transitions (flutter/flutter#186457) 2026-05-18 chris@bracken.jp [Android][macOS][gn] support both x64/arm64 macOS host clang toolchains (flutter/flutter#186660) 2026-05-18 engine-flutter-autoroll@skia.org Roll Packages from 32c84d6 to b9bdd37 (2 revisions) (flutter/flutter#186683) 2026-05-18 jesswon@google.com [AGP 9] Upgrade Flutter Test Apps to AGP 9 (flutter/flutter#186200) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This eliminates instances of clang host toolchain path that assumes clang-x64 on macOS hosts and instead uses the toolchain for the host architecture.
Also updates the subprocess.check_output() call with
text=Trueto return a string back instead of raw bytes, which is what we want in this case.Note that if the build is intentionally invoked under Rosetta 2 -- for example by running
arch -x86_64 BUILD_COMMAND,host_cpuwill evaluate tox86_64. As such, it is still possible to run builds using the x64 toolchain on demand, but the default behaviour is now to build consistently with the appropriate clang toolchain for the host CPU architecture.No test changes because the build (and existing tests) are the test.
Issue: #103386
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.