Build Web SDK in wasm build tree #36520

eyebrowsoffire · 2022-09-30T04:14:15Z

This makes some improvements to the web build process.

felt test no longer requires any build folder at all, unless --use-local-canvaskit is specified. To compile tests, it now uses whatever dart sdk that felt.dart is invoked with (which is by default the prebuilt dart sdk)
felt build only invokes the --wasm --runtime-mode=release of the gn build, and that build now produces the flutter_web_sdk and the .zip artifact. This build also no longer relies on building or copying the dart sdk to the output folder, it just uses the prebuilt sdk where it is.
We don't produce a random unused xcodeproj in the wasm build anymore

We need to do some followup recipe changes to take full advantage of this, which I've outlined here: flutter/flutter#113303

The ultimate goal is going to be:

The engine build produces a single flutter-web-sdk.zip artifact that rolls into the framework, and no longer has platform specific artifacts.
The flutter tool consumes the flutter-web-sdk.zip artifact instead of the platform specific artifacts
The host build no longer builds the flutter_web_sdk targets at all
We don't do host builds at all to do any web engine CI stuff.

flutter-dashboard · 2022-09-30T04:14:19Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

mdebbar · 2022-10-08T18:00:31Z

lib/web_ui/dev/build.dart

-    }
+    print('Running gn...');
+    const List<String> gnArgs = <String>[
+      '--wasm',


Now that this target is going to be used for all of flutter web (not just for wasm), does it make sense to change the name to flutter_web or similar?

I think there was some extensive discussion about this with the rest of the engine team, I'd like to ask @hterkelsen what their concerns were and why they landed on this. In any case, the --wasm can't be immediately deleted because the recipes use it, but if we are going to migrate to some different flag I should at least add it as an option here so the recipes can switch to it.

My concern with using --wasm to mean "build the Flutter Web SDK" is that the Flutter Web SDK bits aren't WASM (for now, anyway). The fact that we can invoke kernel compiler when using the WASM toolchain seems to just be a lucky coincidence. I think we should have a --web flag in GN which sets the default CPU to WASM and builds CanvasKit and the Flutter Web SDK.

That makes sense. I think I get what you're going for. Let me make some changes and see what I can do.

…sed.

…he web sdk artifact.

zanderso

DBC

zanderso · 2022-10-14T23:43:00Z

dart_sdk/BUILD.gn

@@ -0,0 +1,24 @@
+import("//flutter/common/config.gni")


This file needs a copyright header

zanderso

More DBC

I'm skeptical that creating the dart_sdk subdirectory that contains only the BUILD.gn file is needed. What's the reasoning behind that?

zanderso · 2022-10-17T15:28:48Z

BUILD.gn

@@ -12,6 +12,8 @@ import("//third_party/dart/build/dart/copy_tree.gni")

 # Whether to build the dartdevc sdk, libraries, and source files
 # required for the flutter web sdk.
+# TODO(jacksongardner): remove this poorly named argument once the recipes stop
+# using it.


Please link to a github issue for all TODOs.

zanderso · 2022-10-17T15:29:31Z

dart_sdk/BUILD.gn

@@ -0,0 +1,24 @@
+import("//flutter/common/config.gni")


Two things:

What is the reason for moving this?

GN files need a copyright header.

The original reason for moving this was that the flutter/BUILD.gn file is not wasm friendly (it transitively pulls in a bunch of .gni files that have platform checks that choke when the target os/arch is wasm). Since I was trying to build the web sdk in the wasm build, it was pulling in the flutter/BUILD.gn file when it had a dependency on the dart-sdk target, so I separated it out and simplified it so that we wouldn't have to pull in the whole of flutter/BUILD.gn when building the web sdk.

After some additional changes though, the web sdk build now uses the prebuilt sdk in-place and so does not have a dependency on the dart_sdk target anymore, so I can actually move it back if this bothers you.

We want to allow Dart SDK hackers to test Dart SDK builds with local source changes, so we may end up with this dependency again. If it's better to have it split out into a standalone BUILD.gn I think I'd rather keep it.

We want to allow Dart SDK hackers to test Dart SDK builds with local source changes, so we may end up with this dependency again. If it's better to have it split out into a standalone BUILD.gn I think I'd rather keep it.

I don't follow how that's related. The existing way to hack on the Dart SDK in the engine tree is to make changes under third_party/dart, and to use the --no-prebuilt-dart-sdk flag to the gn script. Does that not work for some reason?

If the dart-sdk target is in flutter/BUILD.gn, then the wasm build cannot consume or use it because the wasm build can't actually touch flutter/BUILD.gn without pulling in transitive dependencies that break under a wasm build. The transitive dependencies break even if you aren't building any of their targets, and they are dependencies we don't necessarily have control over.

That being said, we aren't actually using the dart-sdk target for the web build anymore, we just use prebuilts. I also don't actually see us adding a dependency on the dart-sdk target in the future either, and if we did it would be in a different manner that would probably build it with the non-default toolchain, since it's weird to pull host artifacts into the wasm build tree. So actually for our purposes I don't care anymore, and if you want me to refactor that target back into flutter/BUILD.gn I don't have any real practical objection, but I don't actually understand why that would be desirable. flutter/BUILD.gn is a bit of a kitchen sink and probably would be easier to work with if it were broken up into smaller logical components, and so having the dart-sdk separated into a different file seems better to me than keeping it in flutter/BUILD.gn. But again we don't really depend on either any more, so I don't mind either way you want to go with it.

zanderso · 2022-10-17T15:30:23Z

common/config.gni

@@ -22,6 +22,8 @@ declare_args() {

  # Whether to use a prebuilt Dart SDK instead of building one.
  flutter_prebuilt_dart_sdk = false
+
+  build_flutter_web_sdk = false


Please add a comment for this arg.

zanderso · 2022-10-17T15:31:57Z

tools/gn

@@ -627,6 +633,12 @@ def to_gn_wasm_args(args, gn_args):
  gn_args['skia_canvaskit_enable_webgpu'] = False
  is_profile_build = args.runtime_mode == 'profile' or args.runtime_mode == 'debug'
  gn_args['skia_canvaskit_profile_build'] = is_profile_build
+  gn_args['flutter_prebuilt_dart_sdk'] = True
+
+  # TODO(jacksongardner): Make this based off of the input argument rather


Ditto comment about linking TODOs to github issues.

zanderso · 2022-10-17T15:33:03Z

tools/gn

+      action='store_true',
+      help='build canvaskit from source'
+  )
+


For other args there's no extra newline between the arg and the --no version.

zanderso · 2022-10-17T15:38:21Z

web_sdk/BUILD.gn

+    if (target_os == "wasm") {
+      output = "flutter-web-sdk.zip"
+    } else {
+      # TODO(jacksongardner): remove this once we stop making platform-specific


This reverts commit 7a1be8b.

eyebrowsoffire added the Work in progress (WIP) Not ready (yet) for review! label Sep 30, 2022

flutter-dashboard bot added platform-web Code specifically for the web engine needs tests labels Sep 30, 2022

Build web sdk in wasm tree.

543b02d

eyebrowsoffire force-pushed the web_sdk_build branch from 5c53a9c to 543b02d Compare October 7, 2022 23:59

eyebrowsoffire added 4 commits October 7, 2022 17:26

Use prebuilts to build tests.

07690ee

Don't create the copy target if flutter_prebuilt_dart_sdk is false

2259916

Felt test without build.

d1a9f0a

Remove debug statement I accidentally left in.

4650b9e

mdebbar reviewed Oct 8, 2022

View reviewed changes

eyebrowsoffire added 8 commits October 10, 2022 14:29

Refactored ddc and outline generation in gn files.

1041f93

Remove dependency on dart-sdk copy.

e4f0c0d

Formatting stuff.

81eff9c

Use host_prebuilt_dart_sdk instead of just prebuilt_dart_sdk

82812b6

Don't make the flutter_web_sdk artifact on android builds, is isn't u…

f3b6cfd

…sed.

Change strategy for short-circuiting the flutter_web_sdk archive.

a9e7f3f

Merge branch 'main' into web_sdk_build

9d1d6ae

Don't produce a pointless xcode/VS project when building wasm.

8bfb22f

eyebrowsoffire mentioned this pull request Oct 12, 2022

Update recipes to decouple web builds and tests from the host build flutter/flutter#113303

Closed

eyebrowsoffire added 6 commits October 13, 2022 15:36

Change up the way we are treating flags, and package canvaskit with t…

094c678

…he web sdk artifact.

Force --build-canvaskit until we update the recipes.

d81efbd

Merge branch 'main' into web_sdk_build

a0d5345

Oops, only build canvaskit in the web case.

aeaa119

flutter_web_sdk need not be built on in linux_host_release.

3d667d2

Change target for linux_host_engine drone.

924697d

yjbanov approved these changes Oct 14, 2022

View reviewed changes

mdebbar approved these changes Oct 14, 2022

View reviewed changes

eyebrowsoffire added autosubmit Merge PR when tree becomes green via auto submit App and removed Work in progress (WIP) Not ready (yet) for review! labels Oct 14, 2022

auto-submit bot merged commit 7a1be8b into flutter:main Oct 14, 2022

zanderso reviewed Oct 14, 2022

View reviewed changes

dart_sdk/BUILD.gn

@@ -0,0 +1,24 @@

import("//flutter/common/config.gni")

Copy link

Member

zanderso Oct 14, 2022

Choose a reason for hiding this comment

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

This file needs a copyright header

zanderso reviewed Oct 17, 2022

View reviewed changes

eyebrowsoffire mentioned this pull request Oct 17, 2022

Engine post-submit CI - Mac Web Engine Failures flutter/flutter#113548

Closed

eyebrowsoffire added revert Label used to revert changes in a closed and merged pull request. and removed revert Label used to revert changes in a closed and merged pull request. labels Oct 17, 2022

eyebrowsoffire added a commit to eyebrowsoffire/engine that referenced this pull request Oct 17, 2022

Revert "Build Web SDK in wasm build tree (flutter#36520)"

35bc98b

This reverts commit 7a1be8b.

eyebrowsoffire mentioned this pull request Oct 17, 2022

Revert "Build Web SDK in wasm build tree (#36520)" #36805

Merged

auto-submit bot pushed a commit that referenced this pull request Oct 17, 2022

Revert "Build Web SDK in wasm build tree (#36520)" (#36805)

cafafa1

auto-submit bot mentioned this pull request Oct 17, 2022

Review request for Revert PR flutter/engine#36805 flutter/flutter#113561

Closed

eyebrowsoffire mentioned this pull request Oct 18, 2022

Web sdk build reland #36851

Merged

whesse mentioned this pull request Jan 30, 2023

wasm_release builds do not work with --no-prebuilt-dart-sdk flutter/flutter#119513

Closed

Build Web SDK in wasm build tree #36520

Build Web SDK in wasm build tree #36520

Uh oh!

Conversation

eyebrowsoffire commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Sep 30, 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!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eyebrowsoffire commented Sep 30, 2022 •

edited

Loading