Divide project into library and binary parts, round 2 #117

tuguzT · 2025-09-29T18:46:37Z

This is just a fresh redo of #112.
After re-reading discussions of the previous PR, I've came to conclusion that I should reimplement it to split library/binary parts more aggressively (to remove unnecessary dependencies inside of build scripts).

Fixes #105

Requires:

This PR uses move-then-modify trick in order to preserve file history of changes.

Here is the recap:

added new crate rustc_codegen_spirv-cache which responsibility is to install backend codegen dylib (compile it & move inside of cache directory)
added new crate cargo-gpu-build (library part of the project) which handles building of shader crates
this crate could be used inside of build scripts, something like this:

let builder = SpirvBuilder::new(SHADER_CRATE_PATH, "spirv-unknown-vulkan1.2")
    .shader_crate_default_features(false)
    .shader_crate_features(["nightly".into()])
    .spirv_metadata(SpirvMetadata::Full);
let compile_result = CargoGpuBuilder::new(builder)?.build()?;

cargo-gpu is a CLI (just the same as before), some of its internals was just moved into the crates above

Open questions:

~~should ShaderCrateBuilder be renamed into CargoGpuBuilder?~~

tuguzT · 2025-10-04T19:38:43Z

I've noticed just now that Config & Metadata structs of cargo-gpu are tightly linked & depend on Build implementing clap::Parser.
If we would like to support [metadata.*] section of Cargo.toml inside of build scripts, these structs should be decoupled from cargo-gpu's Build struct & moved inside of cargo-gpu-build.

In the near future, I'll try to make it possible, but not sure when exactly...

P. S.: First of all, I could remove shader_crate field from Install struct of newly created codegen backend cache crate and move it inside of cargo-gpu crate...
Or I could point out what fields should be touched by Config and Metadata...

Firestar99 · 2025-10-05T09:43:27Z

If we would like to support [metadata.*] section of Cargo.toml inside of build scripts

Yes please, though could also be in a future PR.

