Skip to content

Updating target json support #75

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Firestar99
Copy link
Member

@Firestar99 Firestar99 commented May 21, 2025

Requires publishing rustc_backend_spirv-target-specs and updating the commit rev of spirv-builder

rust-gpu: Rust-GPU/rust-gpu#256

This is the first PR to update the target spec jsons, which causes cargo gpu to run into some trouble. The codegen backend verifies that these jsons match exactly what it expects, and there's no flag to skip that check, and adding one now wouldn't help since we want to keep compatible with at least 0.9.0. Cargo gpu was never designed to have multiple target-spec versions, it always just used the latest. [...]

Rust-GPU/rust-gpu#249 (comment)

Note: replace rustc_codegen_spirv-types with the new rustc_codegen_spirv-target-specs crate

Could we move the target jsons from spirv_builder to rustc_codegen_spirv-types? That would allow cargo-gpu to resolve the required target jsons from rustc_codegen_spirv depending on rustc_codegen_spirv-types (and spirv_builder will retain access to it). If we can't find any target jsons in rustc_codegen_spirv-types, we can just fall back on the current target jsons, which we'd have to ship with cargo-gpu for the forseeable future. But I think that's a fine compromise, at least we wouldn't need to ship all future target jsons as well.

Rust-GPU/rust-gpu#249 (comment)

@Firestar99
Copy link
Member Author

I've tested this locally with a bunch of variations: path legacy, path new, git legacy, git new, git 2024 PR (expected fail), path 2024 PR rebased (works)

object.insert("build".to_owned(), build);

Ok(cli_args_json)
Ok(serde_json::to_value(crate::build::Build::parse_from(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Niiiice, so much simpler 🚀

Copy link
Member Author

Choose a reason for hiding this comment

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

these are some of the commits I cherry-picked from my library refactor :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

oooh :D

@Firestar99 Firestar99 force-pushed the updating-target-json-support branch from 05084f4 to 876d1cc Compare May 22, 2025 10:12
@Firestar99 Firestar99 force-pushed the updating-target-json-support branch 3 times, most recently from f1d9c5a to 1a1f673 Compare May 26, 2025 11:09
@Firestar99 Firestar99 force-pushed the updating-target-json-support branch 5 times, most recently from 8fc529a to fb10e19 Compare May 28, 2025 17:51
@Firestar99
Copy link
Member Author

Rust-GPU/rust-gpu#256 got merged, so I'm just waiting for LegNeato to publish a version of rustc_codegen_spirv-target-specs on crates, that I can make this PR use that version and we can get this merged

@LegNeato
Copy link

@Firestar99 done! I tried to add you as an owner but it said name not found...you might need to log into crates.io or something 🤷

@Firestar99 Firestar99 force-pushed the updating-target-json-support branch from fb10e19 to fd5da3c Compare May 30, 2025 08:44
@Firestar99
Copy link
Member Author

Firestar99 commented May 30, 2025

@tombh @schell this one is ready for final review and merge.

I've updated shader-crate-template to point to current master, so it should use the other branch that copies the target specs from the new rustc_codegen_spirv-target-specs crate. However, currently those don't differ from the legacy path so you wouldn't notice it failing (yet).

We should consider adding more CI (in a later PR?) to:

@Firestar99 Firestar99 enabled auto-merge (squash) May 30, 2025 16:35
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.

3 participants