-
Notifications
You must be signed in to change notification settings - Fork 12
show-targets #36 #79
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
show-targets #36 #79
Conversation
comments
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 would suggest you wait with this one a little until #75 is merged, as it changes how we forward "target specs" to our codegen backend. Specifically, we'll copy the "target specs" from the new rustc_codegen_spirv-target-specs
crate to ~/.cache/rust-gpu/codegen/<version_string>/target-specs/
, so you should just be able to list all files in that directory. But note that some targets, like opencl, will just error as "unsupported" if you try to use them.
I've also just proposed to make an enum Target
of all targets, to give us compile-time error reporting on the target name. Waiting for suggestions on that one.
Relevant code:
af15805#diff-c1ca29f6b5cc2db8f474ffc26467038108c480552a0961c3442ec6ca81efa18bR195
crates/cargo-gpu/src/show.rs
Outdated
"spirv-unknown-vulkan1.2", | ||
]; | ||
|
||
let output = Command::new("spirv-val") |
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.
We cannot guarantee that the end user has spirv-val
installed on their system. Cargo gpu compiles spirv-tools
itself when building rustc_codegen_spirv
and statically links it, and in theory you'd have to ask that one.
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.
Yep -- had feeling there'd be a better way. TY
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.
We can just use pub use include_str::TARGET_SPECS;
no?
~/.cache/rust-gpu/codegen/<version_string>/target-specs/
's dir contents would be more tedious & obviously non friendly to those unfortunate enough to have to develop on Windows.
It wouldn't be too hard (I think?) to whip up a const fn that just lists of the ones that were used to compile cargo gpu
itself, but it seems a shame to support only specs that were compiled in during install... no?
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.
Note where that symbol is coming from:
cargo-gpu/crates/cargo-gpu/src/legacy_target_specs.rs
Lines 1 to 4 in b497234
//! Legacy target specs are spec jsons for versions before `rustc_codegen_spirv-target-specs` | |
//! came bundled with them. Instead, cargo gpu needs to bundle these legacy spec files. Luckily, | |
//! they are the same for all versions, as bundling target specs with the codegen backend was | |
//! introduced before the first target spec update. |
If future versions of rust-gpu introduce new targets (such as spirv-unknown-vulkan1.3
), they won't be listed. Which also means targets are dependent on the version of rust-gpu used. I'd recommend doing the same as cargo build
does, have a --shader-crate
arg that defaults to ./
.
The I think we want to avoid doing |
There are 4 version ranges of
So while your submitted version will work for now, if I add support for the |
Ok, this directory On Windows: It should be at On Mac: It should be at There is nothing unusual about my particular setups' dir structures or permissions on either of these machines, I'm not familiar enough with what If we can get the It is because of all this, that I went for the contents of the TARGET_SPECS const (for now). So, do we want to park this until the edition 2024 PR goes in? I'm happy to update it/make changes etc. |
use the `cache_dir`, but fallback on the legacy const as it's known to be there, currently the cache_dir's presence is not guaranteed.
cargo-gpu/crates/cargo-gpu/src/install.rs Lines 283 to 288 in b497234
And create cargo-gpu/crates/cargo-gpu/src/install.rs Lines 157 to 160 in b497234
And create the cargo-gpu/crates/cargo-gpu/src/install.rs Line 214 in b497234
|
Mind if I give this a try? |
Of course not :) |
I've noticed I've added some special handling around local file paths to not have to copy the target specs, which required some refactoring to also make it work with that command. Now it prints this if successful, and may fail if the version is not installed or the shader crate's path is not a proper shader crate.
|
Delicious. |
It looks like I completely missed |
I think this is great! Especially the doc comments. I'd be happy to approve this, but I know schell has worked with the target specs more than me, so maybe wait a bit to see if he has any comments? |
Takes a stab at #36, not sure of the methodology for retrieving the targets, but the plumbing is here for when we decide the best method.
closes #36