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

spirv-builder: build rustc_codegen_spirv with the right toolchain via build script. #1103

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Nov 27, 2023

This has been a long-time coming, I've mostly been putting it off because of how annoying it'd be to implement.
(turns out I was a bit wrong and there's so many more things that don't work, than the few that do)

For some background, also see previous discussion in:

Rough summary of the new approach:

  • unknown toolchain builds spirv-builder with no special features
    • spirv-builder's build script kicks in and creates a private temporary helper workspace, which:
      • depends on the exact spirv-builder being built (via path dependency, to wherever it is)
      • enables the internal-unstable-trigger-self-build-for-backend special feature, which in turn is what enables the rustc_codegen_spirv optional dependency
    • however, the rust-toolchain.toml (and Cargo.lock) that rustc_codegen_spirv needs are still unknown!
      • spirv-builder's build script uses cargo_metadata to find its own rustc_codegen_spirv dependency, from the perspective of the helper workspace that enables it via the special feature
        • (the reason for this indirection is a bit subtle - roughly speaking, Cargo pretends to not even know about non-workspace packages that are optional dependencies, unless something in the workspace causes them to be enabled - so the original workspace using spirv-builder is useless for this)
      • rust-toolchain.toml and Cargo.lock are copied from the rustc_codegen_spirv found with cargo_metadata
    • the correct toolchain is then asked to build that temporary helper workspace, causing a nested build of spirv-builder, including the rustc_codegen_spirv dylib, whose path can be embedded in the outer build
      • of note is that we control this build, and can always make it --release, incremental, etc.
  • the resulting (outer) spirv-builder will use information from its build script, to run the correct rustup toolchain, with the correct rustc_codegen_spirv dylib path, regardless of the outer toolchain/env vars/etc.

Benefits:

  • users never have to worry about rust-toolchain.toml or nested workspaces/helper binaries again
  • overriding profiles in Cargo.toml is no longer needed to ensure the codegen backend gets optimized
  • there's no duplicate build of the backend even if both a build script and hot reloading use spirv-builder
    • (in fact, I've been tempted to make the build script side mandatory for hot reloading, because it would allow us to enforce the shader interface, and disallow hot reloading when it'd make the types incompatible)
    • or at the very least we could have SpirvBuilder generate some APIs to conveniently rebuild all the same shaders (making the SPIR-V blobs almost like "self-updating values")

Downsides:

  • working on this repository gets weirder/more contrived
    • specifically around rustc_codegen_spirv, which now has its own nested workspace
  • users can accidentally cause more copies of rustc_codegen_spirv to be built than they'd want
  • users still get all the language/libcore limitations of the current Rust-GPU toolchain in their shader code, but without having a rust-toolchain.toml around to run cargo check on the host with the same toolchain
    • this might not be a huge deal, and we could add cargo +nightly-... check suggestions to errors etc.
  • spurious rebuilds can occur, until we sort out all the env vars and directories that such nested builds can be sensitive to

If you want to give this a go, remove rust-toolchain.toml (and anything you added to e.g. Cargo.toml for overriding profiles for build dependency, any weird workspace indirection executables etc.) and add:

[patch.crates-io]
spirv-builder = { git = "https://github.com/LykenSol/rust-gpu", branch = "auto-self-build" }

(assuming you're already depending on spirv-builder = "0.9")

Comment on lines +61 to +105
// Set up a workspace in `$OUT_DIR` to bootstrap building this very crate
// (`spirv-builder`) with `rust-toolchain.toml` and `Cargo.lock` taken from
// our `rustc_codegen_spirv` dependency.
let ws = out_dir.join("nested-self-builder");
fs::create_dir_all(ws.join("src")).unwrap();
fs::write(
ws.join("Cargo.toml"),
format!(
r#"
[workspace]

[package]
name = "nested-self-builder"
edition = "2021"
version = "0.0.0"

[dependencies.spirv-builder]
path = {spirv_builder_path:?}
default-features = false
features = [{spirv_builder_features}]

# Enable incremental by default in release mode, as well.
[profile.release]
incremental = true
"#,
spirv_builder_path = env!("CARGO_MANIFEST_DIR"),
spirv_builder_features = propagate_features
.iter()
.copied()
.chain(["internal-unstable-trigger-self-build-for-backend"])
.map(|feat| format!("{feat:?}"))
.collect::<Vec<_>>()
.join(", "),
),
)
.unwrap();
const NESTED_SELF_BUILDER_MAIN_RS: &str = r#"
fn main() {
let dylib_path = spirv_builder::codegen_backend::codegen_backend_dylib_path();
println!("cargo:rerun-if-changed={}", dylib_path.display());
println!("cargo:rustc-env=SPIRV_BUILDER_CODEGEN_BACKEND_DYLIB_PATH={}", dylib_path.display());
println!("cargo:rustc-env=SPIRV_BUILDER_RUSTUP_TOOLCHAIN={}", env!("RUSTUP_TOOLCHAIN"));
}
"#;
let ws_src_main_rs = ws.join("src/main.rs");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New idea (from #911 (comment)) to consider trying:

The diff I'm commenting on generates a pair of Cargo.toml and src/main.rs - it could be a -Z script combined .rs file, forcing Cargo to help out with caching.

That is, we could manage ~/.cache/rust-gpu (something respecting $XDG_CACHE_HOME in reality), under which we generate a -Z script .rs file per choice of contents (as in, we can content-address it, i.e. the files just take a name given by their own hash).

That directory of .rs files would be relatively miniscule, but Cargo should be able to e.g. automatically GC the target dirs it created itself (under ~/.cargo) to build -Z script packages.

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.

None yet

1 participant