Skip to content

flutter tools hook support#35803

Closed
kangwang1988 wants to merge 4 commits intoflutter:masterfrom
XianyuTech:alf_master_flutter_tools_hook_support
Closed

flutter tools hook support#35803
kangwang1988 wants to merge 4 commits intoflutter:masterfrom
XianyuTech:alf_master_flutter_tools_hook_support

Conversation

@kangwang1988
Copy link
Contributor

@kangwang1988 kangwang1988 commented Jul 9, 2019

This pr enable developers to add some customized hooks point into flutter tools. So that they can do some things before/after a flutter command, by specify a hooks.yaml under the root directory of their flutter project without modifying the flutter_tools.

A typical hooks.yaml is given below:

# Use the analysis options settings from the top level of the repo (not
# the ones from above, which include the `public_member_api_docs` rule).

commands:
  doctor:
    hook:
      before: 
        executable: "perl"
        argument: "flutter_tools_hook.pl"
      after: 
        executable: "/bin/sh"
        argument: "flutter_tools_hook.sh"
  build:
    bundle:
      hook:
        after: 
          executable: "python"
          argument: "flutter_tools_hook_bundle.py"
    ios:
      hook:
        before: 
          executable: "python"
          argument: "flutter_tools_hook.py"
        after: 
          executable: "ruby"
          argument: "flutter_tools_hook.rb"

For example, 'python flutter_tools_hook_bundle.py "build bundle --target-platform=ios --target=lib/main.dart --release --depfile=build/snapshot_blob.bin.d --asset-dir=/Users/kylewong/Codes/Flutter/alibaba-flutter/flutter/examples/hello_world/ios/Flutter/App.framework/flutter_assets --precompiled" will be executed when flutter build bundle command is executed.

Tests

I added the following tests:
packages/flutter_tools/test/hooks_test.dart
It includes test logic for hooks.yaml parser, hooks execution logic.

Breaking Change

  • No, this is not a breaking change.

@kangwang1988 kangwang1988 changed the title Alf master flutter tools hook support flutter tools hook support Jul 9, 2019
@kangwang1988 kangwang1988 requested a review from gspencergoog July 9, 2019 13:23
@codecov
Copy link

codecov bot commented Jul 9, 2019 •
edited
Loading

Codecov Report

Merging #35803 into master will increase coverage by 0.39%.
The diff coverage is 80.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35803      +/-   ##
==========================================
+ Coverage   54.86%   55.26%   +0.39%     
==========================================
  Files         188      189       +1     
  Lines       17320    17376      +56     
