This repository was archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
clang-tidy: Added an environment variable that allows fix to print the generated diff. #37024
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using an environment variable, I think it would be simpler to just test for the presence of
--fix
in$@
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withFLUTTER_LINT_PRINT_FIX=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely care about postsubmit cycle time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, here is my suggestion:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.