Switch to cargo metadata #66
Conversation
|
I also did |
|
This looks great, thank you for taking the time to look into this. It relates to #55 right? I think this is a good approach. I had already started doing something similar in this function: |
crates/cargo-gpu/src/spirv_source.rs
Outdated
| ) | ||
| ); | ||
| let cargo_tree_string = String::from_utf8_lossy(&output_cargo_tree.stdout); | ||
| log::debug!("Running `cargo metadata` on {}", canonical_shader_path.display()); |
There was a problem hiding this comment.
It might be more accurate to say that we're running cargo_metadata::MetadataCommand now right?
There was a problem hiding this comment.
It was to follow this line
https://github.com/Rust-GPU/cargo-gpu/pull/66/files#diff-e50e99da00483f62292c3fcb50fdea5e75b82a3d8506d159cc26e1bca432c282L208
It says it's running cargo tree like terminal command and I believe it should still print it this way?
|
Why did the CI failed? it looks to be something about spirv-tools build error. Is it related to this pr? |
|
The Windows build certainly looks like it's having a problem building But the Linux builds against older versions of |
|
Currently it works for local project as well but I now have new issue: #68 |
|
Should we bump minor version from 0.1.0 to 0.1.1 after this pr? |
This is bandaid fix, it now ignores RUSTC environment variable set from elsewhere, like when shader crate is part of a workspace. As of today, it fails to compile on workspace which uses nightly because an additional restriction was added to target specification json file on nightly. Even though enforcing specific rustc version seems like good idea for the purpose of cargo-gpu, target specification json file should be updated as well if we were to ever bump up required rustc version.
|
I created a bandaid fix for #68 so that it would ignore RUSTC environment variable set from outside like workspace root. |
|
Could I take over this PR / cherry-pick it into mine? I'm currently refactoring the inner workings of cargo gpu a lot, and while our PRs don't strictly overlap, I see an opportunity to completely remove the |
|
@Firestar99 Yeah sure, I'll still be included in co-author in those cherry picks, right? |
|
of course! |
Hello, I've been using cargo-gpu and trying to contribute for a while. and I found issue #37 which seemed easy enough.
In this pr, I removed 2 few tests checking if package is correctly parsed, because those were now handled by external crate cargo_metadata which is maintained by oli-obk, although I'm just trusting them to maintain this properly.
It now uses shorter revision. Previously, the tests used revision 82a0f but now it's 82a0f69008414f51d59184763146caa6850ac588, which is much longer. and I think same would happen to directory names in cache. so users updating would redownload directories, unless we find a way to check if those are same revisions.
I expect some bugs to present in this PR, and I'm open for more changes. This is just initial proposal.