Add an option to use a prebuilt Dart SDK#26931
Conversation
flutter-dashboard
bot
added
the
platform-web
zanderso
force-pushed
the
prebuilt-dart-sdk
branch
from
3d7ba77 to
c5e44b0
Compare
zanderso
force-pushed
the
prebuilt-dart-sdk
branch
8 times, most recently
from
b080f27 to
555964c
Compare
…ctory This is needed for the Flutter Engine to use a prebuilt Dart SDK as in flutter/engine#26931. When using a prebuilt Dart SDK, the rules producing the built SDK are directed to a different location so that GN doesn't warn about multiple rules that produce the same file. TEST=locally with a flutter/engine checkout Change-Id: I4095f379c772c63f93a1c88c7f4e44804f3a6fc7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204760 Reviewed-by: Alexander Aprelev <aam@google.com> Commit-Queue: Zach Anderson <zra@google.com>
zanderso
force-pushed
the
prebuilt-dart-sdk
branch
3 times, most recently
from
83027b1 to
c387624
Compare
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| { | ||
| 'name': 'Download prebuilt Dart SDK', |
There was a problem hiding this comment.
Should we guard this with some kind of variable? Will downloading this always be necessary - in particular, will there be any builders that still use --full-dart-sdk and so should skip this?
There was a problem hiding this comment.
Good idea. The API in the recipes appears to use env vars, so I added a guard on one in the python script. Next step is to enable in recipes and see how much things get better there.
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
I am a bit nervous about missing something that the CIPD hook already does for us (curl on host, hash checks, fixing permissions, etc) besides just having to maintain another script. But, I couldn't spot anything wrong with this approach till the artifacts are published on CIPD. Is this something that is being tracked?
| "//third_party/dart/utils/dartdevc:dartdevc_files_stamp", | ||
| "//third_party/dart/utils/dartdevc:dartdevc_sdk_patch_stamp", | ||
| ] | ||
| template("_dartdevc") { |
There was a problem hiding this comment.
I don't know what these templates are supposed to do and neither are their arguments documented. It's possible to infer both after reading the implementation but a short docstring might be nice.
| # Downloads a Dart SDK to //flutter/prebuilts. | ||
| def DownloadDartSDK(channel, version, os_name, arch): | ||
| file = 'dartsdk-{}-{}-release.zip'.format(os_name, arch) | ||
| url = 'https://storage.googleapis.com/dart-archive/channels/{}/raw/{}/sdk/{}'.format( |
There was a problem hiding this comment.
This seems to be recreating the functionality of CIPD hooks in the DEPS file. Can we move these artifacts to CIPD instead? Stuff like needing curl on the host and not performing hash consistency checks may lead to unnecessary hassles on CI.
There was a problem hiding this comment.
Yes. Sorry for not being more explicit about the plan. I want to see how much this can improve build times on CI. If there's a reasonably good improvement, then I'll open a thread with Dart EngProd to request that these artifacts are uploaded to cipd, and this script will go away.
If the improvement on CI is not significant, then I will back all this out.
| # Download and extract variants in parallel | ||
| pool = multiprocessing.Pool() | ||
| tasks = [(channel, semantic_version, os_name, arch) for arch in architectures] | ||
| async_results = [pool.apply_async(DownloadAndExtract, t) for t in tasks] |
There was a problem hiding this comment.
Don't thank me, thank stackoverflow!
zanderso
force-pushed
the
prebuilt-dart-sdk
branch
from
c387624 to
51061c6
Compare
3 participants
This PR downloads prebuilt Dart SDKs into a new
prebuilts/directory that match the version of the Dart SDK source that is fetched through the DEPS file.During a build, these prebuilts are copied to e.g.
out/host_debug_unopt/dart-sdkinstead of building them. This is faster than building the snapshots for dart2js, etc..This patch needs https://dart-review.googlesource.com/c/sdk/+/204760.