Standardize package targeting in plugin tools #83413

stuartmorgan-g · 2021-05-26T14:45:56Z

The plugin repo tooling is currently inconsistent about how its commands are targetted:

Many support a common set of flags:
- --plugins to target a specific list (should be changed to --packages ideally, since we use this tooling for flutter/packages too)
- --run-on-changed-packages to only run on changed packages (or in the case of tooling or CI changes, all packages), which we use for presubmit
- No arguments to run on everything, which we use for post-submit
Some, like version-check, always run on exactly the set of changed pubspec files
publish has its own --package, which it uses instead of --plugins
license-check always runs on every file

Also, we have --exclude as a standard option applying to type 1, but xctest has its own --skip which I believe does exactly the same thing and is jus an accident.

The inconsistencies make it harder to use the tooling, and more importantly harder to reason about changes. E.g., we want it to be the case that if a new rule is added to the script that the PR that does so finds and fixes violations, rather than them being submarine issues that break the next PR that happens to touch those plugins. Things that use option 2 don't have that property, and it's not even obvious that that's the case during review.

Option 4 is a feature for the license check since it's cheap and we always want to make sure everything is correct there, but everything else should be standardized on option 1.

The text was updated successfully, but these errors were encountered:

stuartmorgan-g · 2021-05-26T15:25:19Z

This would also be a good opportunity to extract some shared logic and standardize output, since the process of looping over each package and collecting failures could likely be shared in an intermediate Command subclass.

The xctest command has a --skip flag that is redundant with the general --exclude flag. Part of flutter/flutter#83413

…es (#4067) Most of our commands are generally of the form: ``` for (each plugin as defined by the tool flags) check some things for success or failure print a summary all of the failing things exit non-zero if anything failed ``` Currently all that logic not consistent, having been at various points copied and pasted around, modified, in some cases rewritten. There's unnecessary boilerplate in each new command, and there's unnecessary variation that makes it harder both to maintain the tool, and to consume the test output: - There's no standard format for separating each plugin's run to search within a log - There's no standard format for the summary at the end - In some cases commands have been written to ToolExit on failure, which means we don't actually get the rest of the runs This makes a new base class for commands that follow this structure to use, with shared code for all the common bits. This makes it harder to accidentally write new commands incorrectly, easier to maintain the code, and lets us standardize output so that searching within large logs will be easier. This ports two commands over as a proof of concept to demonstrate that it works; more will be converted in follow-ups. Related to flutter/flutter#83413

Switches `analyze` to the new base command that handles the boilerplate of looping over target packages. This will change the output format slightly, but shoudn't have any functional change. Updates tests to use runCapturingPrint so that test run output isn't mixed with command output. Part of flutter/flutter#83413

Switches `podspec` to the new base command that handles the boilerplate of looping over target packages. Includes test improvements: - Adds failure tests; previously no failure cases were covered. - Captures output using the standard mechanism instead of using a custom `_print`, simplifying the command code. Also removes `--skip`, which is redundant with `--exclude`. Part of flutter/flutter#83413

Switches `java-test` to the new base command that handles the boilerplate of looping over target packages. Includes test improvements: - Adds failure tests; previously no failure cases were covered. - Captures output so test output isn't spammed with command output. Part of flutter/flutter#83413

Combines the two different aspects of version-checking into a single looping structure based on plugins, using the new base command, rather than one operating on plugins as controlled by the usual flags and the other operating on a git list of changed files. Also simplifies the determination of the new version by simply checking the file, rather than querying git for the HEAD state of the file. Tests setup is simplified since we no longer need to set up nearly as much fake `git` output. Minor changes to base commands: - Move indentation up to PackageLoopingCommand so that it is consistent across commands - Add a new post-loop command for any cleanup, which is needed by version-check - Change the way the GitDir instance is managed by the base PluginCommand, so that it's always cached even when not overridden, to reduce duplicate work and code. Part of flutter/flutter#83413

) Switches build-examples to the new base command that handles the boilerplate of looping over target packages. While modifying the command, also does some minor cleanup: - Extracts a helper to reduce duplicated details of calling `flutter build` - Switches the flag for iOS to `--ios` rather than `--ipa` since `ios` is what is actually passed to the build command - iOS no longer defaults to on, so that it behaves like all the other platform flags - Passing no platform flags is now an error rather than a silent pass, to ensure that we never accidentally have CI doing a no-op run without noticing. - Rewords the logging slightly for the versions where the label for what is being built is a platform, not an artifact (which is now everything but Android). Part of flutter/flutter#83413

