Made clang tidy use arm64 if on an arm64 mac. #47494

gaaclarke · 2023-10-30T23:04:03Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

matanlurey

Seems reasonable to me, though it's unfortunate this is yet another divergence between the tool and the CI script to run the tool (sure, you could pass the path manually, but exactly nobody will remember to do that).

If we could have the tool find the right binary, instead of relying on the CI script, I would prefer that, but this seems stricly better than the status quo.

gaaclarke · 2023-10-30T23:43:27Z

Yea, fair point. Plus the less code in bash the better. I think people just run the bash script for the most point locally. I don't think there is a reason to use the tool directly.

I'm interested to see what this does to our cpu usage numbers. Maybe we can bring everything back to presubmit.

auto-submit · 2023-10-30T23:47:19Z

auto label is removed for flutter/engine/47494, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

zanderso

I think an easier solution might be to put --force-mac-arm64 on the right gn commands in the build config .json files.

ETA: It's already there: https://github.com/flutter/engine/blob/main/ci/builders/mac_clang_tidy.json#L17, so at least on CI, this CL will be a no-op.

tools/clang_tidy/test/clang_tidy_test.dart

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

gaaclarke · 2023-10-31T16:51:30Z

I think an easier solution might be to put --force-mac-arm64 on the right gn commands in the build config .json files.

ETA: It's already there: https://github.com/flutter/engine/blob/main/ci/builders/mac_clang_tidy.json#L17, so at least on CI, this CL will be a no-op.

Ahh yea, a no-op on ci. I commented in that other issue, we should get rid of --force-mac-arm64. This PR will only make the execution faster locally. Being able to override the clang-tidy executable is a sound design outside of this problem since deriving the clang-tidy location from build_commands.json is built on assumptions of the build environment.

So, considering this speeds up local execution, won't break when the ill-conceived force-mac-arm64 is deleted and has value beyond the problem it addresses, I recommend still landing it.

are nonstandard, we should update the readme.

zanderso · 2023-10-31T17:29:25Z

ci/clang_tidy.sh

+  CLANG_TIDY_PATH="buildtools/mac-arm64/clang/bin/clang-tidy"
+fi
+
+COMPILE_COMMANDS="$SRC_DIR/out/$BUILD_DIR/compile_commands.json"


How is $BUILD_DIR defined?

It doesn't. This effectively makes gn get called every time which isn't an error. In fact it might be desired. It's quick to execute (1704ms) and means you'll get coverage from newly added files. I'm going to delete the check.

gn run time can vary a lot. Some runs on my M1 take >10s. Here's a run on CI taking >4s: https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/194351/overview.

Sorry to nitpick, but would it be okay to just fix the arm vs. intel issue in this PR?

Yea, no problem. I brought back the check. What is it doing in all that 10s? In CI we should have just run gn... Oh I guess it won't match though because of the --force-mac-arm64. How I loathe that flag.

I actually for a moment considered making that check smarter. It should be doing a timestamp check against the repository. That will take a fair bit of work though so I left it for a future exercise. The indiscriminate gn doesn't have that bug.

The clang_tidy script depends on engine_repo_tools, which already has a way of finding the most recently generated out/ directory: https://github.com/flutter/engine/blob/main/tools/pkg/engine_repo_tools/lib/engine_repo_tools.dart#L159, so that could be a good way to avoid adding similar logic in the shell script.

…ne#47494)

…137651) flutter/engine@406b7d7...db06c2e 2023-10-31 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from rg_Ici1tgAVF93cn9... to x2NzYMNhyodIwDl0I... (flutter/engine#47528) 2023-10-31 30870216+gaaclarke@users.noreply.github.com [Impeller] updated ios benchmark link (flutter/engine#47515) 2023-10-31 30870216+gaaclarke@users.noreply.github.com Made clang tidy use arm64 if on an arm64 mac. (flutter/engine#47494) 2023-10-31 bdero@google.com [Impeller] Allow 3D scenes to render when MSAA is not supported. (flutter/engine#47217) 2023-10-31 matanlurey@users.noreply.github.com Surgically remove `.*dither.*` from the Engine (flutter/engine#46750) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from rg_Ici1tgAVF to x2NzYMNhyodI If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

gaaclarke added 3 commits October 30, 2023 16:03

Made clang tidy use arm64 if on an arm64 mac.

d567ad2

added tests

b67f6c6

fixed linux

a356a25

gaaclarke marked this pull request as ready for review October 30, 2023 23:38

gaaclarke requested review from zanderso and matanlurey October 30, 2023 23:38

matanlurey approved these changes Oct 30, 2023

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2023

zanderso reviewed Oct 31, 2023

View reviewed changes

tools/clang_tidy/test/clang_tidy_test.dart Outdated Show resolved Hide resolved

Update tools/clang_tidy/test/clang_tidy_test.dart

e7d1d4b

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

gaaclarke added 2 commits October 31, 2023 09:57

null check

6e55ce1

sorry, wackamole to avoid relearning how to run these tests since they

2208d34

are nonstandard, we should update the readme.

zanderso reviewed Oct 31, 2023

View reviewed changes

gaaclarke added 2 commits October 31, 2023 10:40

started executing gn indiscriminately

abd5e29

brought back the if check

8e94cbd

zanderso approved these changes Oct 31, 2023

View reviewed changes

added doc

170d244

gaaclarke merged commit 34dfd26 into flutter:main Oct 31, 2023

engine-flutter-autoroll mentioned this pull request Nov 1, 2023

Roll Flutter Engine from 406b7d71ca37 to db06c2e10459 (5 revisions) flutter/flutter#137651

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 1, 2023

34dfd268a Made clang tidy use arm64 if on an arm64 mac. (flutter/engi…

f263517

…ne#47494)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Made clang tidy use arm64 if on an arm64 mac. #47494

Made clang tidy use arm64 if on an arm64 mac. #47494

Uh oh!

gaaclarke commented Oct 30, 2023 •

edited

Loading

Uh oh!

matanlurey left a comment

Uh oh!

gaaclarke commented Oct 30, 2023

Uh oh!

auto-submit bot commented Oct 30, 2023

Uh oh!

zanderso left a comment •

edited

Loading

Uh oh!

Uh oh!

gaaclarke commented Oct 31, 2023

Uh oh!

zanderso Oct 31, 2023

Uh oh!

gaaclarke Oct 31, 2023

Uh oh!

zanderso Oct 31, 2023

Uh oh!

gaaclarke Oct 31, 2023

Uh oh!

gaaclarke Oct 31, 2023

Uh oh!

zanderso Oct 31, 2023

Uh oh!

Uh oh!

Made clang tidy use arm64 if on an arm64 mac. #47494

Made clang tidy use arm64 if on an arm64 mac. #47494

Uh oh!

Conversation

gaaclarke commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Oct 30, 2023

Uh oh!

auto-submit bot commented Oct 30, 2023

Uh oh!

zanderso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke commented Oct 31, 2023

Uh oh!

zanderso Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

gaaclarke Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

zanderso Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

gaaclarke Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

gaaclarke Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

zanderso Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke commented Oct 30, 2023 •

edited

Loading

zanderso left a comment •

edited

Loading