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 report future-incompatibilities doesn't check for color support correctly #9960

Closed
ehuss opened this issue Oct 4, 2021 · 5 comments · Fixed by #10024
Closed

cargo report future-incompatibilities doesn't check for color support correctly #9960

ehuss opened this issue Oct 4, 2021 · 5 comments · Fixed by #10024
Assignees
Labels
A-console-output Area: Terminal output, colors, progress bar, etc. A-future-incompat Area: future incompatible reporting C-bug Category: bug

Comments

@ehuss
Copy link
Contributor

ehuss commented Oct 4, 2021

Problem
cargo report future-incompatibilities does not check for color support correctly. If you pipe the output to a file or something like less, it is not stripping the colors. This wasn't quite the intended behavior in #9606. The issue is here where it only checks if stderr supports color.

Steps

  1. cargo report future-incompatibilities > some_file

Notice that some_file contains lots of escape characters

Possible Solution(s)

I'm not entirely sure of the best approach to fix this. Perhaps just check if both stdout and stderr support color at the line noted above? I can't imagine a scenario where that would matter. I'd be reluctant to make this too fussy or complicated, though worst case the check to call strip_ansi_escapes::strip could be moved to the callers, but I think I'd rather avoid that.

Notes

Output of cargo version:

cargo 1.57.0-nightly (d56b42c 2021-09-27)

@ehuss ehuss added C-bug Category: bug A-console-output Area: Terminal output, colors, progress bar, etc. A-future-incompat Area: future incompatible reporting labels Oct 4, 2021
@dswij
Copy link
Member

dswij commented Oct 5, 2021

I might be missing something here, but since cargo report future-incompatibilities command prints to stdout, what's the reason behind checking stderr only?

@ehuss
Copy link
Contributor Author

ehuss commented Oct 5, 2021

That code is shared for two different places. When displaying as part of a regular build (here) it is sent to stderr (since all build errors/warnings go to stderr). When displayed as part of the cargo report subcommand, it is displayed to stdout as it is intended to be a human-readable text report, and it seemed more natural to send that to stdout (and more convenient to pipe the output).

@dswij
Copy link
Member

dswij commented Oct 7, 2021

I'd say that additionally checking stdout would probably be a good enough approach since this should not be too complicated.

I'll try to have a look at it 🙂 @rustbot claim

@ehuss
Copy link
Contributor Author

ehuss commented Oct 30, 2021

@dswij Just checking if you've had a chance to look at this, or had any questions.

@dswij
Copy link
Member

dswij commented Nov 1, 2021

I've had a look, but got distracted along the way. I'll continue working on it once I have the chance to.

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. A-future-incompat Area: future incompatible reporting C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants