Override the WASM toolchain to use CanvasKit-specific args #611

harryterkelsen · 2022-09-06T22:24:48Z

Causes the WASM toolchain to use CanvasKit specific arguments.

This allows us to build CanvasKit cross-compiled as part of the normal "host_debug" build, without affecting the normal
Skia build used for Flutter on mobile devices and desktop.

This enables flutter/engine#32510

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 the Flutter Style Guide recently, and have followed its advice.
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.

dnfield · 2022-09-07T16:42:23Z

build/toolchain/gcc_toolchain.gni

+    if (defined(invoker.toolchain_args)) {
+      toolchain_args = invoker.toolchain_args


nit: I think instead of letting the invoker specify the toolchain args, can we just let the invoker specify some other object like extra_toolchain_args and append them here?

I think GN will let you do that with +=, but maybe I'm wrong. It seems strange to let the invoker change the target_os/target_cpu/is_clang this way.

Just tried this. Unfortunately you can't += a block. But I think I was able to find a way using forward_variables_from

dnfield · 2022-09-07T16:43:13Z

build/toolchain/wasm/BUILD.gn

+    skia_use_angle = false
+    skia_use_dng_sdk = false
+    skia_use_dawn = false
+    skia_use_webgl = true
+    skia_use_webgpu = false
+    skia_use_expat = false
+    skia_use_fontconfig = false
+    skia_use_freetype = true
+    skia_use_libheif = false
+    skia_use_libjpeg_turbo_decode = true
+    skia_use_libjpeg_turbo_encode = false
+    skia_use_libpng_decode = true
+    skia_use_libpng_encode = true
+    skia_use_libwebp_decode = true
+    skia_use_libwebp_encode = false
+    skia_use_lua = false
+    skia_use_piex = false
+    skia_use_vulkan = false
+    skia_use_wuffs = true
+    skia_use_zlib = true
+    skia_enable_gpu = true
+    skia_build_for_debugger = false
+    skia_enable_sksl_tracing = false
+    skia_use_icu = true
+    skia_use_harfbuzz = true
+    skia_enable_fontmgr_custom_directory = false
+    skia_enable_fontmgr_custom_embedded = true
+    skia_enable_fontmgr_custom_empty = false
+    skia_enable_skshaper = true
+    skia_enable_skparagraph = true
+    skia_enable_pdf = false
+    skia_canvaskit_force_tracing = false
+    skia_canvaskit_enable_skp_serialization = true
+    skia_canvaskit_enable_effects_deserialization = false
+    skia_canvaskit_enable_skottie = false
+    skia_canvaskit_include_viewer = false
+    skia_canvaskit_enable_particles = false
+    skia_canvaskit_enable_pathops= true
+    skia_canvaskit_enable_rt_shader = true
+    skia_canvaskit_enable_matrix_helper = false
+    skia_canvaskit_enable_canvas_bindings = false
+    skia_canvaskit_enable_font = true
+    skia_canvaskit_enable_embedded_font = true
+    skia_canvaskit_enable_alias_font = true
+    skia_canvaskit_legacy_draw_vertices_blend_mode = false
+    skia_canvaskit_enable_debugger = false
+    skia_canvaskit_enable_paragraph = true
+    skia_canvaskit_enable_webgl = true
+    skia_canvaskit_enable_webgpu = false


We've typically handled these arguments as things specified in args.gn set up by the gn utility script in the engine repo. Why do we want to move them here? This seems to further complicate buildroot when we're trying to get rid of it/merge it with the engine repo.

The reason I'm doing it this way is because these skia arguments interfere with the Skia build used by Flutter's mobile engines. An example is enabling skia_enable_webgl causes Skia to build with the webgl backend, which is wrong for Flutter mobile.

Sure, but does that just mean we should build canvaskit in its own out directory with its own args?

I think it'll be confusing to see these values set differently in args.gn but actually get overridden here.

We want to build CanvasKit as part of the flutter_web_sdk folder as part of the standard Flutter engine build (e.g. under host_debug_unopt)

We want to build CanvasKit as part of the flutter_web_sdk folder as part of the standard Flutter engine build (e.g. under host_debug_unopt)

@chinmaygarde @zanderso WDYT?

I'm concerned it will be confusing to start introducing artifacts with very different targets/archiectures/build arguments under the same out directory. I'd expect, for example, that we build canvaskit as something like out/wasm_debug or something. There are a number of advantages to keeping wasm separate from the host* builds too, since it doesn't really differ based on host or target architecture type (so canvaskit.wasm and .js are the same whether you're building on or for x64 or arm64, etc.).

It's not clear to me how this will co-exist properly in a single out directory - for example, what will happen when multiple targets want to build the same Skia TUs with different build args?

I think global toolchain configs were a big bug in our system, and this PR is a step in the right direction. A single invocation of gn + ninja should be able to build multiple things targeting different architectures and configurations.

This is possible in GN, but I think it would at least require explicitly plumbing a toolchain in many places where it's currently implicit. I wouldn't be surprised if the additional plumbing needed to cross repo boundaries.

It's not clear to me how this will co-exist properly in a single out directory - for example, what will happen when multiple targets want to build the same Skia TUs with different build args?

