clang-tidy: Added an environment variable that allows fix to print the generated diff.

[gaaclarke]

This helps with migrating new linter checks that have fixes implemented. Turning this on allows the bots to print out the diff that can be patched locally across all platform CI checks.

issue: flutter/flutter#113848

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 Hixie said 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.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

[zanderso]

Instead of using an environment variable, I think it would be simpler to just test for the presence of --fix in $@.

[gaaclarke]

I did that at first, but that makes it hard to use with the bots since $@ is changed in the bot configuration. This way you can just edit lint.sh file temporarily with FLUTTER_LINT_PRINT_FIX=1

[zanderso]

Oh, okay, I get it (I think). So the workflow is to modify the script on a PR branch to get the presubmit checks to generate the diff?

[gaaclarke]

Yep, that's the best I could come up with for now. Alternatively we could always execute with --fix but I worry about execution time.

[zanderso]

Ideally, we'd have something like flutter/flutter#114085, to pass the --fix flag only in presubmit. In the meantime, I think this change is okay. Would you mind adding a TODO in the code for me that links to that issue?

[zanderso]

We definitely care about postsubmit cycle time.

[gaaclarke]

Sure, I wouldn't say they have equal weight though. Given that if there is a failure, turning this on and running again takes ~1hr to get results, it might be worth running it always. So the calculus as-to wether this should be turned on always or not should be: (likelihood of violation * 1hr) - ((1 - likelihood of violation) * 5min).

Let me run some tests locally since that 5 minute I just pulled out of my butt. Maybe it isn't so bad to always have it on. Also, maybe the real solution to performance isn't turning off something that is helpful and saves time but to do better sharding. I discuss this in the post-mortem I just sent. Now that I talk about this I think it's better to having this turn on always, assuming it doesn't add a crazy amount of time. I'll measure it.

[gaaclarke]

I just ran it locally with a diff that was checking 47 files, there was a 12.5% increase in execution time so ~7.5 minutes longer to execute for one that takes ~1 hr.

[zanderso]

Okay, here is my suggestion:

1. We add a new builder that runs only in pre-submit.
2. It passes the --fix flag
3. It only runs when .clang-tidy or engine tree source files have changed.

The new builder definition (one example) would looks something like this: #37082

The missing piece is a new parameter to the engine_lint.py recipe to trigger the logic in this PR around the --fix flag, which would go here:

https://flutter.googlesource.com/recipes/+/refs/heads/main/recipes/engine/engine_lint.proto#8

and here

https://flutter.googlesource.com/recipes/+/refs/heads/main/recipes/engine/engine_lint.py#55

[gaaclarke]

I think we should land this as is for now. This was helpful for me for now. Let's see about turning it on by default if we get feedback from the team that addressing the lints across multiple platforms is a pain. In normal usage this might not be a big deal and more for when migrating new lint flags. Once we land sharding we'll have some more breathing room to do the fix if we want since it is reducing the number of files linted from ~800 to ~600 that should still leave us ahead.

[zanderso]

Another consideration that I just thought about is that the bots do some caching of the engine checkout to make the setup step for each run go faster. @godofredoc is there anything that could go wrong if a test step modifies the engine source checkout? Presumably, the test step should at least restore the repo to its original state, but is that sufficient?

[godofredoc]

d at least restore the repo to its original state, but is that sufficient?

Another consideration that I just thought about is that the bots do some caching of the engine checkout to make the setup step for each run go faster. @godofredoc is there anything that could go wrong if a test step modifies the engine source checkout? Presumably, the test step should at least restore the repo to its original state, but is that sufficient?

Git caches are maintained in [cache]/git and copied to [cache]/builder/src during the bot_update step. Changes to [cache]/builder/src are discarded at the end of the build execution with no impact to [cache]/git.

The changes that impact git caches are changes to the DEPS file where repositories with different commit histories are cloned using the same name. E.g. changing github.com/flutter/buildroot to github.com/godofredoc/buildroot and using commits that only exist on the godofredo's fork to run presubmits.