Skip to content
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

Ran cargo fmt + added some .context() calls for non-obvious errors #56

Merged
merged 8 commits into from
Mar 30, 2025

Conversation

Niedzwiedzw
Copy link
Contributor

No description provided.

@tombh
Copy link
Collaborator

tombh commented Mar 15, 2025

Oh nice! I see we already have a cargo fmt in CI, but it seems it produces different output to yours. Do you have a particular config?

@Niedzwiedzw
Copy link
Contributor Author

hmmm noo I don't think so... it'd have to be in ~/rustfmt.toml right?

@Niedzwiedzw
Copy link
Contributor Author

ahh but now the imports didn't change back

@Niedzwiedzw
Copy link
Contributor Author

@tombh all right I think it's ready for review

Copy link
Collaborator

@schell schell left a comment

Choose a reason for hiding this comment

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

Thank you! Just a few little nits and then I think we're ready to merge.

@@ -7,7 +7,6 @@ use std::io::Write as _;

use crate::{install::Install, target_spec_dir};
use spirv_builder_cli::{args::BuildArgs, Linkage, ShaderModule};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this removed by auto formatting?

@@ -1,4 +1,5 @@
//! Install a dedicated per-shader crate that has the `rust-gpu` compiler in it.
//!
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a blank line, instead of a blank comment.

let mut file = std::fs::File::create(&path)
.with_context(|| format!("creating file at [{}]", path.display()))?;
file.write_all(contents.as_bytes())
.context("writring to file")?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Little typo here.

.context("getting command output")
.and_then(|output| {
log::trace!("STDOUT:\n{}", String::from_utf8_lossy(&output.stdout));
log::trace!("STDERR:\n{}", String::from_utf8_lossy(&output.stderr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines shouldn't be necessary, since the command inherits stdin and stdout, right? Maybe I'm missing something though.

@Niedzwiedzw Niedzwiedzw requested a review from schell March 15, 2025 19:45
@schell
Copy link
Collaborator

schell commented Mar 16, 2025

Thanks @Niedzwiedzw - just waiting for the CI checks and then I'll merge 👍 .

@LegNeato LegNeato merged commit 924d0ab into Rust-GPU:main Mar 30, 2025
6 checks passed
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.

4 participants