refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

Firestar99 · 2025-04-29T15:25:52Z

closes #55
closes #68
closes Rust-GPU/rust-gpu#208

The current version of cargo gpu roughly works like this, imagine it as stack frames:

cargo gpu
spawns spirv-builder-cli process
calls SpirvBuilder
spawns cargo process
spawns rustc process
loads rustc-codegen-spirv

And then we back-propagate the results:

rustc-codegen-spirv: writes spv files and json metadata of ShaderModules
Spirvbuilder: reads and parses json metadata and returns ShaderModules
spirv-builder-cli: converts rust types to version-independent ShaderModules and writes them to a json file
cargo gpu: reads json file, turns ShaderModules to Linkages (basically the same) and pretty writes a json metadata for the end user to parse

The core idea: Why not have cargo gpu directly call cargo with the required rustc-codegen-spirv flags attached, like Spirvbuilder does currently? And remove the complicated setup of spawning a new spirv-builder-cli process to call Spirvbuilder and parsing it all back out?

The key observation is that SpirvBuilder doesn't care at all what toolchain you use to build it, only it's dependency rustc_codegen_spirv has to be built by a specific toolchain. In Rust-GPU/rust-gpu#245 I make this dependency optional and hide it behind the feature rustc_codegen_spirv, which allows us to directly depend on SpirvBuilder and still build on stable. We just need to supply our SpirvBuilder with a librustc_codegen_spirv.so or rustc_codegen_spirv.dll prebuilt using the specific toolchain it requires, which significantly simplifies the build process.

This requires SpirvBuilder to stay be backwards compatible for all future versions, and we need to add support for past versions.

User-facing changes