…4116) Migrates firebase-test-lab to use the new package-looping base command. Other changes: - Extracts several helpers to make the main flow easier to follow - Removes support for finding and running `*_e2e.dart` files, since we no longer use that file structure for integration tests. Part of flutter/flutter#83413

Significantly restructures drive-examples: - Migrates it to the new package-looping base command - Enforces that only one platform is passed, since in practice multiple platforms never actually worked. (The logic is structured so that it will be easy to enable multi-platform if `flutter drive` gains multi-platform support.) - Fixes the issue where `--ios` and `--android` were semi-broken, by doing explicit device targeting for them rather than relying on the default device being the right kind - Extracts much of the logic to helpers so it's easier to understand the flow - Removes support for a legacy integration test file structure that is no longer used - Adds more test coverage; previously no failure cases were actually tested. Fixes flutter/flutter#85147 Part of flutter/flutter#83413

…4119) To support this command's --machine flag, which moves all normal output into a field in a JSON struct, adds a way of capturing output and providing it to the command subclass on completion. Part of flutter/flutter#83413

Most of the tool operates on packages in general, and the targetting done currently by the `--plugins` flag is not actually restricted to plugins, so this makes the name less confusing. Part of flutter/flutter#83413

…lutter#4116) Migrates firebase-test-lab to use the new package-looping base command. Other changes: - Extracts several helpers to make the main flow easier to follow - Removes support for finding and running `*_e2e.dart` files, since we no longer use that file structure for integration tests. Part of flutter/flutter#83413

Significantly restructures drive-examples: - Migrates it to the new package-looping base command - Enforces that only one platform is passed, since in practice multiple platforms never actually worked. (The logic is structured so that it will be easy to enable multi-platform if `flutter drive` gains multi-platform support.) - Fixes the issue where `--ios` and `--android` were semi-broken, by doing explicit device targeting for them rather than relying on the default device being the right kind - Extracts much of the logic to helpers so it's easier to understand the flow - Removes support for a legacy integration test file structure that is no longer used - Adds more test coverage; previously no failure cases were actually tested. Fixes flutter/flutter#85147 Part of flutter/flutter#83413

…lutter#4119) To support this command's --machine flag, which moves all normal output into a field in a JSON struct, adds a way of capturing output and providing it to the command subclass on completion. Part of flutter/flutter#83413

…ter#4134) Most of the tool operates on packages in general, and the targetting done currently by the `--plugins` flag is not actually restricted to plugins, so this makes the name less confusing. Part of flutter/flutter#83413

Migrates `test` to the new package looping base command. Refactors the bulk of the logic to helpers for easier understanding of the flow. Part of flutter/flutter#83413

…4285) Replaces the `publish`-command-specific `--package` flag with support for `--packages`, and unifies the flow with the existing looping for `--all-changed`. This better aligns the command's API with the rest of the commands, and reduces divergence in the two flows (e.g., `--package` would attempt to publish and fail if the package was already published, whereas now using `--packages` will use the flow that pre-checks against `pub.dev`). It also sets up a structure that will allow easily converting it to the new base package looping command that most other commands now use, which will be done in a follow-up. Since all calls now attempt to contact `pub.dev`, the tests have been adjusted to always mock the HTTP client so they will be hermetic. Part of flutter/flutter#83413

Moves `publish` to PackageLoopingCommand, giving it the same standardized output and summary that is used by most other commands in the tool. Adds minor new functionality to the base command to allow it to handle the specific needs of publish: - Allows fully customizing the set of packages to loop over, to support --all-changed - Allows customization of a few more strings so the output better matches the functionality (e.g., using 'published' instead of 'ran' in the summary lines). Fixes flutter/flutter#83413

…es (flutter#4067) Most of our commands are generally of the form: ``` for (each plugin as defined by the tool flags) check some things for success or failure print a summary all of the failing things exit non-zero if anything failed ``` Currently all that logic not consistent, having been at various points copied and pasted around, modified, in some cases rewritten. There's unnecessary boilerplate in each new command, and there's unnecessary variation that makes it harder both to maintain the tool, and to consume the test output: - There's no standard format for separating each plugin's run to search within a log - There's no standard format for the summary at the end - In some cases commands have been written to ToolExit on failure, which means we don't actually get the rest of the runs This makes a new base class for commands that follow this structure to use, with shared code for all the common bits. This makes it harder to accidentally write new commands incorrectly, easier to maintain the code, and lets us standardize output so that searching within large logs will be easier. This ports two commands over as a proof of concept to demonstrate that it works; more will be converted in follow-ups. Related to flutter/flutter#83413

) Switches `analyze` to the new base command that handles the boilerplate of looping over target packages. This will change the output format slightly, but shoudn't have any functional change. Updates tests to use runCapturingPrint so that test run output isn't mixed with command output. Part of flutter/flutter#83413

