Create a package-able incremental compiler #12681
Conversation
This comment has been minimized.
This comment has been minimized.
jonahwilliams
changed the title
frontend_server/publish.dart
Outdated
| meta: any | ||
| '''; | ||
|
|
||
| const String frontendServerPubspec = r''' |
There was a problem hiding this comment.
What is the reason to have pubspec.yaml contents hardcoded in this script?
There was a problem hiding this comment.
The pubspec of vm/build_integration are not valid, while the pubspec of front_end and kernel say not to rely on the versions. To ensure I don't have version solving issues, I've replace the pubspecs here with any constraints. This will be used by a generated pubspec in the flutter_tool that uses the dependency_overrides section to specify path versions.
There was a problem hiding this comment.
Do you know why those packages have invalid or reuse-discouraging pubspec.yamls? Is that to prevent deploying and distributing those packages for some reason? If we intend to have those packages to be distributed and available, wouldn't be cleaner to fix their pubspec.yamls instead of providing our replacements for those?
There was a problem hiding this comment.
I'm not really distributing or publishing them though, I'm just allowing them to be vendored by the flutter SDK. The versions/constraints are not really meaningful in that content, as they won't be user visible.
There was a problem hiding this comment.
Sorry what does "vendored by the flutter SDK" mean? Does this effectively gives flutter tools (and only flutter tools, because nobody else downloads flutter engine artifacts) access to those packages, while currently those packages(well, frontend server only) is only available as executable snapshot?
There was a problem hiding this comment.
Yes, the would be in the cache artifacts where flutter_tools can see the source, and use them to regenerate the snapshot to include other kernel transforms per flutter/flutter#36738
| @@ -1,19 +0,0 @@ | |||
| import 'dart:async'; | |||
There was a problem hiding this comment.
Why is this test being removed?
There was a problem hiding this comment.
It didn't seem like it had been running.
There was a problem hiding this comment.
Does it pass? Would it be valuable to have running somewhere? Maybe @aam knows.
There was a problem hiding this comment.
It's not high-value test, other build-time things will break if running with --train fails somehow. Also there used to be more tests as part of this, but they were migrated over to dart sdk. I would keep it, but no strong feelings if it is deleted.
| @@ -22,36 +26,3 @@ dependencies: | |||
| typed_data: any | |||
| usage: any | |||
| vm: any | |||
|
|
|||
| dev_dependencies: | |||
There was a problem hiding this comment.
Why are these not needed anymore?
frontend_server/BUILD.gn
Outdated
| @@ -71,4 +74,115 @@ if (is_fuchsia_host || is_fuchsia) { | |||
|
|
|||
| inputs = frontend_server_files | |||
| } | |||
|
|
|||
| # Creates a packageable vm. | |||
| prebuilt_dart_action("flutter_package_vm") { | |||
There was a problem hiding this comment.
As discussed offline, add comments about these artifacts pointing at the github issue and add a reviewer from the engine team.
frontend_server/BUILD.gn
Outdated
| # we can generate a local frontend_server snapshot in the tools cache. | ||
|
|
||
| # Creates a packageable vm. | ||
| prebuilt_dart_action("flutter_package_vm") { |
There was a problem hiding this comment.
I understand the desire to use Dart in publish.dart but this template invocation now uses the Python to list the files, Dart to publish the same, and compiles the host VM (~2000 TUs) to boot. We do have a prebuilt VM you can use for complicated Dart scripts but I'd rather you just replace these steps with a single script action. That will be faster in cases where this copy step is the only step necessary (since you wont have to build the VM) and can be parallelized by the build system when other targets that need the VM are also being built.
There was a problem hiding this comment.
Understood, I'll update the script to use python.
There was a problem hiding this comment.
FYI prebuilt_dart_action() should be using the prebuilt Dart VM. If it isn't then that is a bug.
There was a problem hiding this comment.
You're right. I was thinking of dart_action. I suppose I am now significantly less concerned about this but I still think it'd be cleaner and less verbose to just have a single action target that achieves this instead of a target definition that mixes both python and dart with a custom template.
There was a problem hiding this comment.
What would be the resolution here - to write this as a single target in python? dart?
There was a problem hiding this comment.
@chinmaygarde Would a single python script wrapped in a GN template scoped to this file address your concerns?
There was a problem hiding this comment.
Yes. A single python script would be great. Thanks.
There was a problem hiding this comment.
Updated to use a single/action python script.
There was a problem hiding this comment.
Updated to use a single python script
|
|
||
| import argparse | ||
| import os | ||
| import sys |
| @@ -0,0 +1,114 @@ | |||
| #!/usr/bin/env python | |||
| # Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file | |||
| meta: any | ||
| ''' | ||
|
|
||
| PUBSPECS = { |
There was a problem hiding this comment.
Does the engine repo use the google3 style (4 space indent) or the chrome style (2 space indent) for python? @chinmaygarde
There was a problem hiding this comment.
I switched to 2-space, that looks like the rest of the repo
| if not os.path.isdir(os.path.join(args.output, package)): | ||
| os.makedirs(os.path.join(args.output, package)) | ||
|
|
||
| if '.git' in directories: |
There was a problem hiding this comment.
Shouldn't this have been skipped already by the logic above?
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| sys.exit(main()) |
| @@ -0,0 +1,110 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Ensure this file has the execute bit set.
|
@chinmaygarde was there anything else you'd like me to update for this PR? |
[email protected]:flutter/engine.git/compare/7d67e275ff82...8aa4732 git log 7d67e27..8aa4732 --no-merges --oneline 2019-10-03 [email protected] Fix Metal builds. (flutter/engine#12777) 2019-10-03 [email protected] Revert "Manage resource and onscreen contexts using separate IOSGLContext objects (#12277)" (flutter/engine#12773) 2019-10-03 [email protected] Roll src/third_party/dart afac6a3714..07a63a17a4 (6 commits) 2019-10-03 [email protected] roll buildroot to 01e9235 (flutter/engine#12771) 2019-10-03 [email protected] Create a package-able incremental compiler (flutter/engine#12681) 2019-10-03 [email protected] add windows embedding test (flutter/engine#12423) 2019-10-03 [email protected] Roll fuchsia/sdk/core/mac-amd64 from g-PD1... to wYLiQ... (flutter/engine#12770) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
[email protected]:flutter/engine.git/compare/7d67e275ff82...8aa4732 git log 7d67e27..8aa4732 --no-merges --oneline 2019-10-03 [email protected] Fix Metal builds. (flutter/engine#12777) 2019-10-03 [email protected] Revert "Manage resource and onscreen contexts using separate IOSGLContext objects (flutter#12277)" (flutter/engine#12773) 2019-10-03 [email protected] Roll src/third_party/dart afac6a3714..07a63a17a4 (6 commits) 2019-10-03 [email protected] roll buildroot to 01e9235 (flutter/engine#12771) 2019-10-03 [email protected] Create a package-able incremental compiler (flutter/engine#12681) 2019-10-03 [email protected] add windows embedding test (flutter/engine#12423) 2019-10-03 [email protected] Roll fuchsia/sdk/core/mac-amd64 from g-PD1... to wYLiQ... (flutter/engine#12770) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Description
Packages up
package:vm,package:build_integration,package:kernel,package:front_end, andpackage:frontend_serverfor downloading by the flutter tool cache. This copies only thelib/directory and replaces the pubspec, since several of these packages have invalid or outdated constraints.The incremental compiler can optionally take a single ProgramTransformer which will be applied to the Component after compilation, which will be used for kernel transforms.
Work towards flutter/flutter#36738