Add `.flutter-plugins-dependencies` to the project, which contains the app's plugin dependency graph by blasten · Pull Request #45379 · flutter/flutter

blasten · 2019-11-22T03:06:24Z

Description

Creates a .flutter-plugins-dependencies which contains the plugin dependency graph from pub. Then, Gradle reads this file to reconstruct the dependencies.

Related Issues

Fixes #45188

Tests

I added the following tests: unit test

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

blasten · 2019-11-22T03:07:27Z

@tvolkert This is the last thing we need to complete the plugin migration to the new embedding.

xster

Implementation LGTM % tests

xster · 2019-11-22T03:33:01Z

Ultra-nit, // instead of ///

It's not a full dart doc since it only partially describes this function (its return value). If ever this becomes a public method, it's up to that person to write a proper dart doc

/// makes the comment show up in the IDE.

jonahwilliams · 2019-11-22T04:10:19Z

Can we put this somewhere other than the project root?

blasten · 2019-11-22T04:27:40Z

@jonahwilliams this is intended to be used by all platforms. However, this PR only implements the functionality for Android.

xster · 2019-11-22T06:33:31Z

@jonahwilliams It represents a conceptual inter-dependency between plugins for all platforms. If we intend to have a different pub dependency per platform, then it might make sense to have multiple such files.

jonahwilliams · 2019-11-22T17:26:26Z

@blasten and I spoke offline. It may need to be moved when we do platform specific plugins but I'm okay with it being here for now

xster · 2019-11-22T18:39:54Z

tests LGTM

jonahwilliams

Does this handle cyclic dependencies? For example, plugin a and b depend on each other.

blasten · 2019-11-22T19:21:28Z

@jonahwilliams Here's the output (when there's a cycle):

$ flutter build apk --target-platform android-arm 
                                                                        
FAILURE: Build failed with an exception.                                
                                                                        
* What went wrong:                                                      
Circular dependency between the following tasks:                        
:camera:generateReleaseRFile                                            
+--- :camera:generateReleaseRFile (*)                                   
\--- :path_provider:generateReleaseRFile                                
     +--- :camera:generateReleaseRFile (*)                              
     \--- :path_provider:generateReleaseRFile (*)                       
                                                                        
(*) - details omitted (listed previously)                               
                                                                        
                                                                        
* Try:                                                                  
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
                                                                        
* Get more help at https://help.gradle.org                              
                                                                        
BUILD FAILED in 828ms                                                   
Running Gradle task 'assembleRelease'...                                
Running Gradle task 'assembleRelease'... Done                       1.4s
Gradle task assembleRelease failed with exit code 1

jonahwilliams · 2019-11-22T19:34:35Z

    return null;
  }
  final String packageRootPath = fs.path.fromUri(packageRoot);
+  final YamlMap dependencies = pubspec['dependencies'];


This will capture direct dependencies, but not transitive dependencies - is this an issue?. Consider:

plugin A depends on package 1
package 1 depends on plugin B

.flutter-plugins will contain plugin A/plugin B while the dependencies file will not link them.

From offline discussion, this isn't a supported use case - and that platform code cannot depend on eachother. Therefore it doesn't seem like the gradle task ordering needs to take this into consideration

Co-Authored-By: Jonah Williams <[email protected]>

jonahwilliams

LGTM

zanderso

Is there an integration test for this using a plugin that needs this feature?

zanderso

lgtm w/ question

matthew-carroll · 2019-11-22T22:24:33Z

@blasten @xster I ran my maps plugin migration branch against this version of the framework and it appears to correctly link the Android code. I no longer need to use reflection. Good job :)

zanderso · 2019-11-25T17:01:27Z

@blasten @xster I ran my maps plugin migration branch against this version of the framework and it appears to correctly link the Android code. I no longer need to use reflection. Good job :)

Is it possible to extract an integration test from something in there?

blasten · 2019-11-25T17:05:19Z

