[macOS][gn] Use arm64 clang in generate_coverage.py on arm64 Macs#186662
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 the generate_coverage.py script to dynamically detect the system architecture on macOS, ensuring the correct LLVM binary directory is used for both arm64 and x64. Review feedback suggests improving code locality by moving the architecture detection logic inside the macOS-specific conditional block and removing a redundant variable initialization.
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
d994b9d to
39dc77c
Compare
| platform_dir = 'linux-x64' | ||
| elif sys.platform == 'darwin': | ||
| platform_dir = 'mac-x64' | ||
| 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.
See comments here: #186661 (comment)
It won't be arm64_v8a since that's an Android-specific architecture, but the general point is taken. Intel has just as many though -- e.g. x86_64, x64, amd64. For Apple Silicon devices this is always arm64 but figured I'd be extra paranoid -- I'd actually originally written this outside the if/else chain and figured someone else might want to re-use it one day (ARM Linux in particular).
We could simplify down to arm64 and x86_64 in this case since those are the only names Apple uses for apple silicon devices.
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.
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.