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

Addresses cargo-pgrx error reporting #1238

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions cargo-pgrx/src/command/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,14 +271,35 @@ fn untar(bytes: &[u8], pgrxdir: &PathBuf, pg_config: &PgConfig) -> eyre::Result<
.wrap_err("failed to spawn `tar`")?;

let stdin = child.stdin.as_mut().expect("failed to get `tar`'s stdin");
stdin.write_all(bytes)?;
stdin.flush()?;
let mut error = None;

// In order to capture and report any output from the child's stderr (in the event of an error),
// we need to wait until `.wait_with_output()` is called. Bailing early here via `?` or `.expect()`
// means only the immediate error is reported, such as "Broken Pipe". The same applies for the subsequent
// `.flush()` call immediately after.
match stdin.write_all(bytes) {
Ok(_) => (),
Err(e) => error = Some(e),
}

match stdin.flush() {
Ok(_) => (),
Err(e) => error = Some(e),
}
Comment on lines +280 to +288
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for both of these to error? We'd hypothetically be dumping one error on the floor in favor of another, again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be very surprised if flush did anything on this type. But we should probably either take the first error or use chaining, if we care.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this as I was writing it, but I wasn't entirely sure how to handle it without making it "shift right" or making a plate of spaghetti out of it.

Copy link
Member

Choose a reason for hiding this comment

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

You will see me tend to incrementally construct and then immediately pattern-match on tuples a lot in my code for this reason.


let output = child.wait_with_output()?;

if output.status.success() {
if output.status.success() && error.is_none() {
Ok(pgdir)
} else {
Err(eyre!("Command error: {}", String::from_utf8(output.stderr)?))
// It's possible to have a "None" error from above, but the child output still reports an error.
// We need to handle both of those cases.
let command_error = eyre!("Command error: {}", String::from_utf8_lossy(&output.stderr));

match error {
Some(e) => Err(eyre!(e).wrap_err(command_error)),
None => Err(command_error),
}
}
}

Expand Down