Skip to content

update to naga v0.30 and fix output file mismatch - #655

Open
Firestar99 wants to merge 3 commits into
mainfrom
naga-30
Open

Firestar99 wants to merge 3 commits into
mainfrom
naga-30

Conversation

@Firestar99

Copy link
Copy Markdown
Member
  • update to naga v0.30
  • make output file name match with what SpirvBuilder reports

I've been using this workaround for a while but never fixed it upstream:

    let wgsl_result = builder.build()?;
    let path_to_spv = wgsl_result.module.unwrap_single();

    // needs to be fixed upstream
    let path_to_wgsl = path_to_spv.with_extension("wgsl");

Merging this PR will break this workaround, as the file will be called *.spv but contain wgsl source code. Naming it *.wgsl is possible, but would require SpirvBuilder to be aware what targets rust-gpu has, which we specifically wanted to avoid when I refactored our target definitions.

@Firestar99
Firestar99 marked this pull request as ready for review October 1, 2026 16:59

@nazar-pc nazar-pc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

require SpirvBuilder to be aware what targets rust-gpu has, which we specifically wanted to avoid when I refactored our target definitions

dll-suffix/exe-suffix/staticlib-suffix fields of the target specification are designed for this purpose, are they not usable for this purpose?

@Firestar99

Copy link
Copy Markdown
Member Author

Sure, I could easily add a formatting template here at the "dll-suffix" property. Rather, the question is whether Spirvbuilder should be aware of what targets exist. In the past, I've explicitly decided against that:

/// The constructors only check whether the target is well-formed, not whether it is valid. Since spirv-builder is
/// backwards compatible with older rust-gpu compilers, only the compiler itself knows what targets it can and cannot
/// support. This also allows adding new targets to the compiler without having to update spirv-builder and
/// cargo-gpu.

Target parsing happens here in the backend, but we need the target spec before invoking the backend.

The question is whether I want to break that guarantee for some well-known targets. For the benefit that some build artifacts are now called *.wgsl, and let's be honest, how many people have actually inspected the file name of those artifacts and didn't just forward whatever paths SpirvBuilder returned?

@Firestar99
Firestar99 marked this pull request as draft October 2, 2026 09:16
@nazar-pc

nazar-pc commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

When I call backend.to_spirv_builder(shader_crate, "spirv-unknown-vulkan1.2") it kind of already knows the target, but I guess it simply treats it as an opaque string to be passed to the backend and asks backend to write output to a predetermined path, which is why it needs to know it before the backend? Can't backend return the file path to the builder instead of builder creating it on its own?

As for backwards compatibility, I'm not sure how valuable that is in practice and how it is supposed to be used, and how much testing goes into it in practice. I kind of assume one should use a compatible version of the builder and compiler, ideally from the same exact revision, but that is just me.

@Firestar99

Copy link
Copy Markdown
Member Author

Can't backend return the file path to the builder instead of builder creating it on its own?

In theory yes, in practice I don't know if rustc does any sort of special checks on file paths that may be influenced by us writing in a different location.

@Firestar99
Firestar99 marked this pull request as ready for review October 2, 2026 15:26

This branch has not been deployed

No deployments
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