Update shader bencher to share some logic with snapshots #8108
Conversation
There was a problem hiding this comment.
Hmmm this is a tough one. It's almost like we should have a new crate for naga tests, which wgpu-benchmark can depend on.
I'll bring this up at the next meeting as we had a whole discussion about the placement of naga test and I'm not sure how this new issue would change opinion.
naga/Cargo.toml
Outdated
| walkdir.workspace = true | ||
| wgpu-test.workspace = true |
There was a problem hiding this comment.
I'm pretty uncomfortable making this back edge - I feel like there's a better design for sharing the benchmarks. More specifically I'm not really sure why this backedge needs to be here at all if the tests have been moved to wgpu-test.
Ok. I probably won't be able to be there but I don't think I'd be of use in those discussions anyway. And its fine if this PR gets closed, I made these changes to stop a CI from failing which it already accomplished. |
Don't you still need it to land your changes?
We'll keep you in the loop, no worries |
I will need it for whenever mesh shader related tests get added. I'd really really like to get the tests in with WGSL parsing but if that has to come later its not my problem (and the testing has already been done on those changes). |
|
Alright, I'll see if we can muse up a better solution as we definitely want this problem to be solved when mesh shader wgsl-in lands. |
|
@cwfitzgerald I think this is ready for a review. I've tried to split it off into a separate naga-tests crate. The cargo/directory setup is somewhat confusing to me but I tried to mimic what wgpu-test does for the most part. |
| cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} --tests --features glsl,spirv,fragile-send-sync-non-atomic-wasm | ||
| cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} --tests --features glsl,spirv | ||
| cargo doc --target ${{ matrix.target }} ${{ matrix.extra-flags }} --no-deps --features glsl,spirv |
There was a problem hiding this comment.
Iirc the spirv feature isn't really a thing any more, its spv-in and spv-out, so I think somehow in my initial changes (pre naga-test crate) I tripped a CI failure. I tried running this command locally but without the --target part and it seems that it is still an issue locally but I'm not sure if it will still break the CI
There was a problem hiding this comment.
Yeah, it still is broken without this fix. See https://github.com/gfx-rs/wgpu/actions/runs/17475393186/job/49633939357?pr=8108
There was a problem hiding this comment.
Iirc the spirv feature isn't really a thing any more
This is a feature of wgpu, I don't know why it's complaining about naga, as it is a valid feature of wgpu...
There was a problem hiding this comment.
I'll see if I can take a look at this after work, this is weird.
There was a problem hiding this comment.
I was incredibly confused. I think I did some searching/reading of docs but I completely forget the conclusion unfortunately
|
Aside from the CI issues with features, should this PR be good to go? If so, can I leave that part to you? Lmao |
Connections
This is to address test failings in #7930 due to not respecting supported backends. In fact, its pulled directly from the last few commits there.
Description
Previously, the shader bencher was using snapshot shaders but not respecting their options, such as which backends they should be tested against. As a result, in #7930, a mesh shader test shader that only can be written to SPIRV was causing the shader bencher to fail. I have updated the shader bencher and snapshots to share more logic.
See this comment
Testing
The change is to testing itself. I have verified that the bencher still fails on bad shader inputs, and that it ignores my mesh shader as it is supposed to.
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.