-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Deprecate -sUSE_WEBGPU in favor of --use-port=emdawnwebgpu #24220
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
Deprecate -sUSE_WEBGPU in favor of --use-port=emdawnwebgpu #24220
Conversation
7b73690
to
6eff958
Compare
Hi, what about firefox or browsers that dont use dawn? |
emdawnwebgpu is independent of browser (except it supports a few experimental features that are only in Chromium browsers). They're actually just a fork of the bindings currently in Emscripten, the only difference is we're maintaining them in the Dawn repository instead of here. I should clarify this in the deprecation message and/or in the emdawnwebgpu docs. |
0eb1be6
to
b23f8e8
Compare
I think we should maybe wait until we have a way for folks to do |
I agree 100%. It would be better to deprecate |
Seems reasonable. |
Adds a linker warning for using -sUSE_WEBGPU, and update documentation. Emscripten's implementation of webgpu.h is unmaintained. We want people to move to Dawn's emdawnwebgpu implementation (fork). However, the API has changed substantially, so this not a clean transition. Plus, there might be new bugs in that implementation. So we need a decently long deprecation period before removing the old implementation. Issue: 23432
b23f8e8
to
72bcff5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a ChangeLog entry?
be8431a
to
35d443b
Compare
3fdc55f
to
eb3ce73
Compare
@sbc100 I think this is ready again. I've done a bunch of stuff since your last full review, so could you review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comments
The I've removed those for now but how would I fix that? |
The other test suite runs in mode where it assumes that all libraries have already been built by the In order to ensure we don't regresss here we use If you want a port to be available in PIC mode here I think you would need to add it to |
Also the name of the library looks a little odd. Libraries don't normally have an underscore after the |
To play devil's advocate here, I would argue that |
It looks odd because it doesn't follow the standard naming convention. You could argue the convention is wrong, or needs updating, but I don't think this is the right forum for that. |
Thanks for the tip! It's probably not necessary, I think I'd rather not add a port to that list. |
This name comes from the upstream port, so I'll take note and consider changing it at some point while I'm doing something else (doesn't seem important enough to make a whole CL to change). |
The point of that list is purely to enable this kind of testing. Users won't be using that list. And also |
Indeed, not a blocker. |
Ah, then it wouldn't be so bad. Though thinking about it, it looks like that's a list of filenames, and since we have a filename that changes constantly it would introduce friction to the roll (unless maybe we detect that we're being downloaded in "remote port" configuration and remove the hash). It's fine, we want the more rigorous testing in Dawn anyway so we catch issues before landing them. |
Add a linker warning for using
-sUSE_WEBGPU
pointing to--use-port=emdawnwebgpu
, and update documentation.Moves the port from
contrib.emdawnwebgpu
to justemdawnwebgpu
since we'll keep maintaining it as we work on Emdawnwebgpu.Closes #23432
Starts fixing #24265