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.

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

Remove temporary desktop plugin dartPluginClass workaround #57497

Open
stuartmorgan opened this issue May 18, 2020 · 3 comments
Open

Remove temporary desktop plugin dartPluginClass workaround #57497

stuartmorgan opened this issue May 18, 2020 · 3 comments
Labels
a: desktop Running on desktop c: tech-debt Technical debt, code quality, testing, etc. P3 Issues that are less important to the Flutter project platform-linux Building on or for Linux specifically platform-mac Building on or for macOS specifically platform-windows Building on or for Windows specifically team-desktop Owned by Desktop platforms team tool Affects the "flutter" command-line tool. See also t: labels. triaged-desktop Triaged by Desktop platforms team

Comments

@stuartmorgan
Copy link
Contributor

On 1.18 (current master), desktop plugins can either specify a pluginClass for a standard plugin with native code, or a dartPluginClass to allow that platform's implementation of a plugin to be Dart-only. However, the current stable build, 1.17, doesn't have this logic, and requires a pluginClass for all desktop platforms. When building for any platform, all platforms listed in the pubspec are validated, which means that publishing a plugin with:

  plugin:
    platforms:
        dartPluginClass: Foo

will break the iOS and Android builds of any project that ends up with a dependency on that plugin. That means that for example we can't publish a Dart-only Linux implementation of path_provider and endorse it without breaking everyone on stable.

To work around this, I propose temporarily adding special handling to the desktop platforms where having pluginClass: none is treated the same way as having no pluginClass field. This will pass validation on 1.17, but be a no-op on master. The workaround would be removed once 1.18 is released to stable.

Minor downsides:

  • Desktop builds with older versions of master will break when using one of these plugins. It's master though, so it's reasonable to say that you need to be using the latest version if you want to use published plugins on desktop.
  • If someone is building on desktop using another channel and bypassing our check (e.g., changing to a local branch at the same revision but a different name) they will break when using one of these plugins. However, that's not a supported configuration, so I'm fine with that tradeoff. Someone who really wants to do this can always patch their local copy with the workaround, or pin to a version of the plugin that doesn't endorse a plugin using this.
@stuartmorgan stuartmorgan added tool Affects the "flutter" command-line tool. See also t: labels. platform-mac Building on or for macOS specifically platform-windows Building on or for Windows specifically platform-linux Building on or for Linux specifically a: desktop Running on desktop labels May 18, 2020
@stuartmorgan stuartmorgan self-assigned this May 18, 2020
stuartmorgan added a commit to stuartmorgan/flutter that referenced this issue May 18, 2020
Treats 'pluginClass: none' as equivalent to having no 'pluginClass'
entry on the desktop platforms, to satisy stable channel plugin
validation of Dart-only desktop plugin implementations. See
issue for full details.

Part of flutter#57497
stuartmorgan added a commit that referenced this issue May 18, 2020
Treats 'pluginClass: none' as equivalent to having no 'pluginClass'
entry on the desktop platforms, to satisy stable channel plugin
validation of Dart-only desktop plugin implementations. See
issue for full details.

Part of #57497
@jmagman jmagman added this to Awaiting triage in Tools - plugin and package support review via automation May 19, 2020
@christopherfujino christopherfujino moved this from Awaiting triage to Engineer reviewed in Tools - plugin and package support review May 19, 2020
@stuartmorgan stuartmorgan changed the title Add, then remove, temporary desktop plugin dartPluginClass workaround Remove, temporary desktop plugin dartPluginClass workaround May 19, 2020
@stuartmorgan
Copy link
Contributor Author

stuartmorgan commented May 19, 2020

The proposal above was added; this issue now tracks removing the workaround. This is two-phase:

  • Remove pluginClass: none from the plugins
  • Remove the special handling of none from flutter_tools

We should likely leave a while between those steps, since the second will break anyone still building with a version of the plugin from before step 1.

@stuartmorgan stuartmorgan changed the title Remove, temporary desktop plugin dartPluginClass workaround Remove temporary desktop plugin dartPluginClass workaround May 20, 2020
@stuartmorgan stuartmorgan added this to the 1.23 - October 2020 milestone Jun 3, 2020
@stuartmorgan stuartmorgan added the P3 Issues that are less important to the Flutter project label Jun 9, 2020
@stuartmorgan
Copy link
Contributor Author

This will require all of the relevant plugins to require 1.20+. That's something that also came up with regard to removing the iOS stubs from all the non-iOS federated plugins.

@amirh Is there any existing plan to mass-upgrade the requirements for flutter/plugins? If not, what's the best way to drive that discussion?

@Hixie Hixie modified the milestones: 1.24 - October 2020, Overdue Nov 10, 2020
@Hixie Hixie removed this from the Overdue milestone Dec 10, 2020
@stuartmorgan stuartmorgan added the c: tech-debt Technical debt, code quality, testing, etc. label Jun 10, 2021
stuartmorgan added a commit to stuartmorgan/plugins that referenced this issue Nov 4, 2021
This workaround has not been needed since Flutter 1.20, and all plugins
now require at least Flutter 2.0, so it can be safely removed.

Part of flutter/flutter#57497
stuartmorgan added a commit to flutter/plugins that referenced this issue Nov 10, 2021
This workaround has not been needed since Flutter 1.20, and all plugins
now require at least Flutter 2.0, so it can be safely removed.

Part of flutter/flutter#57497
amantoux pushed a commit to amantoux/plugins that referenced this issue Dec 11, 2021
This workaround has not been needed since Flutter 1.20, and all plugins
now require at least Flutter 2.0, so it can be safely removed.

Part of flutter/flutter#57497
KyleFin pushed a commit to KyleFin/plugins that referenced this issue Dec 21, 2021
This workaround has not been needed since Flutter 1.20, and all plugins
now require at least Flutter 2.0, so it can be safely removed.

Part of flutter/flutter#57497
@stuartmorgan stuartmorgan removed their assignment Jan 12, 2022
@stuartmorgan
Copy link
Contributor Author

We're no longer using it in flutter/plugins. We should eventually remove the special-casing, but there's no hurry on it and it's a breaking change.

ValentinVignal pushed a commit to ValentinVignal/flutter__packages that referenced this issue Feb 23, 2023
This workaround has not been needed since Flutter 1.20, and all plugins
now require at least Flutter 2.0, so it can be safely removed.

Part of flutter/flutter#57497
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-desktop Owned by Desktop platforms team triaged-desktop Triaged by Desktop platforms team labels Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop c: tech-debt Technical debt, code quality, testing, etc. P3 Issues that are less important to the Flutter project platform-linux Building on or for Linux specifically platform-mac Building on or for macOS specifically platform-windows Building on or for Windows specifically team-desktop Owned by Desktop platforms team tool Affects the "flutter" command-line tool. See also t: labels. triaged-desktop Triaged by Desktop platforms team
Projects
Development

No branches or pull requests

3 participants