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

progress.rs: don`t fail if not a tty #6395

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@srwalter
Copy link

srwalter commented Dec 7, 2018

It's not clear that it was intentional for progress to be disabled when
the output is not a tty. Indeed there is reason we might want to have
progress output in that case. For example, when cargo is invoked by
bitbake, cargo's output will be run through a pipe. Bitbake can parse
out progress updates from that output, but only if cargo actually emits
them...

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 7, 2018

Thanks for the PR! This was originally done intentionally because most pipes don't handle the commands we later use for things like clearing a line too well (and redirecting to a file looks weird with the progress bar and such). That being said perhaps we could add an option to force the progress bar to show up even if the output isn't a TTY?

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Dec 7, 2018

I personally would prefer a config option (which would implicitly support an env var), though not everyone agrees. cc #5721

@srwalter srwalter force-pushed the srwalter:progress branch from a90c75f to 20bf4b2 Dec 7, 2018

@srwalter

This comment has been minimized.

Copy link

srwalter commented Dec 7, 2018

Something like this?

Steven Walter
Add an option to force progress output
The --progress switch will cause progress updates to be output, even if
the output device is not a TTY.

@srwalter srwalter force-pushed the srwalter:progress branch from 20bf4b2 to 45384bf Dec 7, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 10, 2018

Seems reasonable to me! Let's see what others think.

@rfcbot fcp merge

As one question, do others have thoughts on the name? Would we maybe want to generalize --progress slightly or leave it targeted as it is now?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 10, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Dec 10, 2018

Do we want this to be a arg or an env var?
Do we want to add a symmetric way to force it off?

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Dec 10, 2018

I still lean towards a config var myself.

Either way, I definitely think it should be generalized with a string. There have been several requests for a way to force it off. The current solution of CI=1 is pretty hacky.

@srwalter

This comment has been minimized.

Copy link

srwalter commented Dec 10, 2018

@ehuss so you want to see e.g. --progress=on and --progress=off?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 10, 2018

This is the sort of env var I wish we didn't have to have: it would be ideal to me if tools wrapping cargo and piping its output were dealing with a lower level CLI, and the primary user focused CLI were just opinionated about what its output looks like.

That said, users have problems now so I don't know what else to do.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Dec 10, 2018

I'm also in favour of a config var and using a string. I'd prefer something more descriptive than on and off, e.g., none and something describing how the default bar is rendered?

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Dec 10, 2018

Let's default to "auto" (progress if tty) and have --progress and --no-progress for forcing it on and off. Let's not worry about having a "batch friendly progress" beyond printing complete fetches or builds.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Dec 11, 2018

I don't have a strong opinion about the text. One option is to mirror the color option and use auto/always/never. @nrc's suggestion sounds fine, too.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 12, 2018

From what others have said and remembering about .cargo/config, I think that's the way to go here as well. We've already got a [term] section for other terminal configuration, and I think this'd fit nicely there. Eventually I'd like to implement an arbitrary --config CLI option as well so you can use that instead of an env var too!

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Dec 12, 2018

@alexcrichton The ability to say "always" in configuration can lead to some awful breakage if tools expect to parse the command-line output.

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Dec 13, 2018

This feels like the kind of arbitrary additions to the surface area of the CLI arguments, config keys and env vars that we don't want to rush out.

How many users of Bitbake (and similar) are we trying to cater to? How important is this capability for them?

I don't mean to ask that in a harsh way, just trying to understand better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment