Support for setting config in shader's Cargo.toml

[tombh]

Managed to find quite an elegant solution where serde_json is doing all the heavy lifting. We even have automatic type checking on the config set in the shader crate's Cargo.toml!

Values are set in the TOML section: [package.metadata.rust-gpu.*].
So now the priority, from least to most, is:

• Workspace metadata
• Shader crate's metadata
• CLI args

Fixes #15

Notable changes:

• All clap args for the build and install subcommands are now
  defined in the spirv-builder-cli crate. This makes passing all the
  config between cargo gpu and spirv-builder-cli trivial as serde
  now does all the heavy lifting of creating and reading the JSON
  arguments.
• There is no longer a toml subcommand. The shader crate's Cargo.tomls
  are always read and their configs used as the base for CLI args to
  override.

TODO:

• Reuse the get_cargo_toml_as_json() function to parse the
  shader crate's spirv-std dependency. This will simplify the code
  and be more reliable. See Use cargo metadata to parse spirv-std dep from shader crate #37
• cargo gpu show targets See Add cargo gpu show targets #36
• cargo gpu show capabilities
• cargo gpu show extensions
• I'm still a bit confused about the target and shader_target
  arguments, are they both needed?

[tombh]

I'll have a look at those failing tests tomorrow.

[tmvkrpxl0]

Neat, I actually have usecase for this. I need to enable some shader features.

[schell]

Great work!

I'm still a bit confused about the target and shader_target
arguments, are they both needed?

They don't have to both exist, but they have to boil down to the same thing (if I'm not mistaken) - we have to be able to determine the target specification file to give to SpirvBuilder. I guess it's difficult because we want to present all of SpirvBuilder's options, but it has two methods of specifying the target specification - the old way of taking the spirv spec (eg spirv-unknown-vulkan1.2) and the new way where we give it a path to a specification file explicitly.

That said, I don't think we'll ever use anything but the explicit path-to-specification-file method.

[schell]

Neat, I actually have usecase for this. I need to enable some shader features.

Yes! This is why we're prioritizing this work :)

[tombh]

Thanks!

So let's see if we can come to a conclusion about the old/new way of defining the target.

• The old way is just about providing a target name that matches an existing target JSON file in the rust-gpu repo. The downside to that is that it limits the user, they can't provide their own target.
• The new way allows specifying an arbitrary path. However, I notice that we still unconditionally do target_spec_dir()?.join(format!("{}.json", self.build.shader_target)), which means that an end user still can't provide their own absolute path. So it's currently effectively the same as the old way?

Ideally we want a little automation? For example something like, if the user doesn't provide an absolute path then we assume that the string is the name of an existing target that rust-gpu already has. And if the user provides a path then we just use the file that it points to. I could still be misunderstanding something though?

Either way I think we'll want something like cargo gpu show targets that lists the available known target names?

BTW I've also just pushed a little change that attempts to end-to-end test the changes by altering the output_dir = line in the shader-crate-template's Cargo.toml. But I think it's struggling with sed not being cross platform. Here's the justfile snippet:

cargo install --path crates/cargo-gpu
cargo gpu install --shader-crate crates/shader-crate-template --auto-install-rust-toolchain
sed -i 's/^output-dir =.*/output-dir = \/tmp\/testshaders/' crates/shader-crate-template/Cargo.toml
cargo gpu build --shader-crate crates/shader-crate-template
ls -lah /tmp/test-shaders

It seems a bit hacky. Can anybody think of another way? Like is there another config that we can change and detect in the final built shader?

Edit: the end-to-test is actually surfacing a bug! I think it's to do with - and _ in argument names 🤔

[schell]

About defining a target - the new way is similar to the old way, but we control the paths at shader crate compile time, as opposed to them being hard coded at SpirvBuilder compile time to point to the target dir of the crate being built. So - yes, you're right that the user cannot supply their own target specification file (at least not without placing a new one into the blessed path in the cache directory).

But I think it would be easy enough to create one more option, something like --target-specification-file that would take precedence, and if it's omitted we construct the path from the target string and the target_spec_dir. That would cover all our bases, even though I think a user-supplied spec file will be very rare.

[schell]

Either way I think we'll want something like cargo gpu show targets that lists the available known target names?

I think that's a fine idea, I'll create an issue for that and we can tackle it after this?

[tombh]

