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

Cargo should use progress bar only on terminals that support it #5721

Closed
gnzlbg opened this issue Jul 12, 2018 · 15 comments
Closed

Cargo should use progress bar only on terminals that support it #5721

gnzlbg opened this issue Jul 12, 2018 · 15 comments
Labels
A-console-output Area: Terminal output, colors, progress bar, etc.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 12, 2018

Currently cargo seems to always use a progress bar, but for example the terminal that travis uses does not support it, such that every update to the progress bar creates a lot of noise: https://api.travis-ci.org/v3/job/403149958/log.txt

Cargo should detect whether the terminal allows modifying a line in place, and if it doesn't, it shouldn't really use a progress bar.

Also, there should be an environment variable that allows disabling the progress bar, but I couldn't find one in the cargo book. So maybe the docs need updating, or if there isn't one, we maybe should add one.


Also, kudos to whoever added the progress bar, been using it all day, and its really nice, so thanks for doing that!

@ehuss
Copy link
Contributor

ehuss commented Jul 12, 2018

I think that Travis intentionally sets TERM to xterm because the build output on the website (https://travis-ci.org/rust-lang/rust/) is colorized. Adding TERM=dumb should disable the progress bar.

Note: The progress bar has been placed behind an unstable option in #5716, so it will go away the next time cargo is updated in nightly.

Edit: Also, Travis sets up scripts with a tty.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 12, 2018

I think that Travis intentionally sets TERM to xterm because the build output on the website

Oh crap, then cargo cannot really detect this. If there was an environment variable, one could disable the progress bar without having to change the TERM.

The progress bar has been placed behind an unstable option in #5716, so it will go away the next time cargo is updated in nightly.

Sad to hear this, i've enjoyed it a lot today :/

@RalfJung
Copy link
Member

RalfJung commented Nov 4, 2018

So one could argue that the bug is really on the Travis side for setting TERM=xterm without actually supporting xterm properly? Is there an issue on their side to track that?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 4, 2018

So one could argue that the bug is really on the Travis side for setting TERM=xterm without actually supporting xterm properly?

Yes, but this is no excuse for cargo to break travis-CI for every Rust project using it: I'm getting notifications that builds are failing because now they are writing N-times more logs due to the progress bar, exceeding the log limit.

A workaround could be for cargo to detect whether it is running in a travis instance (e.g. checking if the TRAVIS environment variables is true - https://docs.travis-ci.com/user/environment-variables#default-environment-variables), and disabling the progress bar if this is the case.

cargo should add some integration test to its CI to make sure that future changes do not break Travis-CI - it is the second time that this particular change breaks it in the last couple of months and it appears to have gone completely unnoticed. Who's fault is it (cargo or travis-ci) is not really relevant IMO.

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2018

I agree there probably should be some way to disable it.

Does anyone at @rust-lang/cargo have any opinions about this, and how to approach it?

  • It could check for TRAVIS env var. That seems a little odd, though.
  • It could check for CI env var. It looks like the majority of CI systems will set this.
  • It could check for some custom var (CARGO_PROGRESS_BAR?). However, if this is the only option, that means everyone who uses travis would need to set it, which most probably won't. (EDIT: another idea is to use some config var which would have an env var for free)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 4, 2018

It could check for some custom var (CARGO_PROGRESS_BAR?). However, if this is the only option, that means everyone who uses travis would need to set it, which most probably won't.

I think that having such an environment variable would be useful irrespectively of what's done about the other options.

@nrc
Copy link
Member

nrc commented Nov 4, 2018

It could check for CI env var.

This seems good to me, like the most general option and already an established 'standard'.

It could check for some custom var

I have a bad opinion of env vars in general (they are global mutable state) and I'd much prefer to use a command line option (if the CI env var is not good enough).

@dwijnand
Copy link
Member

dwijnand commented Nov 4, 2018

👍 for the CI env var, seems like a reasonable thing to hinge off of.

If push comes to shove we could also have a CARGO_PROGRESS_BAR env var, that overrides CI.

@alexcrichton
Copy link
Member

Oh one thing I should mention on the cli/env/config var debate is that one thing I've intended we should do for a long time is to add a --config flag to Cargo for all subcommands. Just like .cargo/config already has env vars to configure it I'd ideally like to have a --config flag as well, that way whatever's easiest for the use at hand can be used without us having to dictate which is the best

@dwijnand
Copy link
Member

dwijnand commented Mar 2, 2019

one thing I've intended we should do for a long time is to add a --config flag to Cargo for all subcommands

(there's now a proposal for that in #6699)

@ehuss ehuss added the A-console-output Area: Terminal output, colors, progress bar, etc. label Sep 21, 2019
@ehuss
Copy link
Contributor

ehuss commented May 28, 2021

I'm going to close this as fixed. #6281 disabled the progress bar when CI env var is set (which Travis does). Additionally, there is now a progress bar config, where CARGO_TERM_PROGRESS_WHEN=never can disable the progress bar.

@ehuss ehuss closed this as completed May 28, 2021
@RalfJung
Copy link
Member

FWIW, CI is not set on Azure (because why would MS follow any kind of standard they didn't invent). For that reason Miri also checks the TF_BUILD variable.

@ehuss
Copy link
Contributor

ehuss commented May 28, 2021

So does cargo, see is_ci. Sorry I didn't mention all the details. GitHub Actions, GitLab CI, and CircleCI also sets CI.

@Luchanso
Copy link

@ehuss CI=1 and CARGO_TERM_PROGRESS_WHEN=never don't indicate anything (or does not work as expected) for updating crates.io index:


image
image
image

Maybe add some indicator?
Example for reproduce in Docker (you will understand problem, when run docker build .):

FROM rust:latest

ENV CARGO_TERM_PROGRESS_WHEN=never
ENV CI=1

WORKDIR /usr/src/myapp

RUN cargo new --bin myapp && \
  cd myapp && \
  cargo add rand_core

CMD ["myapp"]

@weihanglo
Copy link
Member

Thank you, @Luchanso. That is an interesting idea. There are already some discussion around console output happening. You can check the label A-console-output Area: Terminal output, colors, progress bar, etc. to get a glimpse of them.

As your proposal seems like a new feature request, to catch everyone's eyes, I would suggest commenting on a relevant open issue or creating your own, rather than replying under a closed issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-console-output Area: Terminal output, colors, progress bar, etc.
Projects
None yet
Development

No branches or pull requests

8 participants