It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Without this change, engine changes upstream will get flagged in default flutter created apps.

What does that look like? Do we have a reasonable error message for people with existing apps?

It looks like this:

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8782821241268644385/+/u/android_lint/dart_bin_main.dart/stdout

Errr. I'm wrong. That's a linting check, not a build error.

@stuartmorgan - IIRC it would result either in a linting error if they bother to run linting (by default Flutter tooling does not run Android linting), or a runtime error if they actually ran the app on a device that does not actually have the requisite API.

If I don't hear otherwise, I'm going to mark this to submit sometime tomorrow and we can revert it if it causes breakages in package testing.

These changes LGTM as such, but:

or a runtime error if they actually ran the app on a device that does not actually have the requisite API

This is a bad failure mode; it sounds like Flutter developers could easily end up accidentally shipping apps that crash on launch on old devices without even knowing it.

In addition to this change, it seems like we should make a project migrator rule that updates any project that currently has the value set to 16, 17, or 18, and we should land that before actually breaking things in the engine. That's what we did when updating the minimum iOS version.

I agree this is a bad failure mode, but we already have this problem. We don't test anything right now on API 16-20 right now. We have no insight right now into whether the code we're shipping crashes immediately on those devices or not.

I do not think we should make a project migator for this. It's too complicated. I think it would be better if we simply detected whether someone was trying to use 16-18 and warned them that they should consider increasing it. But someone in that state is using a project that was created with what is now a pretty old version of flutter, and they haven't touched it otherwise to bump it for some plugin usage (e.g. camera requires API 21).

I do not think we should make a project migator for this. It's too complicated.

How is it more complicated than the iOS migrations we did in the same scenario? It's literally a one-line change in one file in a well known location.

I do not think we should make a project migator for this. It's too complicated.

How is it more complicated than the iOS migrations we did in the same scenario? It's literally a one-line change in one file in a well known location.

I guess it's only a problem if they've updated their java files, which is pretty unlikely.

I think this needs to be reverted until a migrator is built in framework.

Copying context from the linked revert PR: the error message seems straightforward/descriptive enough. A migrator might still be nice to have.

Also capturing discussion from Discord: this breaks the flutter/flutter -> flutter/packages roller; we can update all our examples to a min SDK of 19 to unbreak it, but that only works because we aren't actually doing any testing with 16 (even on stable). If we ever have a scenario in the future where we want to drop (on master) an SDK version that is fully supported on stable, we'll need a migrator first so that the right thing will automatically happen on both testing channels in flutter/packages. (That's how the iOS 9 -> iOS 11 migration worked.)