Switches `podspec` to the new base command that handles the boilerplate of looping over target packages. Includes test improvements: - Adds failure tests; previously no failure cases were covered. - Captures output using the standard mechanism instead of using a custom `_print`, simplifying the command code. Also removes `--skip`, which is redundant with `--exclude`. Part of flutter/flutter#83413

…#4105) Switches `java-test` to the new base command that handles the boilerplate of looping over target packages. Includes test improvements: - Adds failure tests; previously no failure cases were covered. - Captures output so test output isn't spammed with command output. Part of flutter/flutter#83413

Combines the two different aspects of version-checking into a single looping structure based on plugins, using the new base command, rather than one operating on plugins as controlled by the usual flags and the other operating on a git list of changed files. Also simplifies the determination of the new version by simply checking the file, rather than querying git for the HEAD state of the file. Tests setup is simplified since we no longer need to set up nearly as much fake `git` output. Minor changes to base commands: - Move indentation up to PackageLoopingCommand so that it is consistent across commands - Add a new post-loop command for any cleanup, which is needed by version-check - Change the way the GitDir instance is managed by the base PluginCommand, so that it's always cached even when not overridden, to reduce duplicate work and code. Part of flutter/flutter#83413

…utter#4087) Switches build-examples to the new base command that handles the boilerplate of looping over target packages. While modifying the command, also does some minor cleanup: - Extracts a helper to reduce duplicated details of calling `flutter build` - Switches the flag for iOS to `--ios` rather than `--ipa` since `ios` is what is actually passed to the build command - iOS no longer defaults to on, so that it behaves like all the other platform flags - Passing no platform flags is now an error rather than a silent pass, to ensure that we never accidentally have CI doing a no-op run without noticing. - Rewords the logging slightly for the versions where the label for what is being built is a platform, not an artifact (which is now everything but Android). Part of flutter/flutter#83413

…lutter#4116) Migrates firebase-test-lab to use the new package-looping base command. Other changes: - Extracts several helpers to make the main flow easier to follow - Removes support for finding and running `*_e2e.dart` files, since we no longer use that file structure for integration tests. Part of flutter/flutter#83413

Significantly restructures drive-examples: - Migrates it to the new package-looping base command - Enforces that only one platform is passed, since in practice multiple platforms never actually worked. (The logic is structured so that it will be easy to enable multi-platform if `flutter drive` gains multi-platform support.) - Fixes the issue where `--ios` and `--android` were semi-broken, by doing explicit device targeting for them rather than relying on the default device being the right kind - Extracts much of the logic to helpers so it's easier to understand the flow - Removes support for a legacy integration test file structure that is no longer used - Adds more test coverage; previously no failure cases were actually tested. Fixes flutter/flutter#85147 Part of flutter/flutter#83413

…lutter#4119) To support this command's --machine flag, which moves all normal output into a field in a JSON struct, adds a way of capturing output and providing it to the command subclass on completion. Part of flutter/flutter#83413

…ter#4134) Most of the tool operates on packages in general, and the targetting done currently by the `--plugins` flag is not actually restricted to plugins, so this makes the name less confusing. Part of flutter/flutter#83413

Migrates `test` to the new package looping base command. Refactors the bulk of the logic to helpers for easier understanding of the flow. Part of flutter/flutter#83413

…lutter#4285) Replaces the `publish`-command-specific `--package` flag with support for `--packages`, and unifies the flow with the existing looping for `--all-changed`. This better aligns the command's API with the rest of the commands, and reduces divergence in the two flows (e.g., `--package` would attempt to publish and fail if the package was already published, whereas now using `--packages` will use the flow that pre-checks against `pub.dev`). It also sets up a structure that will allow easily converting it to the new base package looping command that most other commands now use, which will be done in a follow-up. Since all calls now attempt to contact `pub.dev`, the tests have been adjusted to always mock the HTTP client so they will be hermetic. Part of flutter/flutter#83413

…4290) Moves `publish` to PackageLoopingCommand, giving it the same standardized output and summary that is used by most other commands in the tool. Adds minor new functionality to the base command to allow it to handle the specific needs of publish: - Allows fully customizing the set of packages to loop over, to support --all-changed - Allows customization of a few more strings so the output better matches the functionality (e.g., using 'published' instead of 'ran' in the summary lines). Fixes flutter/flutter#83413

