Skip to content
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

Add cts_runner and deno_webgpu crate #1859

Merged
merged 13 commits into from
Sep 3, 2021
Merged

Conversation

lucacasonato
Copy link
Contributor

@lucacasonato lucacasonato commented Aug 24, 2021

This PR is a WIP.

@kvark and I discussed how wgpu could run tests against cts. CTS is built on the JS API. gfx-rs/wgpu only has a Rust API. One possible solution that was suggested is to use Deno's implementation of WebGPU JS API.

Deno implements the glue bindings between the WebGPU JS API, and the Rust wgpu-core.

The PR adds the deno_webgpu crate from denoland/deno, and a simple cts_runner runtime which only purpose is to run CTS tests.

To try ou the cts runner with a basic compute example:

cargo run -p cts_runner -- cts_runner/examples/hello-compute.js

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

My main concern here is the amount of duplicated code. If it was possible to auto-generate, say, texture formats from a single place, that would help quite a bit. I guess if it was possible, you'd already be doing something like this...

Another question is - what is Deno's plan here? Would you include the whole wgpu as a sub-module? That's quite a bit of code!

Finally, this marked as WIP. What is missing?

deno_webgpu/webgpu.idl Show resolved Hide resolved
deno_webgpu/texture.rs Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
cts_runner/Cargo.toml Show resolved Hide resolved
cts_runner/build.rs Outdated Show resolved Hide resolved
cts_runner/build.rs Outdated Show resolved Hide resolved
@kvark kvark mentioned this pull request Aug 25, 2021
@lucacasonato
Copy link
Contributor Author

lucacasonato commented Aug 30, 2021

My main concern here is the amount of duplicated code. If it was possible to auto-generate, say, texture formats from a single place, that would help quite a bit. I guess if it was possible, you'd already be doing something like this...

It is possible, and we are considering it. WebGPU has by far the largest API surface of the web APIs we implement (so many types!), so we are sorta pushing this into the future. At some point I want to properly auto generate much JS boilerplate from WebIDL. The same could probably be done for all the enums in wgpu_types.

Another question is - what is Deno's plan here? Would you include the whole wgpu as a sub-module? That's quite a bit of code!

So this gets a little complicated. We have two constraints that we are dealing with from the Deno side:

  • we publish the final deno binary target to crates.io - this requires that all dependencies are also published on crates.io
  • deno_core lives in the same repo as deno, and they are published in sync. deno_webgpu has a dependency on deno_core, but is a dependant of deno.

I propose we do the following:

  • all crates that already exist in this repo now, and are published to crates.io (wgpu_types, wgpu_core, wgpu) are published from this repo like is done now (nothing changes here)
  • deno_webgpu canonically lives in this repo, but it is not tagged or released here (more on this later)
  • deno_webgpu periodically updates to newer deno_core releases from crates.io to mostly stay up to date with the main deno tree
  • we vendor (submodule or otherwise) deno_webgpu into the deno tree
    • we float a patch keeping deno_core pinned to the in tree version
    • we use the version of wgpu_core and friends that is published to crates.io
    • we might also occasionally need to float patches to deal with changes in deno_core
  • deno_webgpu will be published to crates.io in sync with deno releases from the vendored crate living in the deno tree
  • we update the vendored deno_webgpu in the deno tree whenever a new release of wgpu_core is published

Finally, this marked as WIP. What is missing?

I want to make sure this implementation can run at least 1 CTS test correctly. Actual CTS infra can be set up in a follow up.

@kvark
Copy link
Member

kvark commented Aug 31, 2021

Looks to be ready now, thank you!
But let's not merge until we can talk over the long term plan her. ETA this Friday

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Results from talking about this on a call:

  1. the TS bindings can be moved out
  2. build script can be removed
  3. still want to get the first CTS test running before landing

cts_runner binaries are now not portable anymore.

Also startup will now print a bunch of cargo:rerun-if-changed=. This
will be fixed in deno_core.
@lucacasonato
Copy link
Contributor Author

And got CTS working (with these changes to cts: gpuweb/cts#734)

~/p/g/g/cts ❯❯❯ /Users/lucacasonato/projects/github.com/gfx-rs/wgpu/target/debug/cts_runner ./tools/run_deno 'unittests:*'
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/core/00_primordials.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/core/01_core.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/core/02_error.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/webidl/00_webidl.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/console/01_colors.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/console/02_console.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/url/00_url.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/00_infra.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/01_dom_exception.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/01_mimesniff.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/02_event.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/02_structured_clone.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/03_abort_signal.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/04_global_interfaces.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/05_base64.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/06_streams.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/08_text_encoding.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/09_file.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/10_filereader.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/11_blob_url.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/12_location.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/web/13_message_port.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/timers/01_timers.js
cargo:rerun-if-changed=/Users/lucacasonato/.cargo/git/checkouts/deno-22855de1c03c9128/ca75752/ext/timers/02_performance.js
cargo:rerun-if-changed=/Users/lucacasonato/projects/github.com/gfx-rs/wgpu/deno_webgpu/01_webgpu.js
cargo:rerun-if-changed=/Users/lucacasonato/projects/github.com/gfx-rs/wgpu/deno_webgpu/02_idl_types.js
cargo:rerun-if-changed=/Users/lucacasonato/projects/github.com/gfx-rs/wgpu/cts_runner/src/bootstrap.js

** Summary **
Passed  w/o warnings = 110 / 110 = 100.00%
Passed with warnings =   0 / 110 =   0.00%
Skipped              =   0 / 110 =   0.00%
Failed               =   0 / 110 =   0.00%

@lucacasonato
Copy link
Contributor Author

Due to the removal of build.rs, startup will now print a bunch of cargo:rerun-if-changed= lines. This is a bug in deno_core, which I'll get fixed soon. Just an aesthetic thing, so shouldn't prevent this PR from landing.

@kvark
Copy link
Member

kvark commented Sep 3, 2021

Ok, this is wonderful. Looks like we are all set now?

@lucacasonato
Copy link
Contributor Author

Yup, I think so!

@kvark kvark merged commit d5ba0b4 into gfx-rs:master Sep 3, 2021
@kvark kvark changed the title WIP: add cts_runner and deno_webgpu crate Add cts_runner and deno_webgpu crate Sep 3, 2021
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.

2 participants