Skip to content

Use dart2wasm support-detection scripts #3845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

simolus3
Copy link
Contributor

dart2wasm started emitting a .support.js file which contains a JavaScript expression evaluating to true if the browser supports the WebAssembly module emitted by dart2wasm: dart-lang/sdk#59951.

With this PR, we start reading that file and use that as a feature detection if available.

Copy link
Contributor

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments.

I'll be the main person working on build_runner for a while, I'm focused on performance first but will also need to learn all the other things :) ... hopefully my comments make some kind of sense ;)

if (loaderExtension == null || wasmCompiler == null) {
if (loaderExtension == null ||
wasmCompiler == null ||
dart2WasmResult == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it will now do nothing in cases where previously it might have done something? When dart2WasmResult is null and the others are not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To add context here: When we're compiling with both dart2js and dart2wasm, we don't let dart2js emit a .dart.js file like normally but instead make it .dart2js.js.
Then, we emit our own .dart.js loader file that includes this check to determine whether we use the program emitted by dart2js or the module emitted by dart2wasm. This script is necessary whenever dart2wasm is enabled because that compiler doesn't emit a ready entrypoint but rather just a JS module bundle that can't be added to a <script> tag directly.

There's an issue in the current logic when one (but not both) of the compilers fail to compile a program. We don't currently react to this and will try to load scripts that don't exist. This additional check doesn't fix that properly though, I've reverted them (maybe something for a different PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks :)

Copy link
Contributor

@davidmorgan davidmorgan left a comment

Choose a reason for hiding this comment

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

All looks good, thanks.

Will wait to see if anyone from the US side wants to review as well.

Unrelated, we have a discussion section now, it might be useful for more general discussion about the work / direction.

if (loaderExtension == null || wasmCompiler == null) {
if (loaderExtension == null ||
wasmCompiler == null ||
dart2WasmResult == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks :)

@davidmorgan
Copy link
Contributor

There is some issue with CI that I'm trying to fix, as part of it it seems there might already be a failure in this code on Windows, any ideas please?

https://github.com/dart-lang/build/actions/runs/13284718347/job/37090779528?pr=3851

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:build 2.4.2 already published at pub.dev
package:build_config 1.1.2 already published at pub.dev
package:build_daemon 4.0.4 already published at pub.dev
package:build_modules 5.0.11 already published at pub.dev
package:build_resolvers 2.4.4-wip WIP (no publish necessary)
package:build_runner 2.4.15-wip WIP (no publish necessary)
package:build_runner_core 8.0.1-dev ready to publish build_runner_core-v8.0.1-dev
package:build_test 2.2.4-wip WIP (no publish necessary)
package:build_web_compilers 4.1.2-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.

Copy link

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

@simolus3
Copy link
Contributor Author

Was that the first time it has failed or has this been broken for a while? It looks like dart2wasm is no longer honoring the multi-root-scheme option, but I'm also not sure why (and also confused about this only happening on Windows).

@davidmorgan
Copy link
Contributor

It's the first time I've seen it.

Possibly it's caused by an SDK change; there was a new "unused element" warning for dart:js that I hadn't seen before either.

But it could also be that the CI has not been working smoothly.

I think the next thing to try is to repro with different SDK versions, I'll try to get to that today/tomorrow.

@davidmorgan
Copy link
Contributor

Dusting off my Windows VM now, let's see if I can figure it out :)

@davidmorgan
Copy link
Contributor

Yes it's due to an SDK change: I can repro on 3.8.0-edge.66fa0e2a491371b098323a0311a0e400ad1fad65 (main) (Thu Feb 13 01:49:09 2025 -0800) but not on stable or on the also very recent 3.8.0-91.0.dev (dev) (Tue Feb 11 16:06:19 2025 -0800). Lemme track down the commit...

@davidmorgan
Copy link
Contributor

Looks like https://dart-review.googlesource.com/c/sdk/+/397721 is probably the cause as it's the only dart2wasm commit in the range; I'll try to verify.

@davidmorgan
Copy link
Contributor

Hmm actually let me pin the SDK version so we can have working CI and get unstuck #3856

@davidmorgan
Copy link
Contributor

Please merge in #3856 so we get a good CI run, then let's land this--thanks for your patience!

@kevmoo
Copy link
Member

kevmoo commented Feb 13, 2025

How are we handling older SDKs that DON'T have the feature? We're not bumping the min SDK

@davidmorgan
Copy link
Contributor

How are we handling older SDKs that DON'T have the feature? We're not bumping the min SDK

It just falls back to the existing behaviour.

@davidmorgan
Copy link
Contributor

@simolus3 it turns out I can merge in the CI fix :) and it passes. So I'll merge this now.

I wanted to get it in before my #3854 because that's relatively disruptive.

I have no idea how this usually works--does it make sense for me to release build_web_compilers with this change, or wait for some more to accumulate? Thanks!

@davidmorgan davidmorgan merged commit dd4ea07 into dart-lang:master Feb 13, 2025
76 checks passed
@kevmoo
Copy link
Member

kevmoo commented Feb 13, 2025

@simolus3 – you're a MACHINE! Thanks!

@kevmoo
Copy link
Member

kevmoo commented Feb 13, 2025

FYI @mkustermann 😄

@simolus3 simolus3 deleted the dart2wasm-support-scripts branch February 13, 2025 19:52
@simolus3
Copy link
Contributor Author

Thanks for your help getting this merged!

I have no idea how this usually works--does it make sense for me to release build_web_compilers with this change, or wait for some more to accumulate? Thanks!

I actually don't know what the typical workflow is, but since the SDK change is very recent and won't land in stable for a while, I think it's fine to not bother with a release right now.

@davidmorgan
Copy link
Contributor

Thanks for your help getting this merged!

I have no idea how this usually works--does it make sense for me to release build_web_compilers with this change, or wait for some more to accumulate? Thanks!

I actually don't know what the typical workflow is, but since the SDK change is very recent and won't land in stable for a while, I think it's fine to not bother with a release right now.

Noted, thanks. Feel free to ping me any time you feel a release is overdue.

@simolus3
Copy link
Contributor Author

Feel free to ping me any time you feel a release is overdue.

Are the build_test changes mentioned in the build_web_compilers changelog ready for release? It sounds like we'd have to release the two packages together to adopt the breaking changes.
Given that dart2wasm has started emiting code for which feature detection is helpful (dart-lang/sdk#59951 (comment)), I think it may be a good time to get this out.

@davidmorgan
Copy link
Contributor

Hmmm yes it does need the build_test breaking changes.

build_test is starting to settle down but I don't think it's quite ready for release yet, I think probably StubWriter and WrittenAssetWriter will also go away to finish the reader/writer refactoring. Actively working on that :)

Thanks.

@davidmorgan
Copy link
Contributor

Oh ... but actually it's only a dev dependency, I think it should work fine to release. Let me try it :)

@davidmorgan
Copy link
Contributor

#3895

@davidmorgan
Copy link
Contributor

@simolus3 I think that worked :)

@simolus3
Copy link
Contributor Author

simolus3 commented Mar 4, 2025

Unfortunately it looks like the publishing action might be broken :( I don't see the version on pub.dev either.

edit Aaand it got fixed the millisecond I wrote that comment. Thanks!

@davidmorgan
Copy link
Contributor

Coincidentally I checked and someone helped me make it actually publish, so, yup, now I think it worked ;) thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants