Refactor how rustc_codegen_spirv is compiled

[watjurk]

Summary

rustc_codegen_spirv is compiled and discovered in a very hacky way. Cargo compiles rustc_codegen_spirv because it is inside spirv-builder's Cargo.toml as a dependency, and then spirv-builder discovers the rustc_codegen_spirv's dylib by this super hacky function: find_rustc_codegen_spirv. I think this should be avoided.

Motivation

When I've tried to build something using spirv-builder this hacky discovery was not working for me.
Also rustc_codegen_spirv is compiled using the same profile as spirv-builder and we end up with rustc_codegen_spirv being build without optimizations (if we are in debug mode). This is a big problem because compiling rustc_codegen_spirv is a one time cost, but while developing one would recompile thier shaders many times.
Proposed solution is much more elegant than current implemention.

Solution

Let's move rustc_codegen_spirv into another trait witch will compile rustc_codegen_spirv and provide path to it's dylib file.
I've crated an example of how this could be implements: https://github.com/watjurk/spirv-builder-alternative, here is a short summary:

All of this happens inside crates/rustc_codegen_spirv:

Cargo.toml:

[package]
name = "rustc_codegen_spirv_compiler"

src/lib.rs:

pub fn dylib_path() -> PathBuf {
// Compile rustc_codegen_spirv using cargo and return path to it's dylib file.
}

src/rustc_codegen_spirv

Folder with the `rustc_codegen_spirv` crate.

Future

In the future this approach could be modified without breaking code that relies on dylib_path function, for example we can serve pre-builed dylib's of rustc_codegen_spirv and dylib_path instead of building rustc_codegen_spirv will download one of the pre-build dylib's depending on user's hardware.

[eddyb]

I agree it's noticeably suboptimal right now, and we have been wondering for a while about alternatives.

However, there are more variations to what we could do (cc @repi @oisyn).

How should rustc_codegen_spirv be built?

(:stop_sign: used below to represent "downsides" aka "cons" in a easy-to-spot way)

• Dependency (current): by Cargo only because it's a dependency (of spirv-builder)
  🛑 copying rust-toolchain from Rust-GPU is mandatory (and your own code has to also be built with that nightly, if it depends on spirv-builder, not just the codegen backend dylib)
  🛑 unoptimized build by default, without changing Cargo.toml (esp. if spirv-builder is a build
  • dependency kind:
    • normal (current)
      🛑 if spirv-builder is used both as a normal and build dep itself, this will likely lead to multiple builds of rustc_codegen_spirv
    • build
      🛑 always has the unoptimized issue (see above)
• Spirv-builder: spirv-builder running cargo build -p rustc_codegen_spirv itself
  🛑 if spirv-builder is a build dependency, it's tricky (if even possible) to show build progress
  • when to run the build:
    • dynamic: when spirv_builder::... APIs are used
      (IIUC, @watjurk is suggesting specifically this choice)
      🛑 extra "is it fresh" checks every time
      🛑 may be inadvertently affected by e.g. environment variables
    • build: build script (spirv_builder::... refer to a path compiled-in via env!)
      🛑 exacerbates the progress issue (see above)
  • how to determine the version:
    • hardcoded: every time we publish we change spirv-builder source code
      🛑 manual work or scripting required (pretty messy)
    • same: get it from env!("CARGO_PKG_VERSION") of spirv-builder
      🛑 have to assume either https://github.com/EmbarkStudios/rust-gpu or crates.io
      🛑 can't work for specific git revisions (since they're not part of the version)
    • unbuilt-dep: make spirv-builder depend on rustc_codegen_spirv but with impossible conditions, or optional (could just make spirv-builder/build.rs error if enabled), so that Cargo fetches the dependency and we can find it through e.g. cargo metadata (or build it with -p etc.)
      🛑 may still pollute Cargo.lock, introduce versioning conflicts, etc.
    • bundled: have the packaging process archive all the relevant source code
      🛑 would skip relying on Cargo to fetch that source code, but it's still an additional step in a weird place, and so has all the problems of hardcoded and same
      • EDIT: to be clear, this meant a "manual archive" step that we would need a shell script for, and to do before every cargo publish - in Refactor how rustc_codegen_spirv is compiled #911 (comment) I finally understood that @watjurk had a much more interesting trick that "just" relies on putting the entirety of rustc_codegen_spirv inside the src/ of another Cargo package, that should get its own entry here
  • what toolchain to use:
    • rustup: we rely on rust-toolchain and/or explicit rustup commands to run the build
      🛑 not everyone might have rustup installed, they might be running the outermost build with some pinned version scheme of their own choice etc.
    • download: we basically pretend to be rustup and do the above
      🛑 pretty much a bad idea across the board (e.g. distros can provide rustup that works slightly better, or at all, and this would just break in those cases)
    • stable: we abuse RUSTC_BOOTSTRAP and support specific stable versions
      🛑 I doubt we could even get rustc-dev, and very likely not from non-rustup distro packages either - and RUSTC_BOOTSTRAP is terrible to mess with anyway, let's not...
  • where to cache the built backend dylib:
    • target-dir: we nest it in $OUT_DIR or at most go up a bit from it (could use a temporary build dir, like cargo install does, then move the resulting dylib to $OUT_DIR)
      🛑 built once per Cargo workspace, potentially even profile
    • per-toolchain: somewhere in .rustup/toolchains/$our_current_nightly/...
      🛑 we'd have to hash all the relevant version/source-path/etc. details to use as a "cache key", cache invalidation is hard, file locking, there's quota concerns, rustup might not like toolchain dirs being altered, etc.
    • per-user: somewhere in $XDG_CACHE_HOME (i.e. ~/.cache)
      🛑 same as per-toolchain above, except even less scoped (e.g. the files would be kept around even if the user removes the relevant rustup toolchain)
• Manually: separate "install Rust-GPU" step (e.g. into .rustup/toolchains/...)
  🛑 like spirv-build automatic per-toolchain/per-user caching, but requiring user interaction outside of Cargo builds
  • could be offered in conjunction with extra automation, to ensure spirv-builder doesn't itself also need to build the toolchain, but also allowing more direct usage?
  • this also ties into artifact dependencies which would likely always require a modified toolchain if we want to use them
  • (mostly included for completeness, it can't possibly be a full solution IMO, not without having a way to put something in rust-toolchain)

_{(I tried using a table and it feels like I would need 3D or 4D tables to make it work well... in the end I opted to list mostly cons and not so much pros - also, I gave each aspect names so that I can refer to them later but it didn't end up being useful)}

You may notice that I've even included "really bad ideas" in there (decided to use strikethrough to indicate them) - I really want to be clear about the size of the solution space, so that we don't make too many assumptions without realizing.

Even ignoring the unviable options, there's still 14 combinations (12 if we assume S).

---

If I had to pick one, S when=build version=unbuilt-dep toolchain=rustup cache=per-toolchain is probably a local optimum: it may upset Cargo and/or rustup, reporting progress may be annoying/hard/impossible, but it would ideally minimize configuration (use it directly from stable code or any toolchain!), maximize reuse between different build dirs, always have an optimized backend, be directly usable from runtime code for e.g. a hot reloading (without the hacks we need in-tree for all that), etc.

But hybrids are also interesting, e.g. version=same for published versions and something different when developing on Rust-GPU itself (to avoid storing a zillion unique dylibs, and also to enable incremental etc.).

Also, to be perfectly clear: I am not strongly attached to any one idea, all I want to is to present as much as possible so that we can choose knowing what we're deciding against. And it's also possible I missed things, I may edit this comment or try to figure out a better presentation for all the options (might be easier if we decide on some of them first).

[watjurk]

I think that this is not what I meant, I'd like to decouple the process of compiling rustc_codegen_spirv from spirv-builder, so that for spirv-builder obtaining the dylib is just a matter of calling something like this:

let path_to_dylib = rustc_codegen_spirv:: dylib_path();

If you think about it spirv-builder should not be responsible for building rustc_codegen_spirv, this logic should be contained inside rustc_codegen_spirv and. I'd opt for the bundled version, just like in my example the target_crate is contained within the rust_cargo_build_nested crate. In my opinion this option is the most correct one because then we get the version from the parent crate.
The unbuilt-dep poposition would still work in this scenario just that rust_cargo_build_nested would depend on the target_crate.

The next question is, from what perspective are we looking at it? For example from user's perspective who is running spirv-builder inside thier's build.rs script the compilation of rustc_codegen_spirv is really happening at compiletime.

[eddyb]

I think that this is not what I meant, I'd like to decouple the process of compiling rustc_codegen_spirv from spirv-builder

This is orthogonal from pretty much everything I described, at least AFAICT. You can imagine that (almost) every time I said spirv-builder I meant some new rustc_codegen_spirv-builder crate that spirv-builder depends on, and which provides the dylib_path method you're thinking of (though really it should be a constant), it doesn't change almost anything about the solution space matrix (at most it's just another axis).
_{(We really don't want to change what rustc_codegen_spirv is, i.e. it should remain the dylib that rustc loads when we pass -Z codegen-backend=..., you can use -... after its name to indicate some helper crate related to it, but not rename the crate itself)}

---

I'd opt for the bundled version

bundled (EDIT: in its "manual archive" form I imagined, see below!) is not really workable because we would need some kind of shell script to make a .tar.gz or .zip or something with the contents of crates/rustc_codegen_spirv and then require that on every cargo publish, and there would be no easy way to use a git dependency, and it's so unidiomatic and an ops nightmare that you can just ignore it (even if you can trick cargo publish into bundling it for you as "extra files" in the crate, it's still a lot of trouble for something very fragile).

just like in my example the target_crate is contained within the rust_cargo_build_nested crate

Oh that's in an src/ directory? So that would probably fit into "trick cargo publish into including it".
I'm still not extremely confident it won't break in some weird way but I can see why you brought it up now.

This probably means we need to split up bundled into two ("manual archive" vs "nested crates"). My earlier confusion stems from thinking "manual archive" when I wrote the bundled entry, and also taking a while to notice the precise way you have placed a whole Cargo package directory inside the src/ of another.

---

The next question is, from what perspective are we looking at it? For example from user's perspective who is running spirv-builder inside thier's build.rs script the compilation of rustc_codegen_spirv is really happening at compiletime.

"What needs to run every time you change the shaders" is how I see it (and this applies to both build scripts and hot reloading usecases) - there's no reason (IMO) to keep checking if we need to rebuild rustc_codegen_spirv every time the shaders change, if we can avoid it (and Cargo has various ways of minimizing the cost of checking if rebuilds are necessary, for the things that it is in control).

But that is a quite minor point compared to everything else, a microoptimization almost.

Generally, I'm trying to describe how spirv-builder (or rustc_codegen_spirv-builder, if you want) work internally, while still worrying about end-user UX like rust-toolchain/Cargo.toml configuration friction, how build progress is indicated, etc. (a bit like if an user of Rust-GPU had an "xray view" of the whole process).

[watjurk]

I'll point out some things:

1. rustc_codegen_spirv requires the following components to compile: ["rustc-dev", "llvm-tools-preview"], so in order to install them we are forced to use rustup, so as a consequence our users are forced to use rustup, so the issue of installing toolchain is gone because we must have rustup.
2. rustc_codegen_spirv should not have any features that user can enable/disable, I'd even argue that we should remove use-installed-tools and use-compiled-tools, for reasons I'll explain later.
3. rustc_codegen_spirv should be it's own fully self-contained crate that would not be influenced by user in any way.

I'd like to have all of this to prepare rustc_codegen_spirv for the Future. By the Future I mean that we should not require users to compile rustc_codegen_spirv, instead we should provide pre-build binaries that would be downloaded by dylib_path function. The overall interface would stay the same just that we would not compile rustc_codegen_spirv , we would download it. In order to do this seamlessly rustc_codegen_spirv cannot depend on user input in any way, that's why I think we should remove use-installed-tools and use-compiled-tools.
Lastly, when you download rust are you compiling rustc's backend? No, you are getting it pre-build, I think the same should happen in rust-gpu's case.

Ps: The "nested-creates" approach (solution proposed by me) would satisfy the 3rd point.

[eddyb]

Finally took a stab at something like this, in:

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

It's S when=build version=unbuilt-dep toolchain=rustup cache=target-dir, in my old taxonomy (i.e. the only difference from my previous thoughts is that I used to think cache=per-toolchain made sense - maybe it still does, but this stuff is awkward enough as-is within one target-dir, and would likely need custom locking and cache invalidation etc. to make it per-toolchain, not to mention the many rebuilds while working on Rust-GPU).

While version=unbuilt-dep was possible in the end, it's pretty convoluted (even using cargo_metadata, Cargo doesn't really want to let you enable optional dependencies except if you have control of the the workspace).

Also, I've kept the dual features that @watjurk disliked, but I can kinda see how bad they are for this, I ended up having to treat them like they cause different rustup toolchains to be used (i.e. nothing is shared between use-compiled-tools builds and use-installed-tools builds) - it's a shame, but I'm not sure what we can do in the long term (other than stop needing SPIRV-Tools entirely, but that's not happening any time soon).

[eddyb]

New (conceptual) development:

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

This is simultaneously very similar to existing setups (mostly only what decides the Rust-GPU version changes!), but also unlocks a lot more, because we can make a single version of some CLI tool work with many backend versions, as long as it's picked up from the shader crate (whether spirv-std or rust-gpu etc.).

AFAICT in the original nomenclature earlier in the thread, S/version= could only be one of:
hardcoded, same, unbuilt-dep, bundled - but they all refer to spirv-builder + rustc_codegen_spirv, whereas the new one would pull it from the shader itself (even if likely a lot like #1103).

The easiest thing we could try would be:
S when=dynamic version=shader-* toolchain=rustup cache=target-dir
_{(not sure what version= names would even make sense. I denoted changes from #1103 in italics, but build->dynamic is not that big of the deal when you control the whole build, mostly an issue of being careful around env vars)}

Also, if we get -Z script working for shaders, those might end up having their ~/.cargo-backed target dirs anyway, even if we don't come up with our own global caching (in fact, we could probably rely on -Z script to trigger the rustc_codegen_spirv build, so Cargo can both cache it globally and ideally reuse it).

[rust-gpu-bot]

This issue is now being tracked at: Rust-GPU/rust-gpu#104