Background: If we ever want to support cargo gpu clippy (see Rust-GPU/rust-gpu#347), we'd want the cargo-gpu cmdline to have the same configuration as is used in the build script. But since we can't parse the config from the build script, we'd need the "default" configuration in the [metadata.*] section of the Cargo.toml. We could then offer an SpirvBuilder::from_cargo_metadata() to load those values and allow the build script to adjust them before running, to allow compiling multiple different variants (like different capabilities or features).
Not yet sure if we want it to be parsed at SpirvBuilder or at cargo_gpu::Install level. As spirv-builder already needs serde_json, we already got the serde dependency and adding toml parsing won't be a big deal. Though I think we should keep clap behind a feature gate.

Firestar99 · 2025-10-10T07:48:28Z

Support [.metadata.rust-gpu.] sections of Cargo.toml inside of build scripts

If this is not essential to your refactor, I'd rather review and get the current changes in and worry about this part later. As this PR sorta blocking all kinds of changes right now.

…m CLI into `cargo-gpu-build`

tuguzT · 2025-10-10T09:03:52Z

If this is not essential to your refactor, I'd rather review and get the current changes in and worry about this part later. As this PR sorta blocking all kinds of changes right now.

Yes, the main building blocks for that have already been moved into cargo-gpu-build.
Adding support in future PR should be easy enough.

Firestar99

(I may have missed that you set this as ready to review after your last comment)

I'm going to roast this PR a little, and I hope you don't see this as a personal attack or anything. Cause I'm seeing quite a few things that I think are not fine, or just don't make any sense to me. Feel free to explain your thought process behind the changes I consider questionable.

It is really difficult to review this PR, as it's effectively "let's just refactor the entire library"-sized. We're forced to review commit by commit (which is fine), but if we're noting something in the first commits, you now have to rebase all the commits on top. And then we have to rereview them, which is eating extra time from everybody. Many more commits and splitting different changes into different PRs would make this a lot easier.

I want to have a look if I could extract some of the good parts from the first few commits you've made and put it up as a separate PR.

Firestar99 · 2025-10-17T11:10:32Z

crates/rustc_codegen_spirv-cache/Cargo.toml

@@ -0,0 +1,32 @@
+[package]
+name = "rustc_codegen_spirv-cache"


First, a really fundamental question: What is the difference between rustc_codegen_spirv-cache and cargo-gpu-build? Why should they be two different crates, and not be one crate for both?

I wanted to split codegen backed install functionality from the build script API: the first one is being used to implement the second one, but potentially could be used for anything else (but for now I can't imagine a specific use case for this).

Firestar99 · 2025-10-17T11:13:23Z

crates/rustc_codegen_spirv-cache/src/backend.rs

+    #[inline]
+    pub fn install<I, R, W, T, C, O, E>(


Why is install inlined? I see that as you're moving code from cargo-gpu to rustc_codegen_spirv-cache, you're marking every function as #[inline]. I get inlining on small getter/setter functions or minor calculations as that can give you a lot better codegen downstream (without LTO), but install is huge!

It's clippy complaining about each public method of any struct not being inline. I guess it's better to silence this lint for big methods.

Firestar99 · 2025-10-17T11:17:39Z

crates/rustc_codegen_spirv-cache/src/backend.rs

+    pub fn install<I, R, W, T, C, O, E>(
+        &self,
+        params: I,
+    ) -> Result<SpirvCodegenBackend, SpirvCodegenBackendInstallError<R>>
+    where
+        I: Into<SpirvCodegenBackendInstallParams<W, T, C, O, E>>,
+        W: io::Write,
+        R: From<CommandExecError>,
+        T: FnOnce(&str) -> Result<(), R>,
+        C: FnOnce(&str) -> Result<(), R>,
+        O: FnMut() -> Stdio,
+        E: FnMut() -> Stdio,
+    {


This amount of generics is really hard to read, and I have no idea what's going on here. Imagine I'd be a new user trying to correctly use this function and seeing all the (undocumented) generics. Why not use some Box<dyn FnOnce> instead? All of these should (idealized) only be called once per install, them being a virtual call or nor is not going to make any performance difference when we're shelling out multiple times to run cargo metadata or cargo build, or the rustc compile in general.

I agree on this, too: I've overcomplicated it "a bit".
I'll definitely turn generics into thir boxed counter-parts.

I could be made into impl Into<...>, as I've replied to other requested change earlier.

W is for writer, or an impl of std::io::Writer trait, which is used to decide where to write user output (that one with crab emoji).

R, T and C are being used to halt toolchain install process, if needed. As I've understood it, this is clearly not needed in build scripts and was turned off by default in general, then turned on in clap default args.

And so, I've added this abstraction:

to remove crossterm dependency from build scripts (which is clearly redundant there)

to provide some flexibility to halt installation of toolchain & its components for the caller: for example, to forbid installing new toolchain and/or its components (to ensure rustup state is not being changed in any way, which could be useful in CI/CD).

O and E are being used to modify std::process::Stdio to pass inside of stdout & stderr methods of std::process::Command, or to be able to control if output of install & build should be "piped" or discarded.

Firestar99 · 2025-10-17T11:19:06Z

crates/rustc_codegen_spirv-cache/src/backend.rs

+    /// Failed to write user output.
+    #[error("failed to write user output: {0}")]
+    IoWrite(#[source] io::Error),


Why is every single message repeated twice, that seems like unnecessary clutter. And if it's just clippy complaining, feel free to silence it if it makes sense, like here where the errors are documented in the attribute already.

These rustdoc comments are clearly need to be improved (to better explain these kinds of errors). I just couldn't decide what to write there.

Firestar99 · 2025-10-17T11:28:49Z

crates/rustc_codegen_spirv-cache/src/backend.rs

+    /// Failed to create `src` directory for local `rustc_codegen_spirv_dummy` crate.
+    #[error("failed to create `src` directory for `rustc_codegen_spirv_dummy`: {0}")]
+    CreateDummySrcDir(#[source] io::Error),
+    /// Failed to create `src/lib.rs` file for local `rustc_codegen_spirv_dummy` crate.
+    #[error("failed to create `src/lib.rs` file for `rustc_codegen_spirv_dummy`: {0}")]
+    CreateDummyLibRs(#[source] io::Error),
+    /// Failed to write `Cargo.toml` file for local `rustc_codegen_spirv_dummy` crate.
+    #[error("failed to write `Cargo.toml` file for `rustc_codegen_spirv_dummy`: {0}")]
+    WriteDummyCargoToml(#[source] io::Error),
+    /// Failed to query cargo metadata of the local `rustc_codegen_spirv_dummy` crate.
+    #[error(transparent)]
+    QueryDummyMetadata(#[from] QueryMetadataError),
+    /// Could not find `rustc_codegen_spirv` dependency in the local `rustc_codegen_spirv_dummy` crate.
+    #[error(transparent)]
+    NoRustcCodegenSpirv(#[from] PackageByNameError),
+    /// Failed to determine the toolchain channel of `rustc_codegen_spirv`.
+    #[error("could not get toolchain channel of `rustc_codegen_spirv`: {0}")]
+    RustGpuToolchainChannel(#[from] RustGpuToolchainChannelError),
+    /// Failed to update target specs files.
+    #[error(transparent)]
+    UpdateTargetSpecsFiles(#[from] UpdateTargetSpecsFilesError),


I don't understand why we need a giant thiserror enum. Don't get me wrong, thiserror can be really useful to allow you to handle the various error states. But are you seriously expecting the end user to handle the error differently? Most of them wouldn't even know the difference between "failing remove the Cargo.lock file from the dummy project after resolving the toolchain version" (often happens when two installs race, see #72) and "The rustc_codegen_spirv_dummy crate this very program wrote does not contain a dependency on rustc_codegen_spirv" (which effectively never happens other than cargo-gpu itself being broken).
I bet 100% of end users either just unwrap/panic, or when watching, print the error, ignore the result and wait for the next recompilation. At that point, this entire enum is less informative as a simple anyhow::Result, since that one at least records the line it failed.
This specificity of all the errors also makes it super hard if we ever refactor the underlying install algorithm. And I do have some ideas on how to simplify target-spec handling and storing the toolchain version in a file to not have to run cargo metadata each time. So this is going to happen sooner than expected.

I actually agree on that: these error variants are revealing implementation details of codegen backend install algorithm (which could change at anytime), which (I guess) is not desired at all.
Also this errors could occur even when arguments provided by the caller are correct, which you find confusing (I guess).

I'll move these impl-detail error variants in a single one (I could actually use anyhow::Error inside of this merged variant).

Firestar99 · 2025-10-17T11:34:15Z

crates/rustc_codegen_spirv-cache/src/lib.rs

+/// Writes formatted user output into a [writer](std::io::Write).
+#[macro_export]
+macro_rules! user_output {
+    ($dst:expr, $($args:tt)*) => {{
+        #[allow(
+            clippy::allow_attributes,
+            clippy::useless_attribute,
+            unused_imports,
+            reason = "`std::io::Write` is only sometimes called??"
+        )]
+        use ::std::io::Write as _;
+
+        let mut writer = $dst;
+        #[expect(clippy::non_ascii_literal, reason = "CRAB GOOD. CRAB IMPORTANT.")]
+        ::std::write!(writer, "🦀 ")
+            .and_then(|()| ::std::write!(writer, $($args)*))
+            .and_then(|()| ::std::io::Write::flush(&mut writer))
+    }};
+}


Commit d9e440f moved this macro over:

Move toolchain install handling out of CLI

But it doesn't just move the toolchain install handling, it also:

moves this macro

adds $dst:expr, which for every single invocation is set to std::io::stdout.
Why?

I assume it is to later on have the user output be switchable to some other output, as it's one of the generic arguments on install. Yet again I have to ask, why would an end user want to switch that?

Build scripts should always print it into stdout / stderr, so cargo can emit it if it fails or is verbose

If your app is watching spirv-builder, you'd also expect a failed shader recompile to have logs printed to stdout/stderr, and maybe copied to some log by some outside launcher / bash.

As I've told in a change request regarding usage of generics, I've moved it one layer up to allow to decide where crab-prefixed message should be displayed.
This macro is used with generic writer in some places, not just with std::io::stdout, but I agree that the usual usage of this would be to just write the output in the stdout.

Firestar99 · 2025-10-17T11:34:56Z

crates/rustc_codegen_spirv-cache/src/metadata.rs

+pub fn query_metadata<P>(crate_path: P) -> Result<Metadata, QueryMetadataError>
+where
+    P: AsRef<Path>,
+{


Minor: Why generic when impl Path is sufficient? Not just here but everywhere

I prefer it this way to kinda document such generic by its name & because it is more explicit than impl some trait.
If it is preferred (in rust-gpu project) to use impl trait instead of generics, I could easily fix this back (to actually use impl trait).

Firestar99 · 2025-10-17T12:18:58Z

crates/cargo-gpu-build/src/build.rs

+    ///   * <https://github.com/rust-lang/cargo/pull/12280>
+    ///   * <https://github.com/rust-lang/cargo/pull/14595>
+    #[cfg_attr(feature = "clap", clap(long, action, verbatim_doc_comment))]
+    pub force_overwrite_lockfiles_v4_to_v3: bool,


I also don't understand why this bool was moved from cache to both cargo-gpu-build and cargo-gpu, with the entire block of text copy-pasted. Is it just cause of the (needless) generics?

Partially yes, I tried to move it one layer up (to be usable in build scripts), and then I've actually managed to move it in a single place (or deduplicate back) at the latest commits (don't remember which one for sure).

Firestar99 · 2025-10-17T12:33:00Z

crates/rustc_codegen_spirv-cache/src/spirv_source.rs

+    #[expect(
+        clippy::must_use_candidate,
+        reason = "calculations are cheap, `bool` is `Copy`"
+    )]
+    #[inline]
    pub const fn is_path(&self) -> bool {
        matches!(self, Self::Path { .. })
    }


The point of #[must_use] is for you to notice that some function is needlessly called. This isn't about performance, LLVM will inline and remove the call for you, this is about clean code.

I've thought that #[must_use] should be used if the result of this function should be used in some way (to handle errors, in a builder pattern and so on).
I'm not aware of any "some function is needlessly called" usage of this lint.

tuguzT · 2025-10-18T17:21:11Z

I'm going to roast this PR a little, and I hope you don't see this as a personal attack or anything. Cause I'm seeing quite a few things that I think are not fine, or just don't make any sense to me.

I inderstand that this PR could be hard to review due to lots of stuff happening there.
It's actually fine: now that I've cooled down a bit 😅 (after reading your request for the first time), I can reasonably respond to each one of them.
Free discussion of ideas, code style (regarding bound generics vs impl trait) & tradeoffs is what makes review process actually meaningful.

I'll try to address each change requested in some way on this weekend and maybe on the next one, too (in short, when I'll have some free time for this 🫡).

Feel free to move on with implementation of other functionality in separate PRs, I'll try to rebase them accordingly.
In other words, don't be blocked by this PR 🙃

This comment has been minimized.

Sign in to view

tuguzT mentioned this pull request Sep 29, 2025

Divide project into library and binary parts #112

Open

tuguzT force-pushed the split-lib-bin-deps-v2 branch 3 times, most recently from 0fc302e to a1893d4 Compare October 2, 2025 09:09

Firestar99 self-assigned this Oct 2, 2025

tuguzT force-pushed the split-lib-bin-deps-v2 branch 8 times, most recently from 2bff512 to 8670d74 Compare October 4, 2025 19:15

tuguzT force-pushed the split-lib-bin-deps-v2 branch 14 times, most recently from 214c3a2 to 592022f Compare October 7, 2025 20:19

tuguzT added 10 commits October 8, 2025 20:29

Move SpirvSource out of CLI

d96d975

Move target specs handling out of CLI

67e56f6

Move toolchain install handling out of CLI

d9e440f

Move install codegen handling out of CLI

1a697c2

Move lockfile manifest version mismatch handling out of CLI

d270893

Add error handling for rustc_codegen_spirv-cache

f6826cf

Add error handling for lockfile API of cargo-gpu-build

bf8a9ab

Work on a new build script API

0309824

Move user consent flag into cargo-gpu CLI struct

02a1549

Move force overwrite lockfile v4 to v3 flag into cargo-gpu

ce64bef

tuguzT force-pushed the split-lib-bin-deps-v2 branch from eff122a to 765f629 Compare October 8, 2025 17:41

tuguzT added 2 commits October 8, 2025 20:41

Rename some structs, move shader_crate field into cargo-gpu CLI

aeec85b

Refactor config & metadata modules of cargo-gpu

2b965ea

tuguzT force-pushed the split-lib-bin-deps-v2 branch 8 times, most recently from a96cf92 to 0af9e31 Compare October 9, 2025 19:53

Merge config module of CLI into metadata one, move metadata fro…

3b291a6

…m CLI into `cargo-gpu-build`

tuguzT force-pushed the split-lib-bin-deps-v2 branch from e28ea77 to 3b291a6 Compare October 10, 2025 09:02

tuguzT marked this pull request as ready for review October 10, 2025 09:22

tuguzT mentioned this pull request Oct 10, 2025

Redesign spirv-builder's watch API Rust-GPU/rust-gpu#422

Open

Firestar99 requested changes Oct 17, 2025

View reviewed changes

Firestar99 mentioned this pull request Oct 17, 2025

Nextest support and update template #120

Open

Divide project into library and binary parts, round 2 #117

Are you sure you want to change the base?

Divide project into library and binary parts, round 2 #117

Uh oh!

Conversation

tuguzT commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

tuguzT commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Firestar99 commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Firestar99 commented Oct 10, 2025

Uh oh!

tuguzT commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Firestar99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Firestar99 Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuguzT commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tuguzT commented Sep 29, 2025 •

edited

Loading

tuguzT commented Oct 4, 2025 •

edited

Loading

Firestar99 commented Oct 5, 2025 •

edited

Loading

tuguzT commented Oct 10, 2025 •

edited

Loading

tuguzT Oct 18, 2025 •

edited

Loading

tuguzT Oct 18, 2025 •

edited

Loading

Firestar99 Oct 17, 2025 •

edited

Loading

tuguzT Oct 18, 2025 •

edited

Loading

tuguzT Oct 18, 2025 •

edited

Loading

tuguzT commented Oct 18, 2025 •

edited

Loading