Skip to content

Use already downloaded dart sdk for frontend_server. - #14702

Merged
aam merged 5 commits into
flutter:masterfrom
aam:stop-downloading-engine-dart-sdk
Feb 15, 2018
Merged

Use already downloaded dart sdk for frontend_server.#14702
aam merged 5 commits into
flutter:masterfrom
aam:stop-downloading-engine-dart-sdk

Conversation

@aam

@aam aam commented Feb 14, 2018

Copy link
Copy Markdown
Member

This is follow-up to #14610

Fixes #14700

@aam

aam commented Feb 14, 2018

Copy link
Copy Markdown
Member Author

cc @tvolkert

tvolkert
tvolkert reviewed Feb 14, 2018
View reviewed changes

@tvolkert tvolkert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does flutter precache -a still download the SDK for all platforms? We need that to be able to roll Flutter into Google.

@aam

aam commented Feb 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Thanks for the pointer, @tvolkert . It did, cleaned it up.
Note I changed flutter doctor -v reporting so it reports single Dart version now:

╰─➤  bin/flutter doctor -v   
...
• Dart version 2.0.0-edge.3c4dccbd46f152be9e1b6ca95c57357e8e48057c

@tvolkert

tvolkert commented Feb 14, 2018

Copy link
Copy Markdown
Contributor

We need to keep the platform-specific locations to support flutter precache -a, which downloads artifacts for all platforms (not just the current platform). So I think we should probably go the other way - get rid of bin/cache/dart-sdk in favor of always using the platform-specific location.

@aam

aam commented Feb 14, 2018

Copy link
Copy Markdown
Member Author

Got you, sorry I misunderstood what flutter precache -a does.
One issue with using bin/cache/artifacts/engine/<linux-x64|darwin-x64|windows-x64>, which is platform dependent is that packages/flutter_tools/pubspec.yaml points to dart sdk to override dependency on analyzer, front_end and kernel libraries https://github.com/flutter/flutter/blob/master/packages/flutter_tools/pubspec.yaml#L90.

@tvolkert

tvolkert commented Feb 14, 2018

Copy link
Copy Markdown
Contributor

Hmm. symlinks come to mind, but Windows is tricky...

/cc @goderbauer

@aam

aam commented Feb 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

symlinks on Windows seem to require admin elevated privileges, so that leaves copying as the only option for Windows.

@aam

aam commented Feb 14, 2018

Copy link
Copy Markdown
Member Author

Another issue with running flutter from bin/cache/artifacts/engine/ however is that artifacts-handling code expects to own that location, so when updating, for example, it deletes everything first(https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/cache.dart#L253), then downloads. That won't work(as Windows, for example, will tell you that directory is in use) when you have flutter running with dart from that artifacts/engine location.

@aam
WIP to switch back to solution where dart sdk is downloaded into bin/…
3a68dd9 
…cache/dart-sdk, but it is 'flutter precache -a' that can download all dart sdks for other platforms under bin/cache/artifacts/engine/<platform>/dart-sdk
@aam
aam force-pushed the stop-downloading-engine-dart-sdk branch from 825ad42 to 3a68dd9 Compare February 14, 2018 23:38
@aam

aam commented Feb 15, 2018

Copy link
Copy Markdown
Member Author

a894c9e makes flutter download dart sdk for the host into bin/cache/dart-sdk, while only flutter precache -a downloads all dart sdks into bin/cache/artifacts/engine/<platform>/dart-sdk.

tvolkert
tvolkert approved these changes Feb 15, 2018
View reviewed changes

@tvolkert tvolkert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@aam
aam merged commit c0c6c60 into flutter:master Feb 15, 2018
@aam
aam deleted the stop-downloading-engine-dart-sdk branch February 15, 2018 15:29
tvolkert added a commit that referenced this pull request Feb 15, 2018
@tvolkert
Revert "Use already downloaded dart sdk for frontend_server. (#14702)"
65029c7 
This reverts commit c0c6c60.
@aam

aam commented Feb 16, 2018

Copy link
Copy Markdown
Member Author

This broke use of --local-engine with --preview-dart-2 as frontend_server.dart.snapshot produced by local engine's dart sdk is not compatible with cached dart sdk bin/cache/dart-sdk.

cc @alexmarkov

aam added a commit to aam/flutter that referenced this pull request Feb 16, 2018
@aam
Reintroduce engineDartSdkPath artifact.
6759497 
This is needed to support running with local engine's dart sdk.

This is follow-up to flutter#14702.
@aam aam mentioned this pull request Feb 16, 2018
Merged
aam added a commit that referenced this pull request Feb 16, 2018
@aam
Reintroduce engineDartSdkPath artifact. (#14742)
fe00598 
This is needed to support running with local engine's dart sdk.

This is follow-up to #14702.
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
@aam
Use already downloaded dart sdk for frontend_server. (flutter#14702)
52d2825 
* Use already downloaded dart sdk for frontend_server.

This is follow-up to flutter#14610

* Remove reference to engine dart-sdk from artifacts

* Remove unused imports

* WIP to switch back to solution where dart sdk is downloaded into bin/cache/dart-sdk, but it is 'flutter precache -a' that can download all dart sdks for other platforms under bin/cache/artifacts/engine/<platform>/dart-sdk

* Download all dart sdks when 'flutter precache -a'
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
@aam
Reintroduce engineDartSdkPath artifact. (flutter#14742)
374d674 
This is needed to support running with local engine's dart sdk.

This is follow-up to flutter#14702.
@timsneath timsneath mentioned this pull request Nov 4, 2019
Closed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@tvolkert tvolkert tvolkert approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flutter tool downloading Dart SDK twice

3 participants

@aam @tvolkert @googlebot