Support compiling to wasm in `build_web_compilers`

[simolus3]

Add dart2wasm as a compiler option for build_web_compilers.

This is still incomplete, since:

• [x] We need to wait for dart compile wasm to support --multi-root: https://dart-review.googlesource.com/c/sdk/+/377662
• [ ] It's not clear if we want to let users customize the module loader, e.g. by injecting additional imports or wasm files. That would probably be hard to pull off with the existing implementation.
• [x] This should support custom defines / compiler options.
• [x] It obviously needs integration tests.

Closes https://github.com/dart-lang/build/issues/3621

[simolus3]

This would imply being able to compile both with dart2js and with dart2wasm simultaneously, which somewhat complicates things.

I actually ran into this problem already, haha. I thought I could just apply the entrypoint builder twice to different targets to fix this, but build_runner doesn't like that due to conflicting outputs.

I think in the end we may want to support four options in the compiler argument then?

1. dartdevc - as is
2. dart2js - as is
3. dart2wasm - new option, only compile with dart compile wasm
4. both (ideally the new default) - compile with dart2js and dart2wasm.

For 4, the question is how the auto-switching would look like. We could embed a suffix to the dart2js entrypoint file to either invoke the dartProgram directly or instead load the wasm file and use that. That uses fewer files, but requires users to download the potentially large dart2js file. Or we could change the dart2jsBootstrap logic to be aware of a wasm build happening as well, and then emit say a .dart2js.js file instead. Then we generate a small additional .js entrypoint file starting a feature detection and loading the appropriate file? From a quick look that appears to be what Flutter is doing.

[kevmoo]

I support getting wasm working first, then enabling the runtime detection. We should look to see what Flutter does here. I wish we had a good place to put the shared logic!

[jakemac53]

I think in the end we may want to support four options in the compiler argument then?

We may need dart2wasm+DDC and dart2wasm+dart2js eventually?

I was thinking about splitting out whether "wasm" is enabled or not as a separate thing? Then we don't have to have an option for every combination. Maybe, an enable_wasm boolean option, instead of making it a part of the compiler option. And then people can set the compiler separately from whether or not they enable wasm. Possibly a new "none" compiler option for just wasm. It would make more sense if "compiler" was "js_compiler" or something though.

Or we could change the dart2jsBootstrap logic to be aware of a wasm build happening as well, and then emit say a .dart2js.js file instead. Then we generate a small additional .js entrypoint file starting a feature detection and loading the appropriate file? From a quick look that appears to be what Flutter is doing.

Yeah I think we need something like this.

I support getting wasm working first, then enabling the runtime detection. We should look to see what Flutter does here. I wish we had a good place to put the shared logic!

I am fine with that, but we need to make sure the user facing configuration is compatible with a future mode that does runtime detection.

[simolus3]

We may need dart2wasm+DDC and dart2wasm+dart2js eventually?

I agree that eventually compiling with both + emitting a small launcher doing feature detection and picking the appropriate one is the way to go. It's also a good default.

However, there are also some cases where we want to compile with both, but also make an explicit decision about which one to use. One case that comes to mind is prebuilt tests with build_test when passing --compiler dart2js --compiler dart2wasm. We really need both to be compiled by build_web_compilers for that, but the test runner needs some way of then loading the right one. Given that prebuilt dart2wasm appears to be supported by package:test already even though it's not supported by build_web_compilers, I assume there's another tool somewhere generating prebuilt wasm tests? What are the outputs required for that setup?

[jakemac53]

iven that prebuilt dart2wasm appears to be supported by package:test already even though it's not supported by build_web_compilers, I assume there's another tool somewhere generating prebuilt wasm tests?

I don't believe there is one, the SDK does completely its own thing for wasm testing (I have no idea how this works).

[simolus3]

I think this PR now covers everything for basic dart2wasm support - I've opened https://github.com/dart-lang/build/issues/3730 for the future work of compiling with multiple compilers.

@jakemac53 I've added you as a reviewer on https://dart-review.googlesource.com/c/sdk/+/377662 which needs to land before we can ship this. I don't what the right process is to get SDK CLs reviewed, could you help me find reviewers for dart2wasm that may review that CL?

