[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[many] Remove references to v1 embedding #6494

Merged
merged 24 commits into from
May 28, 2024
Merged

[many] Remove references to v1 embedding #6494

merged 24 commits into from
May 28, 2024

Conversation

gmackall
Copy link
Member
@gmackall gmackall commented Apr 9, 2024 •
edited by reidbaker
Loading

#6923 is a follow up pr that includes more migration steps. Both will be required to build after v1 embedding classes are removed.

I have a WIP effort to remove the v1 embedding entirely (flutter/flutter#146523 is the latest pr).

registerWith references the v1 embedding, which has been deprecated for many years, so this PR removes it from all plugins.

Also removes some additional references, see in particular these three commits:

  1. Modifies private ActivityState class to remove PluginRegistry.Registrar member in image_picker: c2e4c87
  2. Replaces FlutterMain.getLookupKeyForAsset() with FlutterLoader.getLookupKeyForAsset() in google_maps plugin: 73c3de3
  3. Removes deprecated RegistrarFlutterAssetManager class in webview_flutter: cc842c6

Fixes flutter/flutter#70923

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gmackall
Copy link
Member Author
gmackall commented Apr 9, 2024

cc @stuartmorgan I was unaware that we had recommended folks include the static registerWith, for compatibility with v1 apps, until recently.

This means that the deletion of the v1 embedding will be a breaking change for most plugins, though the solution is simply to remove that static method.

Do you have any opinions on: how to make this the least painful/would you LGTM the deletion in the first place/would you want to keep the interface needed around, so we can have a sort of no-op registerWith while we advise people to delete it?

@stuartmorgan
Copy link
Contributor
stuartmorgan commented Apr 9, 2024 •
edited
Loading

It's been a while since I was in conversations around the v1 wind-down, so I don't have all the context any more. What's the current state of v1 support on stable? IIRC the last I had been involved it was being moved behind a flag, but it was still possible to use that flag to force build an app in v1 mode. Is that still the case?

The way I would want this to go is:

  • Make v1 fully impossible to use on the tool side (may already be done).
  • Once that's on stable, we can delete the v1 support from all of our plugins (updating the min supported Flutter version if necessary).
  • We can also do outreach to top plugins at that point to let them know they can remove it as well.

I think I'd like to keep registerWith in the interface, with a prominent comment saying it will never be called and is only there to avoid breaking compilation of plugin, for a while; it should be pretty low-impact for us to keep just that declaration. Eventually we could remove it and break plugins on master, but it would be good to have that be a while after all the big plugins (including ours) have had a chance to remove it.

@gmackall
Copy link
Member Author
gmackall commented Apr 9, 2024 •
edited
Loading

What's the current state of v1 support on stable?

Currently still possible to build a v1 app by passing a flag (will error without the flag), though that will change in the next release (once flutter/flutter#144726 gets picked up).

For full transparency we have been attempting to approach it faster, as a flat out deletion rather than wind down, given how long the v1 has been deprecated at this point. But this particular case seems like it will require more messaging before deletion is possible, given our own recommendations put plugin authors in this situation.

I think I'd like to keep registerWith in the interface

To be a little nitpicky, registerWith itself isn't in the interface, it is just an additional static method we have recommended people include. But (if people follow our recommendation) that static method references the inner type io.flutter.plugin.common.PluginRegistry.Registrar, which is part of the v1 embedding.

But I'm fine leaving that interface around (with a comment warning it is slated for deletion) for a release to give plugin authors time to migrate, while deleting the rest of the v1 embedding now. Does that sound reasonable to you? Also I'm happy to help reach out to the top plugin authors, if we have a list somewhere.

@gmackall
Copy link
Member Author
gmackall commented Apr 9, 2024

Also in light of

Once that's on stable, we can delete the v1 support from all of our plugins (updating the min supported Flutter version if necessary).

I'll get this pr to tests passing, and then leave it around until the next release before requesting review.

This was referenced Apr 9, 2024
Closed
Merged
@stuartmorgan
Copy link
Contributor

For full transparency we have been attempting to approach it faster, as a flat out deletion rather than wind down

Right, I was thinking of the entire process of "fully supported" to "completely gone" when I say wind-down. I've been involved in discussions on and off since very early in the process.

To be a little nitpicky, registerWith itself isn't in the interface, it is just an additional static method we have recommended people include. But (if people follow our recommendation) that static method references the inner type io.flutter.plugin.common.PluginRegistry.Registrar, which is part of the v1 embedding.

Ah, that's more unfortunate in terms of code volume. But since it's just an interface we could strip out all the comments and at least get it down to a pretty small amount.

But I'm fine leaving that interface around (with a comment warning it is slated for deletion) for a release to give plugin authors time to migrate, while deleting the rest of the v1 embedding now. Does that sound reasonable to you?

Yes, I'm on board with that. The main thing I had pushed back on in the past was deleting the v1 support from our plugins while the tool still supported v1 builds, because it seemed backward. It's much smoother if we delete things from the tool up (just as we do with dropping old OS versions).

Also I'm happy to help reach out to the top plugin authors, if we have a list somewhere.

I'll get an updated list and do the outreach once the change hits stable; it's not that long a list so I can do it pretty quickly.

@stuartmorgan stuartmorgan added the waiting for stable update Can't be landed until functionality reaches the stable channel label Apr 10, 2024
@gmackall gmackall mentioned this pull request Apr 10, 2024
Merged
8 tasks
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 10, 2024
@gmackall
Remove additional references to engine v1 android embedding (#146523)
286c73c 
flutter/engine#51229 blocked [the roll](#146522) and had to be reverted, which is a shame, but on the bright side it made it possible to point the framework at my removal pr, at the point of its merging the first time 

This fixes all errors that are fixable in the framework that would have blocked the roll. There are some that aren't fixable here (they need to be fixed in the engine)*, so I'll fix those in the engine but unfortunately I can't pick up another version here to re-test until I try to roll again � 

*This category is: uses of plugins that in turn have a `registerWith`, that references the v1 embedding. The plan to fix these cases is to leave the interface that that method relies on around for now. See flutter/packages#6494 (comment) for details
@johnmccutchan
Copy link
Contributor

I'm not sure I understand why we aren't landing this PR now.

  1. The code has been removed from flutter tool.
  2. There are no tests in this repository (or any repository) that are testing the functionality of v1 embedder.
  3. The Android v1 embedder has been broken since July 2023 (I intentionally didn't implement necessary functionality)

Why are we keeping dead and untested code around in our repository?

@stuartmorgan
Copy link
Contributor

I'm not sure I understand why we aren't landing this PR now.

  1. The code has been removed from flutter tool.

According to this comment it's been removed on master, but not stable. We have an explicit policy in this repository of supporting stable for our packages.

  1. There are no tests in this repository (or any repository) that are testing the functionality of v1 embedder.

Not having tests for v1 here doesn't mean we can intentionally break v1 support. It's in the same category as "best effort" OS support.

  1. The Android v1 embedder has been broken since July 2023 (I intentionally didn't implement necessary functionality)

Can you elaborate on what "broken" means, concretely? What happens, right now, if someone attempts to build an app on stable with the v1 flag?

@johnmccutchan
Copy link
Contributor

I'm not sure I understand why we aren't landing this PR now.

  1. The code has been removed from flutter tool.

According to this comment it's been removed on master, but not stable. We have an explicit policy in this repository of supporting stable for our packages.

  1. There are no tests in this repository (or any repository) that are testing the functionality of v1 embedder.

Not having tests for v1 here doesn't mean we can intentionally break v1 support. It's in the same category as "best effort" OS support.

This is an API that was deprecated 5+ years ago and has no users.

  1. The Android v1 embedder has been broken since July 2023 (I intentionally didn't implement necessary functionality)

Can you elaborate on what "broken" means, concretely? What happens, right now, if someone attempts to build an app on stable with the v1 flag?

The app will build but fail at runtime.

@stuartmorgan
Copy link
Contributor

This is an API that [...] and has no users.
[...]
The app will build but fail at runtime.

Both of these things are completely new information to me. Is there more information on this somewhere?

This was referenced Aug 8, 2024
Open
Open
Closed
Open
Closed
Open
Open
Open
Open
Open
Closed
Closed
Open
Open
Open
Closed
Open
Open
Closed
Closed
Open
Open
Open
Open
Closed
Open
Open
Closed
Open
@JayShortway JayShortway mentioned this pull request Aug 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@reidbaker reidbaker reidbaker approved these changes

@bparrishMines bparrishMines Awaiting requested review from bparrishMines bparrishMines is a code owner

@camsim99 camsim99 Awaiting requested review from camsim99 camsim99 is a code owner

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera p: espresso p: flutter_plugin_android_lifecycle p: google_maps_flutter p: google_sign_in p: image_picker p: in_app_purchase p: local_auth p: path_provider p: quick_actions p: shared_preferences p: url_launcher p: video_player p: webview_flutter platform-android waiting for stable update Can't be landed until functionality reaches the stable channel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[google_maps_flutter] FlutterMain has been deprecated
4 participants
@gmackall @stuartmorgan @johnmccutchan @reidbaker