github-actions · 2021-09-14T18:00:47Z

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

…4290) Moves `publish` to PackageLoopingCommand, giving it the same standardized output and summary that is used by most other commands in the tool. Adds minor new functionality to the base command to allow it to handle the specific needs of publish: - Allows fully customizing the set of packages to loop over, to support --all-changed - Allows customization of a few more strings so the output better matches the functionality (e.g., using 'published' instead of 'ran' in the summary lines). Fixes flutter/flutter#83413

Moves `publish` to PackageLoopingCommand, giving it the same standardized output and summary that is used by most other commands in the tool. Adds minor new functionality to the base command to allow it to handle the specific needs of publish: - Allows fully customizing the set of packages to loop over, to support --all-changed - Allows customization of a few more strings so the output better matches the functionality (e.g., using 'published' instead of 'ran' in the summary lines). Fixes flutter/flutter#83413

stuartmorgan-g added c: contributor-productivity Team-specific productivity, code health, technical debt. plugin p: tooling Affects the flutter_plugin_tools package P3 Issues that are less important to the Flutter project labels May 26, 2021

stuartmorgan-g added a commit to stuartmorgan-g/plugins that referenced this issue Jun 7, 2021

[flutter_plugin_tools] Remove --skip flag

65069fb

The xctest command has a --skip flag that is redundant with the general --exclude flag. Part of flutter/flutter#83413

stuartmorgan-g mentioned this issue Jun 7, 2021

[flutter_plugin_tools] Remove xctest's --skip flutter/plugins#4022

Merged

11 tasks

stuartmorgan-g mentioned this issue Jun 17, 2021

[flutter_plugin_tools] Add a new base command for looping over packages flutter/plugins#4067

Merged

11 tasks

This was referenced Jun 23, 2021

[flutter_plugin_tools] Migrate analyze to new base command flutter/plugins#4084

Merged

[flutter_plugin_tools] Migrate build-examples to new base command flutter/plugins#4087

Merged

[flutter_plugin_tools] Overhaul drive-examples flutter/plugins#4099

Merged

This was referenced Jun 25, 2021

[flutter_plugin_tools] Migrate java-test to new base command flutter/plugins#4105

Merged

[flutter_plugin_tools] Migrate podspec to new base command flutter/plugins#4106

Merged

stuartmorgan-g mentioned this issue Jun 28, 2021

[flutter_plugin_tools] Restructure version-check flutter/plugins#4111

Merged

11 tasks

stuartmorgan-g mentioned this issue Jun 29, 2021

[flutter_plugin_tools] Migrate firebase-test-lab to new base command flutter/plugins#4116

Merged

11 tasks

stuartmorgan-g mentioned this issue Jun 30, 2021

[flutter_plugin_tools] Migrate publish-check to the new base command flutter/plugins#4119

Merged

11 tasks

stuartmorgan-g mentioned this issue Jul 2, 2021

[flutter_plugin_tools] Migrate 'test' to new base command flutter/plugins#4133

Merged

11 tasks

stuartmorgan-g mentioned this issue Jul 2, 2021

[flutter_plugin_tools] Add --packages, and deprecated --plugins flutter/plugins#4134

Merged

10 tasks

stuartmorgan-g self-assigned this Aug 27, 2021

stuartmorgan-g mentioned this issue Aug 27, 2021

[flutter_plugin_tools] Switch 'publish' from --package to --packages flutter/plugins#4285

Merged

11 tasks

stuartmorgan-g mentioned this issue Aug 28, 2021

[flutter_plugin_tool] Migrate 'publish' to new base command flutter/plugins#4290

Merged

11 tasks

stuartmorgan-g closed this as completed in flutter/plugins#4290 Aug 31, 2021

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2021

flutter-triage-bot bot added the package flutter/packages repository. See also p: labels. label Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Standardize package targeting in plugin tools #83413

Standardize package targeting in plugin tools #83413

stuartmorgan-g commented May 26, 2021 •

edited

Loading

stuartmorgan-g commented May 26, 2021

Uh oh!

github-actions bot commented Sep 14, 2021

Uh oh!

Standardize package targeting in plugin tools #83413

Standardize package targeting in plugin tools #83413

Comments

stuartmorgan-g commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

stuartmorgan-g commented May 26, 2021

Uh oh!

github-actions bot commented Sep 14, 2021

Uh oh!

stuartmorgan-g commented May 26, 2021 •

edited

Loading