[kevmoo]

@simolus3 – if we wait for 3.6.0-150.0.dev, that should have https://github.com/dart-lang/sdk/commit/ca77beac58dc957da385424fbb7a6ed7f8487eb6

We'll just need to ponder if we want build_web_compilers limited to that SDK for a while. Something to discuss...

[jakemac53]

Fwiw I am perfectly fine with build_web_compilers only supporting dev SDKs or whatever, it is often in that state. And nobody seems to complain 🤷‍♂️

[kevmoo]

@jakemac53 – ditto!

[kevmoo]

Ooo! Looks like 3.6.0-150.0.dev has the fix you want.

So dart:io -> vm only dart:js -> JS only otherwise, wasm

[kevmoo]

@simolus3 – for auto-switch, have you looked at the code in Flutter?

[simolus3]

@simolus3 – for auto-switch, have you looked at the code in Flutter?

Flutter is doing a lot more (like also importing skwasm and so on), but the loader script is minified from these sources using esbuild. I think in the end we can pretty much copy this check to detect whether the browser supports dart2wasm. It's unlikely that we can ever share the files though.

Is it fine if we do this in another PR?

[kevmoo]

Need to do some work to be CI happy...

[simolus3]

The latest release on the dev channel is still 3.6.0-149.0.dev, so it looks like we'll have to wait for that to get updated first.

[github-actions[bot]]

PR Health

Changelog Entry :heavy_check_mark:

