fix: raster shader build fail#3950
fix: raster shader build fail#3950Crypto-Darth wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical build failure in the raster shader compilation process. By carefully managing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the raster shader build fail by correctly managing the CARGO_BUILD_TARGET environment variable during the cargo_gpu installation process. By temporarily unsetting and then restoring this variable, the build system avoids targeting wasm32-unknown-unknown for dylib generation, which was causing the failure. The approach of saving and restoring the environment variable is sound for ensuring the rest of the build process functions as expected.
| unsafe { | ||
| // Unset `CARGO_BUILD_TARGET` to prevent `rustc_codegen_spirv` from picking it up and trying to build for the wrong target. | ||
| std::env::remove_var("CARGO_BUILD_TARGET"); |
There was a problem hiding this comment.
The unsafe blocks for std::env::remove_var (lines 32-34) and std::env::set_var (lines 50-52) are used to manipulate environment variables. While unsafe is required on Windows for these operations due to potential thread-safety concerns, it's good practice to provide a clear // SAFETY: comment explaining why it's safe in this context, similar to the one on lines 21-23. This ensures that the justification for using unsafe is explicit and consistent throughout the file.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
1 participant
Resolves #3939
Cargo run was causing failure near the step where
raster-nodes-shaderswhere it is generatingdylibfor the wrong target (wasm32-unknown-unknown)Solution I implemented was to unset the
CARGO_BUILD_TARGETbeforecargo_gpuinstallation and then setting it right after for the rest of the build process.AI Usage - To help find and understand the root cause of the actual error message generated on
cargo runand suggestions for the solution which was implemented.Thanks to @ghchinoy for his findings which was also same as when i tried debugging it and for his proposed solution as mentioned on #3939