refactor: Differentiate pubspec and resolution errors for plugins #142035

Gustl22 · 2024-01-23T08:15:30Z

Differentiate pubspec and resolution errors
Rename platform to platformKey
Add TODO for rework logic of flag [throwOnPluginPubspecError]
Swap for loop: handle by platform and then by plugin

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

Gustl22 · 2024-01-23T08:58:45Z

The flag throwOnPluginPubspecError never actually was enabled during production in #79669, but only in some tests. And in the tests the outcome of the flag was not explicitly tested.
Should we enable this flag during dart plugin registrant generation? If so, is there a reason to keep the flag, as it is always enabled then (?)

This reverts commit b5a8351.

Gustl22 · 2024-02-04T14:59:33Z

Cc @stuartmorgan regarding the TODO on throwOnPluginPubspecError

packages/flutter_tools/lib/src/flutter_plugins.dart

christopherfujino · 2024-02-08T22:24:22Z

packages/flutter_tools/test/general.shard/dart_plugin_test.dart

@@ -538,7  538,7 @@ void main() {

        },
        throwsToolExit(


while we're at it, can we add a test for the pubspec error?

I also thought this until I realized that the implementation for the pubspec error doesn't work: the reason is that the covered case for this error is: when a native federated inline plugin implementation is used, it does not have to be mentioned as interface or plugin in the pubspec (except from flutter:\n plugin:\n platforms:\n $myplatform:\n pluginClass: MyPluginClass). This case does not occur in the dart only plugin case, therefore the flag throwOnPluginPubspecError can be enabled.

If I write a test, it covers a case, which actually should not be covered, as the same pubspec is used for plugins with mixed implementations (dart and inline native ones).
Also removing or changing a test then means a breaking change, which is not the case, as the feature was never enabled.

At least, that's my theory. I can post some tests here to validate that theory.

I added a test via 70af56b and if enabling throwing with pubspec error (like for the other dart plugin tests), it throws the pubspec error. Although it shouldn't, as the test includes a valid native plugin implementation. I see no case, where the plugin is recognized as one, but then doesn't provide an implementation, so this pubspec error message makes no sense to me.

Therefore the whole error paragraph should be removed. Right now it throws, but then is catched in production, which acts like a continue statement, which would be the correct implementation (at least for resolving dart only plugins).

I would prefer to do this step in a separate PR though.

Gustl22 · 2024-02-15T16:30:01Z

@christopherfujino would you be interested in continuing the review. I appreciate it :)

Gustl22 · 2024-02-15T20:22:58Z

I'm quite confused, how the changes can result in flaky tests (?) Does anyone know these type of errors (flaky error 1, flaky error 2)?

christopherfujino · 2024-02-16T20:54:49Z

I'm quite confused, how the changes can result in flaky tests (?) Does anyone know these type of errors (flaky error 1, flaky error 2)?

Those look like issues with the pub server being down, and unrelated to your PR (package:lints version 3.0.0 was published 4 months ago)--I triggered a re-run of Mac tool_integration_tests_1_4

packages/flutter_tools/lib/src/flutter_plugins.dart

stuartmorgan · 2024-02-20T19:35:08Z

packages/flutter_tools/lib/src/flutter_plugins.dart