Okay, for now I've just hidden the --target argument (because SpirvBuilder::new() still needs a target string, maybe it can just be "" for now?), so then we just have the --shader-target argument. And we can add the --target-specification-file another time. Though spirv-builder::SpirvBuilder still has both the old and new "*target" arguments, but that's another repo, so maybe that should be discussed up there.

From my side this looks ready to merge, or at least ready to give a good testing to. I still want to add the cargo gpu show capabilities/extensions commands. But they don't strictly need to be included in this PR.

[tombh]

I added the cargo gpu show capabilities command. I ended up just copy-pasting the spirv::Capability enum. It seems the only way to serialise an enum is with the strum crate's EnumIter derive macro, and that can't be added to external types. We could send a PR to https://github.com/gfx-rs/rspirv but it seems like it's not an active repo.

I also looked into a cargo gpu show extensions command, but there's no machine-readable source. So I think it's okay to just include the https://github.com/KhronosGroup/SPIRV-Registry link in the CLI help output.

[schell]

@tombh could we write a small wrapper over the enum just to provide our own (de)serialization? Would that be easier?

[tombh]

I'd love to! I'm not sure how though, do you have any ideas? spirv::Capabiltiies (the original type in the external crate) has #[derive(serde::serialize)] on it, so I feel like there should be a way. But I think the reality is just that serde can only serialize/deserialize specific values of an enum, not the the whole enum itself.

Another thing I tried was to type alias, making the external type internal, so type MyCapability = serde::Capability, which does work, but then derives can't be added to type aliases, namely, we need: #[derive(strum_macros::EnumIter)].

I'm very open to trying other ideas 🤓

[tombh]

Found another bug and pushed the fix. Copypasting the new commit's message:

Feature-gate `gfx-rs/rspirv` dependency

Because our `spirv-builder-cli` can modify itself to comply with various
versions of `spirv-builder` from `rust-gpu`, then we also need to match
the version of `spirv` that we use to get `spirv::Capabilities`.

Namely that `rust-gpu <= v0.9.0` uses `spirv = "0.2.0"` and that
`rust-gpu >= "v0.9.0"` uses `spirv = "0.3.0".

Another solution to this could be to re-export `spirv` from `spirv-builder`.
But I'm not sure that would avoid compiling the entirety of `spirv-builder`
(which depends on specific Rust nightly versions), and besides, it's not
trivial to retroactively add re-exports to already-published version of
`rust-gpu`.

[schell]

I'll take a crack at the spirv::Capabiltiies (de)serialization problem.

[schell]

I see what you mean - it's not that values need serialization, we just need to enumerate all values. This is a much easier problem to solve, so long as you don't think too hard ;) How about something like this?

/// Iterator over all `Capability` variants.
pub fn capability_variants_iter() -> impl Iterator<Item = spirv::Capability> {
    // Since spirv::Capability is repr(u32) we can iterate over
    // u32s until some maximum
    #[expect(clippy::as_conversions, reason = "We know all variants are repr(u32)")]
    let last_capability = spirv::Capability::CacheControlsINTEL as u32;
    (0..=last_capability).filter_map(spirv::Capability::from_u32)
}

[schell]

...and if we really need to be complete you could iterate (0..u32::MAX), but I think that's overkill.

[tombh]

Ohhhhh, that's really good 😏 Thank you!

Pushed ✨

[schell]

Is this all ready to go now?

[tombh]

From my perspective yes. There's a new bug I've found from working on #39, though it only happens in certain cases of changing the spirv-std version in the shader crate's Cargo.toml. I've almost got a fix, but I was thinking I'd just make a separate PR for it because it's part of the work to get #39 done.

[LegNeato]

There is also https://docs.rs/strum/latest/strum/derive.EnumIter.html. We use strum in the rust-gpu examples.

[tombh]

Yes, that's what I was using, but @schell found a much simpler method 🤓

[schell]

Wow! This is amazing work. Great job 👍 .

I just had a nitpick or two, but it's no big deal. Feel free to merge when you're ready!

[schell]

Wow! This is a really helpful function that I didn't know existed! Thanks! 🙇

[schell]

👍 good catch.

[schell]

It probably should default to the root of the shader crate, but we can keep that for later.

[schell]

Is this still a TODO?

[tombh]

Yes, we have an issue for it #36

[schell]

Clever use of features and deps! I didn't know you could do this.

[tombh]

Great, this is ready then. I'd merge it but I don't have push permissions.