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

cli: handle EPIPE for version subcommand #774

Merged
merged 1 commit into from
Aug 2, 2024
Merged

cli: handle EPIPE for version subcommand #774

merged 1 commit into from
Aug 2, 2024

Conversation

taspelund
Copy link
Contributor

@taspelund taspelund commented Aug 2, 2024

EPIPE handling was added recently via {e}println_nopipe() macros, but wasn't used for the "version" subcommand. This replaces println() calls with println_nopipe().

Before:

➜  oxide.rs git:(main) ./target/debug/oxide version | head -1
Oxide CLI 0.6.1+20240710.0
➜  oxide.rs git:(main) ./target/debug/oxide version | a
zsh: command not found: a
thread 'tokio-runtime-worker' panicked at library/std/src/io/stdio.rs:1021:9:
failed printing to stdout: Broken pipe (os error 32)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at cli/src/main.rs:105:10:
called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(9), ...)
➜  oxide.rs git:(main)

After:

➜  oxide.rs git:(trey/epipe_version) ./target/debug/oxide version | head -1
Oxide CLI 0.6.1+20240710.0
➜  oxide.rs git:(trey/epipe_version) ./target/debug/oxide version | a
zsh: command not found: a
➜  oxide.rs git:(trey/epipe_version)

Fixes: #773

@taspelund taspelund requested review from ahl and wfchandler August 2, 2024 19:38
EPIPE handling was added recently via {e}println_nopipe() macros, but
wasn't used for the "version" subcommand.  This replaces println()
calls with println_nopipe().

Before:
```
➜  oxide.rs git:(main) ./target/debug/oxide version | head -1
Oxide CLI 0.6.1+20240710.0
➜  oxide.rs git:(main) ./target/debug/oxide version | a
zsh: command not found: a
thread 'tokio-runtime-worker' panicked at library/std/src/io/stdio.rs:1021:9:
failed printing to stdout: Broken pipe (os error 32)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at cli/src/main.rs:105:10:
called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(9), ...)
➜  oxide.rs git:(main)
```

After:
```
➜  oxide.rs git:(trey/epipe_version) ./target/debug/oxide version | head -1
Oxide CLI 0.6.1+20240710.0
➜  oxide.rs git:(trey/epipe_version) ./target/debug/oxide version | a
zsh: command not found: a
➜  oxide.rs git:(trey/epipe_version)
```

Fixes: #773

Signed-off-by: Trey Aspelund <[email protected]>
@taspelund taspelund force-pushed the trey/epipe_version branch from a866fe1 to 1891e12 Compare August 2, 2024 19:41
@ahl
Copy link
Collaborator

ahl commented Aug 2, 2024

@wfchandler do we know why you lint pr didn't turn this up?

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Nailed it

@ahl ahl merged commit 44ec422 into main Aug 2, 2024
16 checks passed
@ahl ahl deleted the trey/epipe_version branch August 2, 2024 21:49
@wfchandler
Copy link
Contributor

@wfchandler do we know why you lint pr didn't turn this up?

@ahl the lint PR isn't merged yet.

@ahl
Copy link
Collaborator

ahl commented Aug 6, 2024

@wfchandler do we know why you lint pr didn't turn this up?

@ahl the lint PR isn't merged yet.

Yes, but the code exists in that PR and wasn't flagged: https://github.com/oxidecomputer/oxide.rs/blob/wc/enforce-nopipe/cli/src/cmd_version.rs#L30

@wfchandler
Copy link
Contributor

@wfchandler do we know why you lint pr didn't turn this up?

@ahl the lint PR isn't merged yet.

Yes, but the code exists in that PR and wasn't flagged: https://github.com/oxidecomputer/oxide.rs/blob/wc/enforce-nopipe/cli/src/cmd_version.rs#L30

Oh interesting, this seems like a clippy bug. In my testing this only happens in #[async_trait] impls, maybe something about the proc macro expansion is preventing clippy from recognizing the println!. Will investigate further.

@wfchandler
Copy link
Contributor

@wfchandler do we know why you lint pr didn't turn this up?

@ahl the lint PR isn't merged yet.

Yes, but the code exists in that PR and wasn't flagged: https://github.com/oxidecomputer/oxide.rs/blob/wc/enforce-nopipe/cli/src/cmd_version.rs#L30

Oh interesting, this seems like a clippy bug. In my testing this only happens in #[async_trait] impls, maybe something about the proc macro expansion is preventing clippy from recognizing the println!. Will investigate further.

More specifically, async_trait functions with a return type will fail to trigger the lint. I've opened rust-lang/rust-clippy#13228

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.

CLI panics upon EPIPE
3 participants