Not sure I'm understanding the concern, but specifying a different toolchain makes a different target. Build results for the default toolchain go directly under out/. Build results for a different toolchain go under a subdirectory of out/, like out/clang_x64.

Ahh, right, ok

@hterkelsen - do you think you could try what Zach is suggesting?

I think my suggestion might not scale to the number of arguments that need to be redefined. If it's not possible to bring it down to a smaller number, and introduce wasm/canvaskit specific targets to Skia, then I am okay with this PR as-is.

dnfield

Requesting changes at least to give time to get feedback from more of engine team.

chinmaygarde

Toolchains should not have or enforce opinions about the how they build specific targets. That's the responsibility of .gn* files in the targets and their users. I believe toolchain args are also interpreted after gn args. So there is no way (please verify) to configure these using the args.gn mechanism and folks will be confused about why their settings are not taking hold when using just this one wasm toolchain. We got into similar messes in the past having global configurations applied to toolchains and those bit-rotting after we updated dependencies. For instance, I think we still have some Valgrind specific global defines left over from the Chromium days even though we have never used it ourselves. This way of specifying toolchain args also does not compose super well.

I think global toolchain configs were a big bug in our system...

@yjbanov: Toolchain configs are not global. You can specify the toolchain for a target explicitly //target/to/build:target(//toolchain/to/use:toolchain). If you skip the toolchain, the one set by set_default_toolchain takes hold.

But what are global are the flags we generally override via declare args. These have been the cause of the whole sordid affair with having multiple out directories.

This patch is perhaps doing too many things here in unblocking the skwasm work and attempting to realize @yjbanovs wish (and mine for whats it's worth) of having a single GN + Ninja step building for multiple architectures. The latter is a much larger effort that would need reworking the declare args blocks to use GN template that callers can use. The former can be unblocked right now with a different out directory. We should split the two efforts if necessary.

zanderso · 2022-09-08T19:09:15Z

build/toolchain/wasm/BUILD.gn

  link_outputs = [ "{{root_out_dir}}/{{target_output_name}}.wasm" ]
+
+  extra_toolchain_args = {
+    skia_use_angle = false


These can be hooked up to GN args from args.gn by introducing new arguments with declare_args like skia_use_angle_for_wasm (or a better name).

Do you mean that we'd have a file called args.gn over in flutter/engine that enumerates all the properties below, and it would override the same properties listed in https://github.com/google/skia/blob/main/modules/canvaskit/canvaskit.gni?

No, sorry. I need to think about this PR some more. I'm going to spend some time with it this morning, and see if I can make some better suggestions.

It seems all these attributes are destructured into some if/else block in the skia_wasm_lib used to build canvaskit: https://github.com/google/skia/blob/main/modules/canvaskit/BUILD.gn#L96

Can we convert the Skia rule into a template that takes these same attributes as invoker.ATTR?

Yes. I think that would be a better way to go here.

zanderso

I did some local experimentation. When a target is built with the wasm toolchain, the default values of GN arguments will be re-evaluated with is_wasm set to true. Then, those defaults will be overridden by the contents of args.gn (in the Engine build, these are the values setup by the tools/gn script.) Finally, toolchain_args for the active toolchain applies the final layer of overrides.

This means that the contents of toochain_args for the wasm toolchain only needs to contain overrides where it wants something different than both the default and what is in args.gn.

However, I think before adding entries to the wasm toolchain's toolchain_args, we should investigate why they're necessary. One example I found was skia_use_metal. The default is false, which works for wasm, but the macOS host build overrides it to true. One way to fix this for wasm would be to add skia_use_metal = false to the toolchain_args, but a better way to fix it would be to change reads of skia_use_metal to instead be skia_use_metal && !is_wasm.

Continuing in that direction, I think we can minimize the leakage.

harryterkelsen · 2022-09-13T21:07:35Z

Closing this. After discussion with the engine team we have decided on a different approach: #622

Override the WASM toolchain to use CanvasKit-specific args

236575b

harryterkelsen requested review from dnfield and yjbanov September 6, 2022 22:24

dnfield reviewed Sep 7, 2022

View reviewed changes

dnfield suggested changes Sep 7, 2022

View reviewed changes

harryterkelsen added 4 commits September 7, 2022 11:15

Refactor to use extra_toolchain_args instead

6dca5e7

Remove extraneous changes

27cf07f

Rename to wasm_for_canvaskit and add comment

bd693c8

Narrow down args

09c2cf7

chinmaygarde suggested changes Sep 8, 2022

View reviewed changes

zanderso reviewed Sep 8, 2022

View reviewed changes

zanderso reviewed Sep 9, 2022

View reviewed changes

Fix build with some more arguments

bdb087b

harryterkelsen closed this Sep 13, 2022

mdebbar mentioned this pull request Dec 21, 2022

[web] Expose wasm_toolchain to customize CanvasKit through toolchain_args #663

Merged

		if (defined(invoker.toolchain_args)) {
		toolchain_args = invoker.toolchain_args

Override the WASM toolchain to use CanvasKit-specific args #611

Override the WASM toolchain to use CanvasKit-specific args #611

Uh oh!

Conversation

harryterkelsen commented Sep 6, 2022

Pre-launch Checklist

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde 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!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

harryterkelsen commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants