-
Notifications
You must be signed in to change notification settings - Fork 6k
Add et lint command #51238
Add et lint command #51238
Conversation
johnmccutchan
commented
Mar 6, 2024
- Invokes lints for dart, python, c, and java.
- Captures all output of executions into an artifacts file (a json file).
- Runs lints concurrently.
- Cool status display while running.
- Tests.
matanlurey
left a comment
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.
Seems reasonable.
Most of my comments are optional, feel free to do the ones you have time for, and I'd leave TODOs the ones you agree with but want to us to collectively handle post-merge.
| // TODO(loic-sharma): Relax this restriction. | ||
| if (environment.platform.isWindows) { | ||
| environment.logger | ||
| .error('lint command is not supported on Windows (for now).'); |
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 should consider just adding a logger.fatal that:
- Throws
FatalError(...) - The root program catches it, displays it (in verbose mode, with a stack trace), and returns 1
| return 1; | ||
| } | ||
| final bool verbose = globalResults![verboseFlag] as bool; | ||
| final ProcessQueue pq = ProcessQueue(environment); |
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.
Zach has some similar classes for this in other parts of tools/, I'd check if we could either use it, or at least make sure we're not creating multiple things to do roughly the same thing (I'd look in tools/clang_tidy as a start).
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.
| } | ||
|
|
||
| /// Returns the dart-sdk/bin directory. | ||
| String findDartBinDirectory(Environment env) { |
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.
bin/et{.bat} invokes the engine tool entrypoint using the prebuilt dart binary under prebuilts, so you might also just use the parent directory of env.platform.resolvedExecutable for this.
|
auto label is removed for flutter/engine/51238, Failed to merge flutter/engine/51238 with Pull request flutter/engine/51238 could not be merged: Head branch is out of date. Review and try the merge again.. |
…145067) flutter/engine@285b9fb...2871c26 2024-03-13 [email protected] Add et lint command (flutter/engine#51238) 2024-03-13 [email protected] Refactor `golden_tests_harvester`, throw when not `--dry-run`, add tests. (flutter/engine#51364) 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 [email protected],[email protected],[email protected] 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