@zanderso definitely. Once, the Google maps plugin is published, I can add an integration test. I will file an issue.

matthew-carroll · 2019-11-27T23:17:35Z

FYI - I won't be publishing the migration because I was just attempting to prove that Lifecycle works with plugins. So we'll be waiting on the ecosystem team to do the public migration.

fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Nov 22, 2019

googlebot added the cla: yes label Nov 22, 2019

xster reviewed Nov 22, 2019

View reviewed changes

blasten added the work in progress; do not review label Nov 22, 2019

Emmanuel Garcia added 3 commits November 22, 2019 09:02

Support plugin platform dependencies on Android

a0beeb0

typo

9d56f40

tweaks

4ad2762

blasten force-pushed the plugin_platform_dependencies branch from 9be0ea5 to 4ad2762 Compare November 22, 2019 17:22

Emmanuel Garcia added 2 commits November 22, 2019 10:16

rename property to flutterPluginsDependenciesFile

0e4e372

unit test

44f9aee

blasten force-pushed the plugin_platform_dependencies branch from f8e4bb2 to 44f9aee Compare November 22, 2019 18:18

blasten removed the work in progress; do not review label Nov 22, 2019

blasten requested review from jonahwilliams and zanderso November 22, 2019 18:19

blasten changed the title ~~Support plugin platform dependencies on Android~~ Add .flutter-plugins-dependencies to the project, which contains the app's plugin dependency graph Nov 22, 2019

jonahwilliams reviewed Nov 22, 2019

View reviewed changes

blasten requested a review from jonahwilliams November 22, 2019 19:23

jonahwilliams reviewed Nov 22, 2019

View reviewed changes

Comment thread packages/flutter_tools/lib/src/plugins.dart Outdated

jonahwilliams reviewed Nov 22, 2019

View reviewed changes

Update packages/flutter_tools/lib/src/plugins.dart

432d449

Co-Authored-By: Jonah Williams <[email protected]>

jonahwilliams approved these changes Nov 22, 2019

View reviewed changes

Comment

233c68e

zanderso reviewed Nov 22, 2019

View reviewed changes

Comment thread packages/flutter_tools/gradle/flutter.gradle

Comment thread packages/flutter_tools/test/general.shard/plugins_test.dart Outdated

Emmanuel Garcia added 2 commits November 22, 2019 12:53

Fix build

332c185

Use JSON instead of properties file

ccd5d68

blasten force-pushed the plugin_platform_dependencies branch from f585bae to ccd5d68 Compare November 22, 2019 21:37

blasten requested a review from zanderso November 22, 2019 21:38

zanderso approved these changes Nov 22, 2019

View reviewed changes

Comment thread packages/flutter_tools/gradle/flutter.gradle

Feedback

1fe1d7e

blasten added the waiting for tree to go green label Nov 22, 2019

blasten merged commit b6e9200 into flutter:master Nov 22, 2019

blasten deleted the plugin_platform_dependencies branch November 22, 2019 23:02

blasten mentioned this pull request Nov 25, 2019

Add integration test for transitive plugin dependencies #45579

Merged

8 of 9 tasks

amirh mentioned this pull request Dec 7, 2019

android/ folder is required for non-android plugins #46304

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Conversation

blasten commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

blasten commented Nov 22, 2019

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xster Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

blasten Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 22, 2019

Uh oh!

blasten commented Nov 22, 2019

Uh oh!

xster commented Nov 22, 2019

Uh oh!

jonahwilliams commented Nov 22, 2019

Uh oh!

xster commented Nov 22, 2019

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

blasten commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jonahwilliams Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthew-carroll commented Nov 22, 2019

Uh oh!

zanderso commented Nov 25, 2019

Uh oh!

blasten commented Nov 25, 2019

Uh oh!

matthew-carroll commented Nov 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

blasten commented Nov 22, 2019 •

edited

Loading

blasten commented Nov 22, 2019 •

edited

Loading