trycmd icon indicating copy to clipboard operation
trycmd copied to clipboard

trycmd should normalise some env vars including COLUMNS

Open ijackson opened this issue 1 year ago • 4 comments
trafficstars

Steps

git clone https://gitlab.torproject.org/tpo/core/arti.git
cd arti
git checkout  5e2d5532d002c9df9a4cf6d83cdbe7d6125bc4be
COLUMNS=100 cargo test --locked --offline -p arti --all-features -- cli_tests
COLUMNS=1000 cargo test --locked --offline -p arti --all-features -- cli_tests

Expected output

  1. Successful run
  2. Successful run

Actual output

  1. Successful run
  2. Report of differences, apparently due to line wrapping

Discussion

Programs using clap, and many similar, may pay attention to COLUMNS.

Probably, the variable should be set rather than left unset. After, all, a conscientious program which finds it has no COLUMNS might fall back on interrogating the tty with termios ioctls.

There may be other relevant env vars. Maybe grepping the source code to clap would be helpful. It would also be nice if the API had a way to pass additional env vars as a workaround.

(edited to correct the wrong git commitid)

ijackson avatar Aug 27 '24 17:08 ijackson

I lean towards giving users control over this rather than setting policy for how programs are tested.

epage avatar Aug 27 '24 17:08 epage

It would also be nice if the API had a way to pass additional env vars as a workaround.

We should be able to use TestCases::env here, but I agree it would be better if trycmd set COLUMNS to a reasonable default (if unset).

gabi-250 avatar Aug 27 '24 17:08 gabi-250

I lean towards giving users control over this rather than setting policy for how programs are tested.

I agree with the idea of giving users control. I don't think that control is most conveniently achieved by honouring whatever COLUMNS is in the outer environment.

We should be able to use TestCases::env here

Oh yes, sorry, I had missed that. So I agree with @gabi-250's suggestion.

ijackson avatar Aug 27 '24 17:08 ijackson

Ah, I hadn't looked to see if we already provided something.

While Cargo doesn't use trycmd, I think looking at its env variables it sets on tests is useful https://github.com/rust-lang/cargo/blob/ef854d2f66df7bfcd803a6ca1cfa3b619603142c/crates/cargo-test-support/src/lib.rs#L1388-L1478

That is a sliding scale from "this is cargo specific" to "this is like COLUMNS" which includes what domains the application touches (e.g. git). At this time, I don't feel up to arbitrating which variables we process automatically for users and which we don't.

epage avatar Aug 27 '24 18:08 epage