Allow externally hosted contrib ports #24303
Conversation
|
After investigating the code further, I may have found a solution for the cache issue: I added Please let me know what you think of theses changes. I don't understand why |
|
@sbc100 Note that it is no longer a draft pull request as the implementation is working. The test added works: The test failures don't seem to be related, for example: This test passes locally. Of course the So at this stage I am awaiting your feedback to see if this is the right direction and what you want me to do to move forward with this PR. I will obviously update documentation/Changelog once you are satisfied with the changes |
sbc100
left a comment
There was a problem hiding this comment.
This approach seem reasonable yes.
Can update the PR description with some more details.
|
@sbc100 I have made the changes you requested. Here are some comments: I added a TODO comment for In regards to the "delay" feature, I implemented the change you requested but I want you to know that I do not agree with it and here is an example that demonstrates why: As you can see from this example, a local port behaves differently from a remote port. After deleting the |
Yup, looks like you are correct. I was assuming that the build directory for the remote port was not even a thing. I don't love the way we need to modify fetch_project like that have here, or even the fact that we are using |
So I reintroduced the behavior but I did it differently taking into account your comment about not loving the way it was done: I refactored This way the new code calls I think it is lot cleaner (and from my manual testing, replaying the scenario I described earlier, it works great). If you agree with these changes, I can then update the ChangeLog and Documentation about the new concept of remote ports. I can also update the emdawn contrib port to a more recent version. @kainino0x is there a version of Dawn/emdawn that is more "stable" than others or should I just pick the latest (nightly) version? |
sbc100
left a comment
There was a problem hiding this comment.
Great! I like this better. LGTM % docs, although TBH we could land this now as an internal-only experiment (without public docs)
|
@sbc100 I am good with landing this internal-only. Once this is released, I can then also turn contrib.glfw as a remote port for another real-life test case and release it publicly with this change. |
Latest nightly v20250514.194204 should be the best one we have. It has the most bugfixes. |
|
@sbc100 @kainino0x Alright, so I just updated the PR with the recommended emdawn version ( As per our prior conversation, let's move forward with pushing this PR as-is (no docs / internal-only). Once this is merged, @kainino0x can deprecate Once the next version of Emscripten is released with these changes, I can then work on making |
I wonder if we can avoid asking folks to make actual releases for repos that just contains a single port (like |
|
I merged main and the tests reran. There are 2 failures (which were not present before the merge) which don't seem to be related to this PR. So I believe this is ready to be merged. |
|
@kainino0x I don't think it is a showstopper for releasing these changes, but version v20250514.194204 generates an issue with the closure compiler: |
Looks like a call to an undefined function. Likely a missing dependency. |
|
This is the snippet of code that triggers the error: // line 579 | library_webgpu.js
...
}
},
fillAdapterInfoStruct: (info, infoStruct) => {
{{{ gpu.makeCheckDescriptor('infoStruct') }}}
// Populate subgroup limits.
{{{ makeSetValue('infoStruct', C_STRUCTS.WGPUAdapterInfo.subgroupMinSize, 'info.subgroupMinSize', 'i32') }}};
{{{ makeSetValue('infoStruct', C_STRUCTS.WGPUAdapterInfo.subgroupMaxSize, 'info.subgroupMaxSize', 'i32') }}};
// Append all the strings together to condense into a single malloc.
var strs = info.vendor + info.architecture + info.device + info.description;
var strPtr = stringToNewUTF8(strs);
...it is definitely missing the |
|
Oops, that was a recent regression (from here). We haven't quite got automated testing up and running yet. Will fix. |
|
@kainino0x it's funny because it is my own CI that caught the problem :) |
|
Heh, any build is better than none! I just added a basic compile test but it's not running anywhere, I should add that to the github release so it does a last check before releasing. Should be fixed today with https://dawn-review.googlesource.com/c/dawn/+/242734. Thanks for the report. |
The deps need to be on the fillAdapterInfoStruct helper rather than the functions that use it, I think because it's positioned like a public API so Emscripten assumes it can't eliminate it (but still assumes it can eliminate stringToNewUTF8). It's not an immediate code size problem because Closure still eliminates it if it's unused, but Closure needs the code to be valid to do that. Tested this fixes a local release+closure build. Also updated the externs for these; I have not verified that change (but it's safe, it can't break anything). No-Try: true Bug: emscripten-core/emscripten#24303 (comment) Change-Id: Ib718f37c2dccf18374a115c9f12192685da55e46 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/242734 Commit-Queue: Loko Kung <lokokung@google.com> Auto-Submit: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Loko Kung <lokokung@google.com>
|
Fixed release here. https://github.com/google/dawn/releases/tag/v20250516.124039 |
There was a problem hiding this comment.
I would prefer to call this emdawnwebgpu for consistency with the project name (the reason it has that weird name is to make it very easily keyword searchable)
The tests are passing minus one which seems to be irrelevant |
|
I merged with main yesterday and there was no test failures. I merged again this morning and there are 2 test failures which are unrelated to these changes: @sbc100 Is there anything else that I need to do for this PR? Just as an FYI, I will be traveling in a couple of days so I will be less available for changes. So please let me know if there is anything that need to be changed. |
|
Both of those failures are known issues. The former has now been fixed and the later is just a flake. |
|
|
||
| @requires_network | ||
| def test_remote_ports(self): | ||
| self.emcc(test_file('hello_world.c'), ['--use-port=contrib.emdawnwebgpu']) |
There was a problem hiding this comment.
@kainino0x the name emdawnwebgpu seems rather long and redundant to me.
How about simple emdawn or even just dawn. The em prefix and the webgpu suffix both seem redundant in this context but maybe I'm missing something?
In any case we can land this and bikeshed later.
There was a problem hiding this comment.
FWIW, I agree 100% with @sbc100. I had named it originally dawn because it is the name of the project and it is following the pattern of all the other ports (sdl2, glfw3, etc... which are not named emsdl2, emglfw3). I only renamed it because @kainino0x asked me to.
There was a problem hiding this comment.
Emdawnwebgpu is the name of the project, it's used for the package releases and in the Dawn source tree. I chose it to be easily searchable and unambiguous (from USE_WEBGPU, any other potential/future ways of using Dawn in Wasm, Tint in Wasm, etc.)
There was a problem hiding this comment.
Fair enough!
Also, we could drip the contrib. here if you like since we do care internally about maintain this port if good working order. When we could just have --use-port=emdawnwebgpu
There was a problem hiding this comment.
If the distinction is how much we maintain it, then making it a non-contrib port makes sense to me. Will probably roll that change into #24220.
There was a problem hiding this comment.
Yeah, kind of. Its not super well defined. Basically I wanted to allow folks to create new ports in emscripten without adding more work for emscripten developers to make sure they keep working in all out different build configurations. I think we webgpu is key enough that we should consider it core and make sure it works over time.
That said, we should at some point add a few more emscripten-side tests to ensure we don't break things on our end.
|
🎉 |
Thanks to emscripten-core/emscripten#24303 (which should be in the next Emscription release, 4.0.10(?)), users can use this new port format which just includes a pointer to a zip containing a port file. Generate and release a port file like the one used in Emscripten, which can (1) be used instead of extracting the zip and (2) be copied directly into Emscripten whenever we want to update its version of `tools/ports/emdawnwebgpu.py`. Bug: 371024051 Change-Id: I12ee39d68454d7e7dba56798864b39ade51862b0
Thanks to emscripten-core/emscripten#24303 (which should be in the next Emscription release, 4.0.10), users can use this new port format which just includes a pointer to a zip containing a port file. This change generates and releases a port file like the one used in Emscripten, which can (1) be used instead of extracting the zip and (2) be copied directly into Emscripten whenever we want to update its version of `tools/ports/emdawnwebgpu.py`. Bug: 371024051 Change-Id: I12ee39d68454d7e7dba56798864b39ade51862b0
Thanks to emscripten-core/emscripten#24303 (which should be in the next Emscription release, 4.0.10), users can use this new port format which just includes a pointer to a zip containing a port file. This change generates and releases a port file like the one used in Emscripten, which can (1) be used instead of extracting the zip and (2) be copied directly into Emscripten whenever we want to update its version of `tools/ports/emdawnwebgpu.py`. Bug: 371024051 Change-Id: I12ee39d68454d7e7dba56798864b39ade51862b0
Thanks to emscripten-core/emscripten#24303 (which should be in the next Emscription release, 4.0.10), users can use this new port format which just includes a pointer to a zip containing a port file. This change generates and releases a port file like the one used in Emscripten, which can (1) be used instead of extracting the zip and (2) be copied directly into Emscripten whenever we want to update its version of `tools/ports/emdawnwebgpu.py`. Our version of Emscripten is still 4.0.8, but people can still use this with 4.0.10+. No-Try: true Bug: 371024051 Bug: 415224043 Change-Id: I12ee39d68454d7e7dba56798864b39ade51862b0
Thanks to emscripten-core/emscripten#24303 (which should be in the next Emscription release, 4.0.10), users can use this new port format which just includes a pointer to a zip containing a port file. This change generates and releases a port file like the one used in Emscripten, which can (1) be used instead of extracting the zip and (2) be copied directly into Emscripten whenever we want to update its version of `tools/ports/emdawnwebgpu.py`. It includes a full copy of the package README as a docstring to make the port self-documenting. Our version of Emscripten is still 4.0.8, but people can use this with 4.0.10+. No-Try: true Bug: 371024051 Bug: 415224043 Change-Id: I12ee39d68454d7e7dba56798864b39ade51862b0
Thanks to emscripten-core/emscripten#24303 (which should be in the next Emscription release, 4.0.10), users can use this new port format which just includes a pointer to a zip containing a port file. This change generates and releases a port file like the one used in Emscripten, which can (1) be used instead of extracting the zip and (2) be copied directly into Emscripten whenever we want to update its version of `tools/ports/emdawnwebgpu.py`. It includes a full copy of the package README as a docstring to make the port self-documenting. Our version of Emscripten is still 4.0.8, but people can use this with 4.0.10+. No-Try: true Bug: 371024051 Bug: 415224043 Change-Id: I12ee39d68454d7e7dba56798864b39ade51862b0 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/243214 Auto-Submit: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Loko Kung <lokokung@google.com> Commit-Queue: Kai Ninomiya <kainino@chromium.org>
The logic that this PR implements is the following: * the external port is loaded like the other ports and "flagged" as being external (`port.is_external = hasattr(port, 'EXTERNAL_PORT'`) * if the port is never needed, then nothing else happens * if the port is needed (because the user wants to use it via `--use-port=contrib.emdawn`, or it is a dependency of another port), then the code fetches the artifact and uses the port file included in the artifact to load the actual port file as if it was a local port, and replaces it in the ports array and `ports_by_name` map. => as a result, all calls using `ports_by_name` have been changed to call `get_port_by_name` to detect whether the port is remote or not and act appropriately An external port looks like this: it only contains info about where the port is located, the actual logic is coming from `PORT_FILE` ``` TAG = 'v20250509.171557' EXTERNAL_PORT = f'https://github.com/google/dawn/releases/download/{TAG}/emdawnwebgpu_pkg-{TAG}.zip' SHA512 = '4b66bf0f64b9616a6420abdad636b3ecefab892dde8f67cd941147bfddf7920f5523ff10160c9a563ef377a0f88b2dfc033527591b2d0753d531de5cbbabde59' PORT_FILE = 'emdawnwebgpu_pkg/emdawnwebgpu.port.py' # contrib port information (required) URL = 'https://dawn.googlesource.com/dawn' DESCRIPTION = 'Dawn is an open-source and cross-platform implementation of the WebGPU standard' LICENSE = 'BSD 3-Clause License' ```
This PR is implementing #24260.
The logic that this PR implements is the following:
port.is_external = hasattr(port, 'EXTERNAL_PORT')--use-port=contrib.emdawn, or it is a dependency of another port), then the code fetches the artifact and uses the port file included in the artifact to load the actual port file as if it was a local port, and replaces it in the ports array andports_by_namemap.=> as a result, all calls using
ports_by_namehave been changed to callget_port_by_nameto detect whether the port is remote or not and act appropriatelyAn external port looks like this: it only contains info about where the port is located, the actual logic is coming from
PORT_FILE