Add option in ProxiedDevice to only transfer the delta when deploying. by chingjun · Pull Request #97462 · flutter/flutter

chingjun · 2022-01-29T00:53:59Z

...using an rsync-like algorithm. This reduces the data that needs to be
transferred, and removes the need to retransfer the assets on subsequent
calls on flutter start.

Also, the reason I'm not using rsync directly is to not introduce any external dependencies, or in case we need to do this on a Windows machine.

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.

chingjun · 2022-02-04T18:10:24Z

Friendly ping on this, PTAL, thanks :)

christopherfujino · 2022-02-04T18:35:35Z

packages/flutter_tools/lib/src/commands/daemon.dart

christopherfujino · 2022-02-04T18:39:02Z

packages/flutter_tools/lib/src/commands/daemon.dart

what's the difference here?

I've changed it to use the same temp directory every time the app runs, so that we could reuse the file that is transferred from the previous run.

ahh, makes sense.

could you leave a breadcrumb comment, lest someone revert this in a "cleanup"?

Good point about leaving a comment, just added. Thanks!

christopherfujino · 2022-02-04T18:41:15Z

packages/flutter_tools/lib/src/proxied_devices/devices.dart

should we put a debug log if _deltaFileTransfer == true && rollingHashResultJson == null?

This is normal when it's the first run. But it's probably still a good idea to add a log, just added, thanks!

oh good point about first run.

christopherfujino

LGTM

jmagman

Also, the reason I'm not using rsync directly is to not introduce any external dependencies, or in case we need to do this on a Windows machine.

Can we rsync when it is available, and fall back to this when it's not? The tool already uses it in a few places.

flutter/packages/flutter_tools/lib/src/base/os.dart

Line 443 in 610b41e

