-
Notifications
You must be signed in to change notification settings - Fork 55
Updating target json support #256
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
Conversation
Should this be its own crate? @eddyb and I discussed it in the past. |
You mean just the target specs json files as a separate crate? And have cargo gpu to just permanently depend on the "legacy" version of the target specs? (legacy meaning before this PR) Just one issue with that: |
Yeah the json as their own crate. Doing so lets us have multiple versions and doesn't bring in other deps if something just needs the json. As I said we discussed it in the past but I think just kicked the can down the road until problems cropped up. We may now be there ha |
cb11fc7
to
0f6287b
Compare
Created a new rust-gpu/crates/rustc_codegen_spirv_target_specs/README.md Lines 1 to 7 in 0f6287b
|
ee6afdb
to
0ee6729
Compare
I'd like @eddyb to chime in before approving. |
@LegNeato it would be nice to have |
0ee6729
to
b77676e
Compare
Agreed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't like the proliferation of crates, and I've yet to review any of the cargo-gpu
work, but:
- most of this seems to be straightforward
- the ugly
rustc_codegen_spirv
prefix actually works in its favor
I only have one requests, which is mostly aesthetic.
I'm tempted to suggest something more general that could emcompass both the -types
crate and the new target specs, or at least not overly narrowing the new one to "target specs" - but being specific to rustc_codegen_spirv
, not Rust-GPU or even the spirv-*
crates, makes me once again not want to overthink it.
What's kind of nice with this setup is you can easily release older versions of this crate afterwards, even if starting at 0.9. The only downside is the "dead zone" of git revs (e.g. if anyone wanted to bisect Rust-GPU versions), though, hmm, it's not like the files weren't there in the git repo - I'm maybe a bit surprised you couldn't pull it from the source of |
3ef526e
to
9b99f20
Compare
I didn't want to "magically" pull them from |
48c290a
to
9b19c61
Compare
cargo gpu: Rust-GPU/cargo-gpu#75
#249 (comment)
Note: replace
rustc_codegen_spirv-types
with the newrustc_codegen_spirv-target-specs
crate#249 (comment)