Renames:
- capability -> capabilities
- extension -> extensions
- shader-target -> target
- debug -> release (clap: remains --debug)
- no_default_features -> default_features (clap: remains --no-default-features)
- removed rust_toolchain (as you can't use a different toolchain than the one required anyway)
proper path support for rust-gpu
- will build rustc_codegen_spirv in that local path, no need to compile twice
- will always try to rebuild
faster install: removed slow rust-gpu repo cloning, instead reuse cargo's cache
faster build: fewer cmds need to be launched or things parsed

Internal changes, including to `SpirvBuilder`

removed spirv_builder_cli crate
split mod spirv_cli into spirv_source, install_toolchain and lockfile
moved most BuildArgs members to SpirvBuilder
- made all fields pub on SpirvBuilder, replacing setters (but keeping them around)
- made SpirvBuilder derive serde::Serialize, serde::Deserialize
- moved clap macro args to SpirvBuilder (masked by clap feature)
- BuildArgs has a new SpirvBuilder member with clap(flatten) and serde(flatten)
- required some renames, see above
replaced cargo tree calls with cargo_metadata crate, cherry-picked from Switch to cargo metadata #66 by @tmvkrpxl0, required to "retrieve path to manifest" during install
new install procedure:

  * check spirv-std version
- * clone rust-gpu repo at revision (slow)
- * check `toolchain.toml` for toolchain
- * copy `spirv-builder-cli` to cache dir
+ * write blank `rustc_codegen_spirv_dummy` project
+   * only depends on `rustc_codegen_spirv`, version equal to `spirv-std`
+ * check metadata for project
+   * cargo creates `Cargo.lock`
+   * cargo clones repo into it's cache, if necessary
+   * retrieve path to manifest, either in crates cache or cloned repo cache
+ * check `build.rs` of `rustc_codegen_spirv` for toolchain version
+   * has a **string literal** with the exact same contents as the `toolchain.toml`
+ * delete `Cargo.lock` to prevent lockfile version issues
  * build dummy project with determined toolchain
  * retrieve `librustc_codegen_spirv.so` or `rustc_codegen_spirv.dll`
-   * retrieve `spirv-builder-cli` binary
  * copy target-specs jsons

new build procedure:

  * cargo gpu
- * spawns `spirv-builder-cli` process
+ * set `rustc_codegen_spirv_location` and `toolchain_overwrite` on SpirvBuilder
  * calls SpirvBuilder
(...)
  * SpirvBuilder returns
- * converts `CompileResult` into `ShaderModule`s, writes to disk
- * `spirv-builder-cli` process exits
- * load `ShaderModule`s from disk
  * convert to `Linkage`s
  * write `Linkage`s to file

Todo

Firestar99 · 2025-04-29T17:23:39Z

@schell @tombh cargo gpu install works! build still needs to be adjusted

Firestar99 · 2025-04-29T17:40:46Z

I would like to see the Switch to cargo metadata PR to be merged first (or integrated into here), as you can't just use it for more efficient version querying. It also tells you where cargo has checked out the repo / downloaded the crate, which we can use to retrieve the toolchain version we need without having to clone it ourselves.

Sample:

{
  "name": "rustc_codegen_spirv",
  "version": "0.9.0",
  "id": "git+https://github.com/Rust-GPU/rust-gpu?rev=82a0f69#[email protected]",
  "license": "MIT OR Apache-2.0",
  "license_file": null,
  "description": "SPIR-V code generator backend for rustc",
  "source": "git+https://github.com/Rust-GPU/rust-gpu?rev=82a0f69#82a0f69008414f51d59184763146caa6850ac588",
  ...
  "manifest_path": "/home/firestar99/.cargo/git/checkouts/rust-gpu-d06d15e2ba0f0ae2/82a0f69/crates/rustc_codegen_spirv/Cargo.toml",
  ...
},

… cargo gpu This is bandaid fix, it now ignores RUSTC environment variable set from elsewhere, like when shader crate is part of a workspace. As of today, it fails to compile on workspace which uses nightly because an additional restriction was added to target specification json file on nightly. Even though enforcing specific rustc version seems like good idea for the purpose of cargo-gpu, target specification json file should be updated as well if we were to ever bump up required rustc version.

tombh · 2025-04-30T14:03:21Z

This looks great! So let me see if I have a rough idea of what the change is here. In short, we're replacing spirv-builder-cli with rustc_codegen_spirv_dummy (which in turns builds the custom SPIRV codegen backend for rustc)? The main benefit of which is that we can now interact directly with rust-gpu, significantly simplifying and stabilising cargo-gpu?

tombh · 2025-04-30T14:09:13Z

I think this PR would also fix this Rust-GPU/rust-gpu#208

Firestar99 · 2025-04-30T16:20:06Z

Ignore rustc_codegen_spirv_dummy that only exists to depend on rustc_codegen_spirv and have cargo build it for us. I'll try to write a better summary in the description of all the changes.

tombh · 2025-04-30T20:50:33Z

Wow, great write up, thank you. This is actually awesome, there's so many improvements, it's going to make a big difference.

schell · 2025-04-30T21:52:24Z

I'm looking forward to this work landing @Firestar99, thanks!

Firestar99 · 2025-05-01T10:34:13Z

c1xx: fatal error C1081: 'C:\Users<user>\AppData\Local\rust-gpu\rustc_backend_spirv_install\https___github_com_Rust-GPU_rust-gpu+eca83dfbfc0aae5c72f1e3f53081f5ccb72c27a4\target\release\build\spirv-tools-sys-0a716808b3e410a8\out\942b8c9adc6a33b2-fold_spec_constant_op_and_composite_pass.o': file name too long

Oh f*ck off windows

Firestar99 · 2025-05-01T13:33:14Z

Watch works on Linux, but doesn't work on windows due to rust-gpu's depfile parsing being broken, see the missing \ in the key file path, first line?

\?\C:\Users\user\workspace\frameworks\cargo-gpu\crates\shader-crate-template\target\spirv-unknown-vulkan1.2\release\rust_gpu_shader_crate_template.spv.json: ["\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\src\\lib.rs", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\release\\deps\\spirv_std_macros-fb14afbeddf119cb.dll", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libbitflags-9fbb05a69a74b591.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libbitflags-9fbb05a69a74b591.rmeta", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libcompiler_builtins-30c147eaedb4971d.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libcore-8ebebbd1746b7c70.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libglam-5df165d896175a1d.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\liblibm-de1e5ee44605b253.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\liblibm-de1e5ee44605b253.rmeta", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libnum_traits-325c0de23b9ff9b5.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libnum_traits-325c0de23b9ff9b5.rmeta", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\librustc_std_workspace_core-9fb42a86c2d80d14.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\librustc_std_workspace_core-9fb42a86c2d80d14.rmeta", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libspirv_std-bd942fd01083ffa9.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libspirv_std_types-ea31211670fb7009.rlib", "\\?\\C:\\Users\\user\\workspace\\frameworks\\cargo-gpu\\crates\\shader-crate-template\\target\\spirv-unknown-vulkan1.2\\release\\deps\\libspirv_std_types-ea31211670fb7009.rmeta", "C:\\Users\\user\\AppData\\Local\\rust-gpu\\codegen\\https___github_com_Rust-GPU_rust-gpu+82a0f690\\rustc_codegen_spirv.dll"]
recurse:
\\?\C:\Users\user\workspace\frameworks\cargo-gpu\crates\shader-crate-template\target\spirv-unknown-vulkan1.2\release\rust_gpu_shader_crate_template.spv.json

Has it ever worked before my refactor? Had other difficulties testing that.

Firestar99 · 2025-05-01T13:47:19Z

@tombh @schell @LegNeato I think this is ready for review. The merge procedure will be to first merge Rust-GPU/rust-gpu#245, then update this PR to point to rust-gpu's master and merge it as well (which is why I won't yet mark this PR as ready). I think the remaining todos can be done in followup PRs.

Please don't squash these commits on either PR, they change so much it would probably be better to keep them around.

tombh

Everything looks great. Just a couple of minor suggestions.

crates/cargo-gpu/src/args.rs

crates/cargo-gpu/src/linkage.rs

crates/cargo-gpu/src/metadata.rs

crates/cargo-gpu/src/lockfile.rs

Firestar99 · 2025-05-06T16:23:30Z

@tombh feel free to rereview and merge, but I got one followup question:

cargo-gpu/crates/xtask/src/main.rs

Lines 154 to 188 in 6408188

    
           log::info!("installing cargo gpu artifacts"); 
        
           cmd([ 
        
               "cargo", 
        
               "gpu", 
        
               "install", 
        
               "--shader-crate", 
        
               SHADER_CRATE_PATH, 
        
               "--auto-install-rust-toolchain", 
        
               "--force-overwrite-lockfiles-v4-to-v3", 
        
           ]) 
        
           .unwrap(); 
        
           let dir = tempdir::TempDir::new("test-shader-output").unwrap(); 
        
           let mut overwriter = ShaderCrateTemplateCargoTomlWriter::new(); 
        
           overwriter.replace_output_dir(dir.path()).unwrap(); 
        
           if let Some(rust_gpu_version) = maybe_rust_gpu_version { 
        
               if rust_gpu_version != "latest" { 
        
                   overwriter 
        
                       .replace_spirv_std_version(rust_gpu_version) 
        
                       .unwrap(); 
        
               } 
        
           } 
        
           cmd([ 
        
               "cargo", 
        
               "gpu", 
        
               "build", 
        
               "--shader-crate", 
        
               SHADER_CRATE_PATH, 
        
               "--auto-install-rust-toolchain", 
        
               "--force-spirv-cli-rebuild", 
        
               "--force-overwrite-lockfiles-v4-to-v3", 
        
           ]) 
        
           .unwrap();

Why are we installing one rust-gpu version (whatever is in the repo, currently a rev), then replace the version with what was specified in the cmdline and when building, reinstall a different rust-gpu version? I was wondering why our 0.9.0 and 0.8.0 builds were taking twice as much as the latest does. Any specific reason it has been done like that, or should we unify to testing only one version?

tombh · 2025-05-07T00:14:12Z

Wow, that doesn't seem right. I can't think of any reason why that might be. I wonder if @schell does? I think as long as the tests pass then just a single cargo build ... should be enough?

tmvkrpxl0 · 2025-05-07T00:30:24Z

I like how we are discovering what it does along the way

Firestar99 added 3 commits April 29, 2025 17:33

spirv_source minor cleanup

cb1d8b1

gitignore: .idea

ca952e1

cargo gpu: remove ignored profiles emitting warnings

32c6d62

Firestar99 force-pushed the total_refactor branch from ba36f53 to 32c6d62 Compare April 29, 2025 15:33

Firestar99 added 7 commits April 29, 2025 17:34

remove spirv-builder-cli, leave install broken

21bddf5

minor cleanup

4d140a0

adjust install

859af40

fix clap and serde errors

a524212

working install, build broken

897531a

adjust tests to changed names

7f6552f

some clippy fixes

f5660df

Firestar99 mentioned this pull request Apr 29, 2025

Switch to cargo metadata #66

Open

tmvkrpxl0 and others added 9 commits April 30, 2025 00:44

replace cargo tree with cargo metadata

021073e

fix local path spirv-std dependency

4f5b9cb

removed repo cloning, grab toolchain from cargo's checkout

eacfbc8

reactive slow test, no longer clones a repo

8df2452

port mod metadata to cargo_metadata

2a74f92

split apart spirv_cli into spirv_source, install_toolchain and lockfile

dda18d9

support for local paths, also always build it there instead of our cache

6fef1fd

support for old rust-gpu versions on crates.io

8b5ea07

Firestar99 force-pushed the total_refactor branch from b826762 to f02f61c Compare April 30, 2025 13:28

Firestar99 added 3 commits April 30, 2025 19:50

adjust build action to use SpirvBuilder

37512d9

don't remove Cargo.lock for local paths

60fb2e5

use target-specs from spirv-builder

595dd1b

remove unused dependencies

fc31784

Firestar99 force-pushed the total_refactor branch from 629a4f9 to 7ec968a Compare April 30, 2025 21:10

Firestar99 added 2 commits April 30, 2025 23:35

old toolchain support

3f65b3c

fix clippy

b401f77

Firestar99 force-pushed the total_refactor branch from 9239d3c to b401f77 Compare April 30, 2025 21:35

Firestar99 mentioned this pull request Apr 30, 2025

spirv_builder feature compile_codegen to disable building rustc_codegen_spirv Rust-GPU/rust-gpu#245

Merged

Firestar99 force-pushed the total_refactor branch 2 times, most recently from 4920a39 to 4aebc9a Compare May 1, 2025 10:52

shorten build dir for windows

5290d92

Firestar99 force-pushed the total_refactor branch from 4aebc9a to 5290d92 Compare May 1, 2025 10:53

watch support, broken on windows

5e26615

Firestar99 force-pushed the total_refactor branch from 13e976a to 5e26615 Compare May 1, 2025 13:28

Firestar99 requested review from schell and tombh and removed request for schell May 1, 2025 13:47

tombh requested changes May 2, 2025

View reviewed changes

Firestar99 added 4 commits May 6, 2025 12:42

use master rust-gpu

82468e4

apply suggestions from @tombh

2c333e7

rename mentions of --capability to --capabilities

b06ee19

update readme's dump-usage

363b6ec

Firestar99 marked this pull request as ready for review May 6, 2025 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

Firestar99 commented Apr 29, 2025 •

edited

Loading

Firestar99 commented Apr 29, 2025

Firestar99 commented Apr 29, 2025

tombh commented Apr 30, 2025

tombh commented Apr 30, 2025

Firestar99 commented Apr 30, 2025

tombh commented Apr 30, 2025

schell commented Apr 30, 2025

Firestar99 commented May 1, 2025

Firestar99 commented May 1, 2025 •

edited

Loading

Firestar99 commented May 1, 2025 •

edited

Loading

tombh left a comment

Firestar99 commented May 6, 2025 •

edited

Loading

tombh commented May 7, 2025

tmvkrpxl0 commented May 7, 2025

refactor: use SpirvBuilder directly, just manage rustc_backend_spirv dylibs #69

Are you sure you want to change the base?

refactor: use SpirvBuilder directly, just manage rustc_backend_spirv dylibs #69

Conversation

Firestar99 commented Apr 29, 2025 • edited Loading

User-facing changes

Internal changes, including to SpirvBuilder

Todo

Firestar99 commented Apr 29, 2025

Firestar99 commented Apr 29, 2025

tombh commented Apr 30, 2025

tombh commented Apr 30, 2025

Firestar99 commented Apr 30, 2025

tombh commented Apr 30, 2025

schell commented Apr 30, 2025

Firestar99 commented May 1, 2025

Firestar99 commented May 1, 2025 • edited Loading

Firestar99 commented May 1, 2025 • edited Loading

tombh left a comment

Choose a reason for hiding this comment

Firestar99 commented May 6, 2025 • edited Loading

tombh commented May 7, 2025

tmvkrpxl0 commented May 7, 2025

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

refactor: use SpirvBuilder directly, just manage `rustc_backend_spirv` dylibs #69

Firestar99 commented Apr 29, 2025 •

edited

Loading

Internal changes, including to `SpirvBuilder`

Firestar99 commented May 1, 2025 •

edited

Loading

Firestar99 commented May 1, 2025 •

edited

Loading

Firestar99 commented May 6, 2025 •

edited

Loading