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

Consider build CLI (e.g. cargo gpu) mapping spirv-std deps to toolchain & backend versions. #1137

Open
eddyb opened this issue Feb 27, 2024 · 6 comments

Comments

@eddyb
Copy link
Contributor

eddyb commented Feb 27, 2024

I was looking at minimal shader Cargo.tomls and they're often mainly:

[dependencies]
spirv-std = "0.9.0"

In the past (e.g. #911 (comment)) any speculation was focused on the backend (rustc_codegen_spirv) being built in some way tied to whatever interface (e.g. spirv-builder, or some future CLI) was being used to actually build, and therefore their versions being tied.

For example we thought maybe we could have a way to "install Rust-GPU 0.123.0" but then you will have to install a new one every Rust-GPU release, and anyone who wants to interact with your shader crates has to do the same etc.

Also, as big as it is, the (still unmerged) PR I made on a whim a few months ago also has that limitation:


But what if we instead made a future-compatible tool (usable as build dep/CLI/etc.) that:

  1. used cargo_metadata to understand the shader's dependency graph (from Cargo.toml, workspace etc.)
    • in hot-reload mode, this would add more latency but at the same time we probably do want more visibility into stuff like that, and ideally it could be used in such a way that changing only the source code is much faster
  2. expects exactly one spirv-std dependency, and uses it to get the actual toolchain+backend
    • wrt spirv-builder: build rustc_codegen_spirv with the right toolchain via build script. #1103:
      • this is actually very similar to that PR, just spirv-std instead of spirv-builder
      • in theory, there's not much stopping us from having spirv-std's build script build the backend, or anything that silly, I would just be worried about making it too much a house of cards
    • an important consequence is that any path/git rev/crates.io version dependency for spirv-std would work (just like the PR which does this for spirv-build)
      • this is important to avoid impeding testing! (unlike "install a Rust-GPU version" workflows)
  3. gets the toolchain it wants with rustup and builds the right version of rustc_codegen_spirv
    • (again this is the same as the PR that does all that in spirv-builder's build script)
    • another potential advantage is that dependening on how this is done, it could let it show progress for that slower part of the build (though if you were using build scripts you'd have the same problem again, I think?)
  4. expose e.g. Cargo subcommands, with the env vars set to get you a Rust-GPU build
    • this could potentially mean cargo gpu check / cargo gpu clippy! (tricky to expose right now)
  5. ???
  6. profit
    • more seriously, it would be nice if we could have a "safe hot-reload" interface eventually, where the build script "freezes" your shader interface and generates Rust types/impls so you can safely fill compatible buffers, and also generating the Rust code for a hot-reloading convenience that forces that "frozen interface" to keep matching (i.e. rebuild the app itself if you want to change the interface incompatibly)

I'm not sure if I've heard this kind of "dependency consequence inversion" being used anywhere - usually libraries might be driven by compiler versions, not the other way around. If anyone had suggested this for Rust-GPU in the past, I apologize for being unfamiliar with your game because all my other ideas until now have been mediocre compromises at bets.


EDIT1: forgot to mention an incidental source of inspiration - back around Rust-GPU 0.4 (which had to change how #[spirv(...)] works due to upstream rustc changes), we considered making spirv-std-macros turn #[spirv(foo)] into #[rust_gpu_0_9_0_git9d8095177::spirv_foo] (or some other kind of verbose encoding of a version) - and once you have that kind of "self-awareness" in spirv-std about the backend version it wants, a build tool can pull that out. (stricter checks around that would've saved me from some confusing build failures just earlier, which is probably why it was on my mind)


EDIT2: in theory, this is comparable to if Rust itself let you do:

[dependencies]
std = "1.79.0"

and have that act as a rust-toolchain.toml replacement. (likely unviable without replacing (parts of) rustup+cargo+rustc CLIs all at once with a combined rust one or something similarly extreme)

Actually, if we allow more "conceptual" dependencies, the comparison becomes

[dependencies]
rust = "1.79.0"
rust-gpu = "0.123.0"

And we do have the rust-gpu package on crates.io so that could be used instead of spirv-std to be more explicit (and then "namespaced dependencies" like rust-gpu/spirv-std, or really rust_gpu::spirv_std could exist? also the attribute could then not have to go into spirv-std etc.)

@LegNeato
Copy link
Contributor

Look who has cargo-gpu on crates.io...a couple steps ahead of you 😄

@LegNeato
Copy link
Contributor

LegNeato commented Mar 1, 2024

Food for thought, though I don't want this to be come like JS where every tool has its own config file, having a GPU.toml or whatever might be a better route.

@eddyb
Copy link
Contributor Author

eddyb commented Mar 1, 2024

See https://doc.rust-lang.org/cargo/reference/manifest.html#the-metadata-table

With [package.metadata.rust-gpu.*] we could configure anything beyond what we can with just dependencies.

And you have to keep in mind that we can't remove the spirv-std dependency being explicit for several reasons (e.g. users wanting to compile common code, or even the entire shader, as a regular dependency of host code, or me wanting to have access to the breadth of Cargo path/git dependency/override specifiers.

@charles-r-earp
Copy link
Contributor

krnl has krnlc, which includes rustc_codgen_spirv into the binary and at runtime copies it back to the target dir. I found I had to sym link the rust sys root libs as well. So that's a working example of a cli instead of a builder / build script.

It does something similar to what @eddyb describes with spirv-std, krnl has krnl-core which links to spirv-std, and krnlc looks for krnl-core via cargo_metadata, checks that the version is compatible, and copies that dependency into the generated Cargo.toml for the device crate. This ensures that host and device crates have identical krnl-core / krnl-macros versions. Macros are used to generate host / device bindings, so identical versions means that the private interface can be unstable.

One thing that is somewhat awkward is needing to install krnlc with a specific nightly version, so that it isn't as easy to update and it's more cumbersome to mix two different versions. The spirv-builder dependency means krnlc has to have its own workspace.

Considering that rust-gpu requires a specific nightly, I'm not sure it would make sense to have something like krnlc be installable on stable or even just nightly, but it could. So if rust-gpu handled installing the required nightly + components + rustc_codegen_spirv, that makes it easier for other tools like krnlc or naga to use it.

@schell
Copy link

schell commented Nov 11, 2024

So if rust-gpu handled installing the required nightly + components + rustc_codegen_spirv, that makes it easier for other tools like krnlc or naga to use it.

@charles-r-earp Just an FYI than the new cargo-gpu does almost all of this. You still need to install nightly and components before hand, but I'll fix that tomorrow.

I ran into this problem when running cargo-gpu in my CI, so a fix is important. I think using the cargo-gpu cache dir $CACHE_DIR/rust-gpu/{dep_prefix}+{nightly_channel} would be a good place to put these files. cargo-gpu can put the files in place and maybe we could pass a parameter to spirv-builder to tell it where to look, and if omitted it would fall back to the current behavior?

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

No branches or pull requests

5 participants
@schell @eddyb @LegNeato @charles-r-earp and others