-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[tool] Combine code analysis commands into analyze
#9860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tool] Combine code analysis commands into analyze
#9860
Conversation
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.
Code Review
This pull request refactors the code analysis tooling by consolidating the lint-android and xcode-analyze commands into a single, more flexible analyze command. This new command allows running Dart and native code analysis for various platforms (Android, iOS, macOS) together or separately, controlled by flags. The changes include updating the AnalyzeCommand to handle different analysis types, removing the old command files, and updating CI configurations and documentation to use the new command. The test suite has also been significantly updated to cover the new unified command's functionality. My review found one critical issue in the CI configuration that needs to be addressed.
|
|
||
| /// Runs Gradle lint analysis for the given package, and returns the result | ||
| /// that applies to that analysis. | ||
| Future<PackageResult> _runGradleLintForPackage( |
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.
This is runForPackage from lint_android_command.dart, with essentially no changes beyond renaming.
| } | ||
|
|
||
| /// Analyzes [plugin] for [targetPlatform]. | ||
| Future<PackageResult> _runXcodeAnalysisForPackage( |
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.
This is runForPackage from xcode_analyze_command.dart with some minor changes to make it return a PackageResult, and to include the skip logic that was a level higher in the old command. The result aggregation logic that xcode-analyze had is gone since analyze now has more generic aggregation logic that makes it unnecessary.
| final RepositoryPackage plugin1 = createFakePlugin('a', packagesDir); | ||
|
|
||
| await runCapturingPrint(runner, <String>['analyze']); | ||
| group('result aggregation', () { |
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.
This small test group is new.
| runner.addCommand(analyzeCommand); | ||
| }); | ||
|
|
||
| test('analyzes all packages', () async { |
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.
All the "removed" code is actually just wrapped in a new 'dart analyze' group.
|
|
||
| test('skips commands if all files should be ignored', () async { | ||
| createFakePackage('package_a', packagesDir); | ||
| group('gradle lint', () { |
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.
This group is the old lint-android tests, with minor string changes where I changed some logging messages.
| }); | ||
|
|
||
| // TODO(stuartmorgan): Separate iOS and macOS. | ||
| group('Xcode analyze', () { |
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.
This is the old xcode-analyze tests with very minor tweaks.
This comment was marked as spam.
This comment was marked as spam.
flutter/packages@3db5adc...5d785a0 2025-09-02 stuartmorgan@google.com [tool] Combine code analysis commands into `analyze` (flutter/packages#9860) 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-flutter-autoroll Please CC flutter-ecosystem@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
…#174840) flutter/packages@3db5adc...5d785a0 2025-09-02 stuartmorgan@google.com [tool] Combine code analysis commands into `analyze` (flutter/packages#9860) 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-flutter-autoroll Please CC flutter-ecosystem@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
…#174840) flutter/packages@3db5adc...5d785a0 2025-09-02 stuartmorgan@google.com [tool] Combine code analysis commands into `analyze` (flutter/packages#9860) 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-flutter-autoroll Please CC flutter-ecosystem@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
Folds native code analysis commands into
analyzeto simplify the set of commands:lint-androidis nowanalyze --no-dart --androidxcode-analyze --iosis nowanalyze --no-dart --iosxcode-analyze --macosis nowanalyze --no-dart --macosIn practice
--no-dartis mostly there for CI; Dart analysis is fast enough that for local plugin development the new expected use case would likely beanalyze --whateverplatorm, and it will run analysis on all relevant code in the package.Part of flutter/flutter#173413
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).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.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3