-    for (final String platform in platforms) {
-      if (plugin.platforms[platform] == null &&
-          plugin.defaultPackagePlatforms[platform] == null) {
+  for (final String platformKey in platformKeys) {


Why the variable rename here from platform to platformKey? Is there some other representation of a platform in the context of this method to distinguish from? I'm only seeing the string.

In my opinion this not only depends on the context of the current method, but on the readability of the whole file.

It could be confused with the Platform class:

flutter/packages/flutter_tools/lib/src/flutter_plugins.dart

Line 1033 in 64c63d3

required Platform platform,

Also platformKey is already used 13 times in this file, where it also not always has a conflicting "representation", and it would be good to keep that word consistent with its meaning across that file to improve overall readability:
https://github.com/search?q=repo:flutter/flutter platformKey&type=code

The method structure and affiliations in this file are already hard to understand, which I'm trying to simplify.

Gustl22 · 2024-02-23T17:56:23Z

@christopherfujino is the test sufficient?
@stuartmorgan is the explanation for throwOnPluginPubspecError sufficient.

I know you have a lot on the plate. Therefore I tried to make the changes/refactors small to be able to review them faster. But this may is a false expectation. Do you have any advice to push the goal of faster development? Thanks in advance and sorry for tagging you.

stuartmorgan · 2024-02-23T19:01:26Z

Therefore I tried to make the changes/refactors small to be able to review them faster. But this may is a false expectation.

It is reasonable to expect that smaller PRs will generally be reviewed faster than larger and more complex PRs. "Faster" won't mean "right away", however.

Do you have any advice to push the goal of faster development?

It wouldn't hurt to be explicit about review requests. When you initially tagged me it was as a "CC", and I don't read "CC" as "please review", nor does it cause the PR to show up in my GitHub review queue. Similarly, if you are ready for a re-review you can use the GitHub UI to re-request review. Many people post comments even when they haven't finished addressing feedback, so you should not assume that just because you post a response to something it will trigger a new review.

Gustl22 · 2024-02-23T19:09:22Z

Thank you for the explanation. I'll pay attention to this in my future PRs and comments.

christopherfujino

LGTM. With regards to expediting code review:

the tools team weekly looks at all outstanding PRs on Thursdays. However, as this is a no-meetings week for us, I cancelled the meeting--intending to look through open PRs on my own but forgot. This is my fault.
Tool code review is largely bottlenecked by the time I can find for it. I see no short term fix for this. Had Stuart approved this I would be happy with merging (although my understanding is that Stuart has an even greater review queue than I do). Long term if we had more tool contributors who shared the code review load that would increase the velocity of tool PRs getting merged.

In any case, I appreciate your contributions, and I hope the delays didn't discourage you!

stuartmorgan

LGTM

…gins (flutter/flutter#142035)

flutter/flutter@b77560e...c30f998 2024-02-27 [email protected] [flutter_tools] Fix missing stack trace from daemon (flutter/flutter#144113) 2024-02-27 [email protected] Roll Flutter Engine from 04ff2868ce80 to 0bc21ea7bc92 (21 revisions) (flutter/flutter#144208) 2024-02-26 [email protected] Move debugShowWidgetInspectorOverride (flutter/flutter#144029) 2024-02-26 [email protected] Allow `Listenable.merge()` to use any iterable (flutter/flutter#143675) 2024-02-26 54558023 [email protected] Mark firebase_release_smoke_test as `bringup: true` (flutter/flutter#144186) 2024-02-26 [email protected] refactor: Differentiate pubspec and resolution errors for plugins (flutter/flutter#142035) 2024-02-26 [email protected] Roll Flutter Engine from 34a8b9bdaaac to 04ff2868ce80 (1 revision) (flutter/flutter#144159) 2024-02-26 [email protected] Implementing `switch` expressions in `rendering/` (flutter/flutter#143812) 2024-02-26 [email protected] Roll Flutter Engine from 7a573c21a4ad to 34a8b9bdaaac (1 revision) (flutter/flutter#144147) 2024-02-26 [email protected] Roll Flutter Engine from a15326b2c439 to 7a573c21a4ad (1 revision) (flutter/flutter#144145) 2024-02-26 15619084 [email protected] Update copyDirectory to allow links to not be followed (flutter/flutter#144040) 2024-02-26 [email protected] Roll Packages from 7df2085 to 353086c (7 revisions) (flutter/flutter#144144) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

…on (#144214) Part of #137040 and #80374 The flag `throwOnPluginPubspecError` never actually was enabled during production in #79669, but only in some dart plugin tests. And in the tests the case of the error when enabling the flag was not explicitly tested. The only thing tested was, that it is not thrown when disabled. As explained [here](#142035 (comment)) the only case, where this error could be thrown is, when a dart implementation and a native inline implementation are provided simultaneously. But throwing an exception there is a wrong behavior, as both can coexist in a plugin package, thus in the pubspec file. Disabling the flag means, that the error is not thrown and not shown to the user. This is the case in production (contrary to the dart plugin tests), which acts like these plugin cases of implementations are just skipped. And this is what actually should be done. In conclusion, I think the case of coexisting dart and native implementation in pubspec was just overlooked and therefore this error validation was introduced, which is not necessary or even valid. For more discussion, see: https://discord.com/channels/608014603317936148/608022056616853515/1200194937791205436 - This is tricky: I already added a test in #142035, which finally complies with the other tests, by removing the flag. So I think it falls in the category of "remove dead code". - Theoretically this is a breaking change, as removing / altering some tests. But the flag actually was never valid or used, so IDK. But this may not does fall in the category of "contributed tests".

…gins (flutter/flutter#142035)

flutter/flutter@b77560e...c30f998 2024-02-27 [email protected] [flutter_tools] Fix missing stack trace from daemon (flutter/flutter#144113) 2024-02-27 [email protected] Roll Flutter Engine from 04ff2868ce80 to 0bc21ea7bc92 (21 revisions) (flutter/flutter#144208) 2024-02-26 [email protected] Move debugShowWidgetInspectorOverride (flutter/flutter#144029) 2024-02-26 [email protected] Allow `Listenable.merge()` to use any iterable (flutter/flutter#143675) 2024-02-26 54558023 [email protected] Mark firebase_release_smoke_test as `bringup: true` (flutter/flutter#144186) 2024-02-26 [email protected] refactor: Differentiate pubspec and resolution errors for plugins (flutter/flutter#142035) 2024-02-26 [email protected] Roll Flutter Engine from 34a8b9bdaaac to 04ff2868ce80 (1 revision) (flutter/flutter#144159) 2024-02-26 [email protected] Implementing `switch` expressions in `rendering/` (flutter/flutter#143812) 2024-02-26 [email protected] Roll Flutter Engine from 7a573c21a4ad to 34a8b9bdaaac (1 revision) (flutter/flutter#144147) 2024-02-26 [email protected] Roll Flutter Engine from a15326b2c439 to 7a573c21a4ad (1 revision) (flutter/flutter#144145) 2024-02-26 15619084 [email protected] Update copyDirectory to allow links to not be followed (flutter/flutter#144040) 2024-02-26 [email protected] Roll Packages from 7df2085 to 353086c (7 revisions) (flutter/flutter#144144) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

Gustl22 added 6 commits January 23, 2024 00:24

Rename platforms to platformKeys

4fb9a09

format

00714dc

platformKey

e38a53d

Replace with putIfAbsent

405dcae

More precise error description

dfa7a09

WIP: don't throw pubspec error

b5a8351

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 23, 2024

Gustl22 added 4 commits January 23, 2024 10:00

Revert "WIP: don't throw pubspec error"

eb4a4b7

This reverts commit b5a8351.

WIP: enable to throw on pubspec error

2d308b3

Merge remote-tracking branch 'upstream/main' into 80374-refactor-2

a531b6b

Add TODO for flag [throwOnPluginPubspecError]

d3e8b97

Gustl22 changed the title ~~refactor: Differentiate pubspec and resolution error, platform to platformKey~~ refactor: Differentiate pubspec and resolution error Feb 4, 2024

Gustl22 changed the title ~~refactor: Differentiate pubspec and resolution error~~ refactor: Differentiate pubspec and resolution error for plugins Feb 4, 2024

Gustl22 changed the title ~~refactor: Differentiate pubspec and resolution error for plugins~~ refactor: Differentiate pubspec and resolution errors for plugins Feb 4, 2024

Gustl22 marked this pull request as ready for review February 4, 2024 12:23

christopherfujino reviewed Feb 8, 2024

View reviewed changes

Gustl22 added 5 commits February 9, 2024 13:00

Test mixed dart & native implementations

70af56b

Merge branch 'master' into 80374-refactor-2

58889e8

Merge branch 'master' into 80374-refactor-2

c7eff5f

Merge branch 'master' into 80374-refactor-2

bd09ea1

Merge branch 'master' into 80374-refactor-2

aba57f3

Merge branch 'master' into 80374-refactor-2

4a1a5f6

Merge branch 'master' into 80374-refactor-2

bf715ba

stuartmorgan reviewed Feb 20, 2024

View reviewed changes

change error message

272e07d

Gustl22 requested review from stuartmorgan and christopherfujino February 23, 2024 19:09

christopherfujino approved these changes Feb 23, 2024

View reviewed changes

stuartmorgan approved these changes Feb 26, 2024

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 26, 2024

auto-submit bot merged commit b4f925e into flutter:master Feb 26, 2024
121 checks passed

Gustl22 deleted the 80374-refactor-2 branch February 26, 2024 23:05

Gustl22 mentioned this pull request Feb 27, 2024

refactor: Remove throwOnPluginPubspecError flag for plugin validation #144214

Merged

9 tasks

engine-flutter-autoroll mentioned this pull request Feb 27, 2024

Roll Flutter from b77560e92a58 to c30f998eb5db (12 revisions) flutter/packages#6211

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2024

b4f925e refactor: Differentiate pubspec and resolution errors for plu…

eb424a6

…gins (flutter/flutter#142035)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

b4f925e refactor: Differentiate pubspec and resolution errors for plu…

c18b2c6

…gins (flutter/flutter#142035)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Differentiate pubspec and resolution errors for plugins #142035

refactor: Differentiate pubspec and resolution errors for plugins #142035

Gustl22 commented Jan 23, 2024 •

edited

Loading

Gustl22 commented Jan 23, 2024 •

edited

Loading

Gustl22 commented Feb 4, 2024

christopherfujino Feb 8, 2024

Gustl22 Feb 9, 2024 •

edited

Loading

Gustl22 Feb 9, 2024 •

edited

Loading

Gustl22 commented Feb 15, 2024

Gustl22 commented Feb 15, 2024 •

edited

Loading

christopherfujino commented Feb 16, 2024 •

edited

Loading

stuartmorgan Feb 20, 2024

Gustl22 Feb 20, 2024 •

edited

Loading

Gustl22 commented Feb 23, 2024

stuartmorgan commented Feb 23, 2024

Gustl22 commented Feb 23, 2024

christopherfujino left a comment

stuartmorgan left a comment

refactor: Differentiate pubspec and resolution errors for plugins #142035

refactor: Differentiate pubspec and resolution errors for plugins #142035

Conversation

Gustl22 commented Jan 23, 2024 • edited Loading

Pre-launch Checklist

Gustl22 commented Jan 23, 2024 • edited Loading

Gustl22 commented Feb 4, 2024

christopherfujino Feb 8, 2024

Choose a reason for hiding this comment

Gustl22 Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Gustl22 Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Gustl22 commented Feb 15, 2024

Gustl22 commented Feb 15, 2024 • edited Loading

christopherfujino commented Feb 16, 2024 • edited Loading

stuartmorgan Feb 20, 2024

Choose a reason for hiding this comment

Gustl22 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Gustl22 commented Feb 23, 2024

stuartmorgan commented Feb 23, 2024

Gustl22 commented Feb 23, 2024

christopherfujino left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Gustl22 commented Jan 23, 2024 •

edited

Loading

Gustl22 commented Jan 23, 2024 •

edited

Loading

Gustl22 Feb 9, 2024 •

edited

Loading

Gustl22 Feb 9, 2024 •

edited

Loading

Gustl22 commented Feb 15, 2024 •

edited

Loading

christopherfujino commented Feb 16, 2024 •

edited

Loading

Gustl22 Feb 20, 2024 •

edited

Loading