Skip to content

Add a Nix configuration for the cargo rust-gpu environment #3027

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

timon-schelling
Copy link
Collaborator

No description provided.

@Firestar99
Copy link
Collaborator

I'd merge this with the shaders togehter, since we'll be jumping rev quite a few more times

@timon-schelling
Copy link
Collaborator Author

timon-schelling commented Aug 11, 2025

I suggest merging this separately and updating the rev in your pr, this should not have a influence on other devs.

@timon-schelling
Copy link
Collaborator Author

@Firestar99 I will merge this right before #2985 when we actually need it.

@Firestar99 Firestar99 force-pushed the provide-cargo-rust-gpu-to-nix-devs branch from b9dde9a to 75bbe25 Compare August 14, 2025 12:44
@Keavon Keavon mentioned this pull request Aug 18, 2025
@Keavon Keavon changed the title Provide cargo rust-gpu env to nix devs Add a Nix configuration for the cargo rust-gpu environment Aug 18, 2025
@Firestar99 Firestar99 marked this pull request as draft August 18, 2025 17:20
@Firestar99
Copy link
Collaborator

Converting to draft to prevent merging, for now, as I'm may have to update the rust-gpu rev quite a bit.

Comment on lines +66 to +81
execWithRustGPUEnvironment = pkgs.writeShellScriptBin "rust-gpu" ''
#!${pkgs.lib.getExe pkgs.bash}

filtered_args=()
for arg in "$@"; do
case "$arg" in
+nightly|+nightly-*) ;;
*) filtered_args+=("$arg") ;;
esac
done

export PATH="${pkgs.lib.makeBinPath [ rustGPUToolchainPkg pkgs.spirv-tools ]}:$PATH"
export RUSTC_CODEGEN_SPIRV_PATH="${rustc_codegen_spirv}/lib/librustc_codegen_spirv.so"

exec ${"\${filtered_args[@]}"}
'';
Copy link

Choose a reason for hiding this comment

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

It's so much easier to use RUSTUP_TOOLCHAIN for this, I was just asking @Firestar99 why the + syntax when the env var also works.

All you need in the script, if you want to have dynamic dispatch, is:

''
  #!${pkgs.lib.getExe pkgs.bash}
  exec "${toolchains}/${"\$"}{RUSTUP_TOOLCHAIN:-stable}/bin/$(basename "$0")" "$@"
''

(I don't like that ${"\$"} but am not sure I can make it much nicer)

I suppose something different than what you are doing today is you would want to define toolchains as one of those sym trees with one directory per supported toolchain - maybe the oxalica overlay rust-bin has something along those lines? (it's been a while since I looked at it anyway)


Although, if cargo-gpu/spirv-builder switched to setting RUSTUP_TOOLCHAIN, the fact that this script only has to filter out the + syntax, makes me think the script can just go away? (since I guess there is no dynamic dispatch right now, just a pesky first argument)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't want devs mistaking the pinned rust-gpu rust nightly toolchain for a normal nightly toolchain. We want a way to ensure that it is only ever usable to compile rust-gpu code. A wrapper script is the best way to ensure that in my opinion.

But getting rid of the filter logic would be great. Thanks for the comment.

Copy link
Collaborator

@Firestar99 Firestar99 Aug 20, 2025

Choose a reason for hiding this comment

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

I just want to note that on master, we kill the RUSTUP_TOOLCHAIN env var. I'm unsure if my branch has these changes already or not, so we could see behaviour differences between now and me rebasing the branch.

https://github.com/Rust-GPU/rust-gpu/blob/e0dfdd1112d9b2cf27d10e25104dc997db9acdf0/crates/spirv-builder/src/cargo_cmd.rs#L52

Yes we do need a better solution for this, but since this has to work within the next week, it'll have to wait.

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.

4 participants