==========================================
+ Hits         9502     9602     +100     
+ Misses       7818     7774      -44
Flag Coverage Δ
#flutter_tool 55.26% <80.35%> (+0.39%) ⬆️
Impacted Files Coverage Δ
...r_tools/lib/src/runner/flutter_command_runner.dart 68.42% <35.71%> (-0.25%) ⬇️
packages/flutter_tools/lib/src/hooks.dart 95.23% <95.23%> (ø)
packages/flutter_tools/lib/src/web/workflow.dart 50% <0%> (-27.78%) ⬇️
...ckages/flutter_tools/lib/src/flutter_manifest.dart 78.94% <0%> (-3.69%) ⬇️
packages/flutter_tools/lib/src/base/io.dart 67.64% <0%> (-2.95%) ⬇️
...ges/flutter_tools/lib/src/android/android_sdk.dart 77.09% <0%> (-2.3%) ⬇️
.../flutter_tools/lib/src/fuchsia/fuchsia_device.dart 55.39% <0%> (-0.38%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 38.81% <0%> (-0.32%) ⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart 82.66% <0%> (+0.44%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91c43c9...0cbf499. Read the comment docs.

@goderbauer goderbauer added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 9, 2019
jonahwilliams
jonahwilliams suggested changes Jul 10, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to implemented within the flutter tool? It seems like a lot of complexity to support something that could ultimately be accomplished with a bash/batch script that wrapped the commands.

@kangwang1988
Copy link
Contributor Author

kangwang1988 commented Jul 10, 2019

@jonahwilliams
This pr originally comes from a case that we want to do some dill transformation after it's generated by "flutter build bundle" command.
flutter build bundle command is often wrapped in flutter build apk, flutter build ios,etc, if we want to wrap it using a shell or some scripts, we need to modify the xcode_backend.sh or flutter.gradle, this kind of injection would make things more complex.
Besides, now, different from gradle/cocoapods, flutter_tools don't offer hooks so that developers can do this kind of stuff before or after a flutter command. A natively supported hook mechanism would give developers freedom to do their own job without injecting the flutter_tools.

@kangwang1988
Copy link
Contributor Author

kangwang1988 commented Jul 10, 2019 •
edited
Loading

I developed a aop framework aspectd available at https://github.com/alibaba-flutter/aspectd
Its core logic is to do dill transformation after a dill file is generated.
However, I want the dill transformation is called automatically if aop annotation is enabled, for a developer who want to use the aop, they can still use flutter build ios flutter build apk, flutter run --debug flutter run --release,or run from Android project(flutter.gradle call) or run from iOS project(xcode_backend.sh call),without changing their way to use the flutter command.
Aspectd has to modify the flutter_tools, so that the dill transformation logic is executed after a dill file is generated, which would request more inner knowledge about the flutter_tools and made the injection logic vulnerable to changes.
Aspectd tries to solve the aop problem without invasion, however, the logic of flutter_tools still made it necessary to do invasion to flutter build process.

In this pr, i not only want to solve the aspectd invasion problem, but also provides a more general way to give developers chance to do some job they want.

@kangwang1988
Copy link
Contributor Author

kangwang1988 commented Jul 10, 2019

image

We also have hook logic as above.
Which brings some differences from the github flutter repo and makes it hard to maintain.

@kangwang1988
Copy link
Contributor Author

kangwang1988 commented Jul 17, 2019

@gspencergoog
What do you think is the best way to do provide hook support when using flutter command?

@kangwang1988
Copy link
Contributor Author

kangwang1988 commented Jul 17, 2019

@jason-simmons
Different from gradle, flutter_tools provides no ways to do the hook related job when needed, that's what I'm trying to solve.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jul 18, 2019

@kangwang1988 It looks a bit like you're using the aspectd package to plugin into the CFE. It would be great to start with a detailed issue describing a bit more how this works and what sort of transformations you are making. I think the CFE might have some APIs to do this sort of thing. We could cc the dart team and get a discussion going

@codecov
Copy link

codecov bot commented Jul 22, 2019 •
edited
Loading

Codecov Report

Merging #35803 into master will increase coverage by 0.39%.
The diff coverage is 80.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35803      +/-   ##
==========================================
+ Coverage   54.86%   55.26%   +0.39%     
==========================================
  Files         188      189       +1     
  Lines       17320    17376      +56     
==========================================
+ Hits         9502     9602     +100     
+ Misses       7818     7774      -44
Flag Coverage Δ
#flutter_tool 55.26% <80.35%> (+0.39%) ⬆️
Impacted Files Coverage Δ
...r_tools/lib/src/runner/flutter_command_runner.dart 68.42% <35.71%> (-0.25%) ⬇️
packages/flutter_tools/lib/src/hooks.dart 95.23% <95.23%> (ø)
packages/flutter_tools/lib/src/web/workflow.dart 50% <0%> (-27.78%) ⬇️
...ckages/flutter_tools/lib/src/flutter_manifest.dart 78.94% <0%> (-3.69%) ⬇️
packages/flutter_tools/lib/src/base/io.dart 67.64% <0%> (-2.95%) ⬇️
...ges/flutter_tools/lib/src/android/android_sdk.dart 77.09% <0%> (-2.3%) ⬇️
.../flutter_tools/lib/src/fuchsia/fuchsia_device.dart 55.39% <0%> (-0.38%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 38.81% <0%> (-0.32%) ⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart 82.66% <0%> (+0.44%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91c43c9...0cbf499. Read the comment docs.

@kangwang1988 kangwang1988 mentioned this pull request Jul 23, 2019
Closed
@gspencergoog gspencergoog removed their request for review August 5, 2019 23:26
@christopherfujino christopherfujino added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Aug 15, 2019
@kangwang1988
Copy link
Contributor Author

kangwang1988 commented Sep 19, 2019

Close this issue as I think we are on the right way to have a better solution to resolve my problem, see:#36738 for more.

@kangwang1988 kangwang1988 deleted the alf_master_flutter_tools_hook_support branch October 14, 2019 13:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels. waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kangwang1988 @jonahwilliams @goderbauer @googlebot @christopherfujino