| Package | Changed Files | | :--- | :--- | Changes to files need to be [accounted for](https://github.com/dart-lang/ecosystem/wiki/Changelog) in their respective changelogs.

Package publish validation :heavy_check_mark:

| Package | Version | Status | | :--- | ---: | :--- | | package:build | 2.4.2-wip | WIP (no publish necessary) | | package:build_config | 1.1.2-wip | WIP (no publish necessary) | | package:build_daemon | 4.0.3-wip | WIP (no publish necessary) | | package:build_modules | 5.0.10-wip | WIP (no publish necessary) | | package:build_resolvers | 2.4.3-wip | WIP (no publish necessary) | | package:build_runner | 2.4.13-wip | WIP (no publish necessary) | | package:build_runner_core | 7.3.3-wip | WIP (no publish necessary) | | package:build_test | 2.2.3-wip | WIP (no publish necessary) | | package:build_web_compilers | 4.0.12-wip | WIP (no publish necessary) | | package:scratch_space | 1.0.3-wip | WIP (no publish necessary) | Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[kevmoo]

I guess we DON'T have a release after -149. 🤷

[simolus3]

Looks like 3.6.0-165.0-dev has just been released, so maybe it works when triggering the CI again?

[kevmoo]

Just pushed @simolus3 with 3.6.0-164.0.dev

Let's see!

[kevmoo]

We're looking a LOT better on tests!

Some of the health checks need some looking at

[simolus3]

Some of the health checks need some looking at

They are complaining about missing changelog entries for the pubspec changed in the other packages. The only reason we had to change these pubspecs was for mono_repo to run tests with the latest Dart version. We have to use the latest Dart version everywhere thanks to workspaces, since we can't run pub get on any package in the workspace with a Dart release too old to support build_web_compilers.

So we now have to make every package in this workspace depend on ^3.6.0-165.0.dev, with a changelog entry and everything, because otherwise mono_repo thinks it can run tests with 3.5.0. That's a ridiculous churn for users over nothing. Shouldn't mono_repo use only the highest Dart SDK of any package in the workspace?

[jakemac53]

We have to use the latest Dart version everywhere thanks to workspaces, since we can't run pub get on any package in the workspace with a Dart release too old to support build_web_compilers.

Hmm that is an interesting point regarding workspaces... cc @jonasfj @sigurdm have we put any thoughts into solutions there? I don't have any particular suggestion or idea, seems like it is probably just a somewhat unavoidable consequence.

[jakemac53]

Fwiw, one option is to remove build_web_compilers from the workspace for a bit. That would also be somewhat unfortunate, but maybe better in the long term.

[jakemac53]

Shouldn't mono_repo use only the highest Dart SDK of any package in the workspace?

Each individual package chooses which versions it runs on in its mono_pkg.yaml file. They are opting to run with their "pubspec" version, because we only want to claim support for things we can actually test on CI.

So, yes we could only run all packages on a specific version, and mono_repo does support that, but we would end up wanting to increase the pubspecs anyways because we have a policy to only support what we can test (on CI).

[sigurdm]

have we put any thoughts into solutions there? I don't have any particular suggestion or idea, seems like it is probably just a somewhat unavoidable consequence.

Yeah - I think that is hard to avoid requiring at least the dart that supports workspaces if you want to use workspaces.

You could (perhaps) make the CI script such that it strips the workspace and resolution fields from the pubspecs before testing. But I'm not sure it is worth it.

FWIW Dart 3.5.0 has workspaces support (though unannounced because it doesn't work in flutter).

[jakemac53]

Yeah - I think that is hard to avoid requiring at least the dart that supports workspaces if you want to use workspaces.

Yeah I am not concerned about requiring 3.5 (for workspace support generally). It is more the requirement of all packages having the same min SDK in order to be tested on their min SDK that is unfortunate (since it isn't a one time thing, that will be a persistent issue).

We could certainly hack up the pubspecs to remove them from the workspace but that seems pretty unfortunate. It is quite a lot more complicated than it seems also, because you would often also need to add dependency overrides, which you don't normally put in individual packages for workspaces (I think it isn't allowed).

Maybe it would be possible to pass a flag to pub upgrade to tell it to only solve for the current package, even if it is in a workspace, including respecting any dependency overrides in the workspace pubspec and implicitly using path dependencies for other packages in the repo? That sounds like kind of a complicated feature potentially though, but it is essentially what is needed here.

[simolus3]

Fwiw, one option is to remove build_web_compilers from the workspace for a bit. That would also be somewhat unfortunate, but maybe better in the long term.

I've done that, let's see if it works. A downside is that _test can't be part of the workspace either then (because it needs a dependency override to the path build_web_compilers, and dependency overrides are shared in workspaces).

[kevmoo]

Thanks for your patience here @simolus3 – lets see if tests pass

[sigurdm]

Yeah I am not concerned about requiring 3.5 (for workspace support generally). It is more the requirement of all packages having the same min SDK in order to be tested on their min SDK that is unfortunate (since it isn't a one time thing, that will be a persistent issue).

Not sure I follow - that is a feature request for mono_repo?

Maybe it would be possible to pass a flag to pub upgrade to tell it to only solve for the current package, even if it is in a workspace, including respecting any dependency overrides in the workspace pubspec and implicitly using path dependencies for other packages in the repo? That sounds like kind of a complicated feature potentially though, but it is essentially what is needed here.

I think this is the same request as https://github.com/dart-lang/pub/issues/4357

[simolus3]

Let me know if there's anything else for me to do here now that the tests are passing. I think the package version thing is a mono_repo issue primarily (it tests each package with the oldest SDK it claims to support, which can break in workspaces if other packages require newer SDKs). I've added the workaround of removing build_web_compilers from the workspace in the meantime, I think we can revert that change with the 3.6.0 stable release.

[jakemac53]

I think this is the same request as dart-lang/pub#4357

Similar, but not quite. The main difference is I would still want it to use a path dependency for all the packages in the workspace that the package does depend on. I do have access to the entire workspace, and not just a single package, and the package may rely on unpublished functionality from some of the workspace packages.

[jakemac53]

Just took a final pass with a few final nits, then we can land this. @kevmoo would we want to ship this ASAP?

[sigurdm]

Similar, but not quite. The main difference is I would still want it to use a path dependency for all the packages in the workspace that the package does depend on. I do have access to the entire workspace, and not just a single package, and the package may rely on unpublished functionality from some of the workspace packages.

Not sure I follow - if you feel like it, could you open a feature request with perhaps an example scenario of what you would like to be able to do?