search

oxidecomputer / oxide.rs

The Oxide Rust SDK and CLI
Mozilla Public License 2.0
37 stars 12 forks source link

Ban use of print macros in non-test code #749

Closed wfchandler closed 1 month ago

wfchandler commented 1 month ago

With #746 we added the _nopipe variants of the (e)print(ln)! macros to avoid panicking when piping to head(1). However, we do not systematically enforce their use, which will inevitably lead inconsistent usage within the project.

Add clippy lints to ban use of the print macros in all non-test code. Either the _nopipe variants should be used when we don't care if the input is read, or write! when the information is crucial, such as an interactive session.

ahl commented 1 month ago

This is what the rust big-brains came up with?

wfchandler commented 1 month ago

This is what the rust big-brains came up with?

No, I was just looking at what clippy could do for us before going to the council of elders and saw these lints.

ahl commented 1 month ago

This is what the rust big-brains came up with?

No, I was just looking at what clippy could do for us before going to the council of elders and saw these lints.

Worth asking the council of elders? The _nopipe suffix strikes me as inelegant.

ahl commented 1 month ago

Given https://github.com/oxidecomputer/oxide.rs/pull/774#issuecomment-2272135166 should we consider other approaches?

wfchandler commented 1 month ago

I brought this to the council of big brains going on vacation. There are a few possibilities, but given our need to support Windows I think the approach in this PR is best.

  1. Reset the SIGPIPE handler using the sigpipe crate. Ripgrep was doing this at one point, and we've done this in oxlog, but on Illumos using Rust < 1.82 this is not handled properly. More importantly, this does not work on non-Unix systems.
  2. Manually update the panic handler to ignore errors containing the string 'broken pipe'. This is the approach taken by uutils, but the consensus was that this is a bit janky.
  3. Don't use the standard print macros at all; the approach taken in this PR. If you feel that the _nopipe macros are too wordy, we could rename them to shadow the std macros. The clippy bug is a bit of a pain, but we're aware of it now and I think it's reasonable to watch for it until the lint is fixed.
ahl commented 1 month ago

Sounds like options 3 is the best option; and let's stay vigilant in code review given the clippy limitations.