if (_processManager.canRun('rsync')) {

christopherfujino · 2022-02-17T22:11:23Z

Also, the reason I'm not using rsync directly is to not introduce any external dependencies, or in case we need to do this on a Windows machine.

Can we rsync when it is available, and fall back to this when it's not? The tool already uses it in a few places.

flutter/packages/flutter_tools/lib/src/base/os.dart

Line 443 in 610b41e

if (_processManager.canRun('rsync')) {

This is a good suggestion (we might as well use rsync if it's present on the path)

chingjun · 2022-02-17T22:32:49Z

Sorry for the late reply, missed the email from two weeks ago :(

Currently rsync is only used for more efficiently transferring files within a single machine.

Remote rsync requires different setup. We need to make sure that rsync is available on both ends and that they are compatible (rsync protocol is at version 31 now), and then (without assuming that there is SSH connection available between both ends) start an "rsync daemon" on one machine, forward the port and start an "rsync client" on the other end.

I'm mostly worried about the possible edge cases that we'll need to cover, and the security implications of doing this.

IMO if we already have to implement the algorithm in the flutter_tools, it is better to use it for all platforms to increase the chance of it being used, therefore higher chance that any bug will be found and fixed.

What do you think?

christopherfujino · 2022-02-17T22:53:24Z

Sorry for the late reply, missed the email from two weeks ago :(

Currently rsync is only used for more efficiently transferring files within a single machine.

Remote rsync requires different setup. We need to make sure that rsync is available on both ends and that they are compatible (rsync protocol is at version 31 now), and then (without assuming that there is SSH connection available between both ends) start an "rsync daemon" on one machine, forward the port and start an "rsync client" on the other end.

I'm mostly worried about the possible edge cases that we'll need to cover, and the security implications of doing this.

IMO if we already have to implement the algorithm in the flutter_tools, it is better to use it for all platforms to increase the chance of it being used, therefore higher chance that any bug will be found and fixed.

What do you think?

Ahh, good point. Makes sense to me.

chingjun · 2022-02-22T18:29:48Z

Jenn, what do you think @jmagman ?

jmagman · 2022-02-23T01:59:53Z

Jenn, what do you think @jmagman ?

Whatever you and @christopherfujino think to do here rsync or not works for me. 🙂

christopherfujino · 2022-02-23T02:10:39Z

Jenn, what do you think @jmagman ?

Whatever you and @christopherfujino think to do here rsync or not works for me. slightly_smiling_face

This PR LGTM as is; however merging is blocked by your requested changes @jmagman

jmagman

Jenn, what do you think @jmagman ?

Whatever you and @christopherfujino think to do here rsync or not works for me. slightly_smiling_face

This PR LGTM as is; however merging is blocked by your requested changes @jmagman

Ah, sorry, missed that.

Using an rsync-like algorithm. This reduces the data that needs to be transferred, and removes the need to retransfer the assets on subsequent calls on flutter start.

chingjun · 2022-02-23T19:45:10Z

Thanks!

…eploying. (flutter/flutter#97462)

flutter#97462)

chingjun requested review from christopherfujino and jmagman January 29, 2022 00:53

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 29, 2022

chingjun force-pushed the rsync branch from 371aaf4 to 92e9eca Compare January 29, 2022 03:59

christopherfujino reviewed Feb 4, 2022

View reviewed changes

christopherfujino approved these changes Feb 4, 2022

View reviewed changes

jmagman requested changes Feb 4, 2022

View reviewed changes

jmagman approved these changes Feb 23, 2022

View reviewed changes

chingjun added 3 commits February 23, 2022 10:48

Add option in ProxiedDevice to only transfer the delta when deploying.

b93dcf3

Using an rsync-like algorithm. This reduces the data that needs to be transferred, and removes the need to retransfer the assets on subsequent calls on flutter start.

Review feedback

5d8c2e7

Add comment about not cleaning up tempDirectory

1ffdbfd

chingjun force-pushed the rsync branch from 59a982c to 1ffdbfd Compare February 23, 2022 18:49

chingjun added the waiting for tree to go green label Feb 23, 2022

fluttergithubbot merged commit 6a9ac45 into flutter:master Feb 23, 2022

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to 6a9ac4509b77 (1 revision) flutter/packages#951

Closed

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

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

0a9fecf

…eploying. (flutter/flutter#97462)

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to 47b5ed3948e8 (2 revisions) flutter/packages#952

Closed

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

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

a9f3a5e

…eploying. (flutter/flutter#97462)

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to 47b5ed3948e8 (2 revisions) flutter/plugins#4914

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 23, 2022

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

095204b

…eploying. (flutter/flutter#97462)

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to c178cf859f25 (3 revisions) flutter/packages#953

Closed

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

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

27211c2

…eploying. (flutter/flutter#97462)

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to 9e7cc970eafc (4 revisions) flutter/packages#954

Closed

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

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

53290e6

…eploying. (flutter/flutter#97462)

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to 2ad0ac0ef227 (5 revisions) flutter/packages#955

Closed

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

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

54e28e7

…eploying. (flutter/flutter#97462)

engine-flutter-autoroll mentioned this pull request Feb 23, 2022

Roll Flutter from c74a646b7bba to 01893b6cea4c (6 revisions) flutter/packages#956

Merged

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

6a9ac45 Add option in ProxiedDevice to only transfer the delta when d…

2cc6ffa

…eploying. (flutter/flutter#97462)

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022

Add option in ProxiedDevice to only transfer the delta when deploying. (

ee45ea7

flutter#97462)

Conversation

chingjun commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chingjun commented Feb 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Feb 17, 2022

Uh oh!

chingjun commented Feb 17, 2022

Uh oh!

christopherfujino commented Feb 17, 2022

Uh oh!

chingjun commented Feb 22, 2022

Uh oh!

jmagman commented Feb 23, 2022

Uh oh!

christopherfujino commented Feb 23, 2022

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

chingjun commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chingjun commented Jan 29, 2022 •

edited

Loading

jmagman left a comment •

edited

Loading