Add more safeties against breaking the ecosystem with federated plugin PRs

[stuartmorgan]

flutter/plugins#4288 was an ecosystem-breaking PR that passed both presubmit and postsubmit (and was thus auto-published), demonstrating that we need a stronger safety system.

In particular, it looks like the CI did correctly detect that the isolated change to the interface package was wrong (a breaking change, but not marked as such)—I assume via the build_all_plugins test, but can't tell because of force-pushing that happened in the PR—but then that safety was disabled by the addition of unpublished source changes to a different package in the same PR. That should absolutely not have happened, and ideally should have been caught in review, but it was missed and the PR was approved with that change.

We should have something that's not subject to human error that would prevent that. The tricky part is going to be figuring out how to do it in a way that doesn't have false positives that prevent reasonable multi-package changes (e.g., something that is fixing a new analyzer warning that involves changing both interface and implementation packages). Worst case we could force the false positives to just require separate PRs, but I'd rather not do that if possible.

[stuartmorgan]

Actually, now that I think about this more I don't understand how it passed CI in the form it was landed. image_picker_for_web's tests should have been triggered by the change to one of its files, and its tests should have been run against the published image_picker_platform_interface, and thus failed. I'll need to investigate further.

[stuartmorgan]

Oh, right. A subclass override can add optional named parameters in Dart, so the web change in isolation is actually a valid change against the original code.

[stuartmorgan]

So far I see two ways we could have caught this:

1. Whenever anything in *_platform_interface/lib/* is changed, run a Dart AST analysis on the head version and the previous version, see if the parameter list of any existing method has changed in any way, and if so fail unless there's a major version bump.
   • (+) Should have no false positives.
   • (+) Would catch things that weren't caught by any implementation packages (e.g., if no in-tree implementation overrides the method in question), although this seems very unlikely to actually come up in practice.
   • (-) Only catches the specific kind of errors we explicitly check for. E.g., implementing just this as described would not catch a breaking change to an enum, until/unless we thought (or more likely, found the hard way) each specific case and added checks for it.
   • (-) More complex to write and maintain.
2. Forbid code changes to foo_platform_interface and foo/foo_* in the same PR.
   • (-) Will have false positives. These can be reduced by requiring that the platform interface (PI) change be in lib/, and optionally by there being a change to foo_platform_interface/pubspec.yaml and CHANGELOG.md to filter out unpublished changes.
   • (+) Should catch anything that would actually break any 1P implementation if actually published.
   • (+) Relatively simple to check.

Given the added safety I'm strongly inclined toward #2. To see how much of an impact that's likely to have, I checked the repo history for landed PRs that would trip that check. There have been 26 (using the lib filter but not the publish filter; the latter drops it to 19):

• (-) 5 were mass changes (e.g., copyright block cleanup) that were reasonable changes that would have been blocked
  • Most of these could have been filtered out by one or both of:
    • Ignoring unpublished PI changes.
    • Ignoring changes that touch code in more than one conceptual plugin. (This would reduce safety slightly in theory, but a PR that makes substantive changes to plugin A but also randomly touches files in plugin B seems much more likely to be caught in review).
• (+) 2 were the issue that spawned this and its revert
• (+) 1 was from early in the days of federation and was landing a multi-stage PR as one PR with instructions about publishing only part of it, doing a follow-up PR to adjust dependencies, and then publishing the second. I.e., something that would be banned by our current process for safety reasons.
• (/) 7 were plugin restructurings, mostly around the initial conversion of plugins to the federated structure, where there was some amount of bleed between the folder restructuring PR and the creation of the PI package (e.g., part 1 was moving the monolithic package to a subfolder, part 2 was the creation of the PI, but part 2 included moving or deleting a file that was left behind in part 1)
• (-?) 7 were minor cleanups to PI comments while making a change in an implementation. Generally these were adding comments to PI methods that said things like "On Android, this does X because Y". I'm marking this (-?) because it's not clear to me that's actually a good change in general; the PI package shouldn't be where we document details of implementations. On the other hand, maybe that's to some extent unavoidable.
  • In most of these cases the PI part was an unpublished change.
• (/) 2 were making orthogonal but related changes to two packages. I'm counting this as neutral because while it didn't hurt anything to have them in one PR and it was slightly easier for the author that way, it would have been trivial for them to be 2 PRs, and smaller, more focused PRs are generally better.
• (+) 2 were making changes that were really unrelated in two packages. I.e., someone was working on the plugin, fixed something in one sub-package, noticed something in another, and changed that in the same PR. They weren't bad per se, but would have been better as two PRs.

Since almost all the negatives (-) would can be screened out by additional checks, and the neutrals (/) don't seem like they would have been particularly annoying to adjust the PRs for, my plan is to do 2 with the added checks—with a very clear error message describing options—and we'll see how problematic it is in practice. If we find that we're often having to jump through hoops to work around it, we can revisit. My impression from the audit is that it will give us significant added safety without much cost in annoyance.

[github-actions]

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.