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.

Sign up for GitHub

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

Jump to bottom

future-incompat-report checks both stdout and stderr for color support #10024

Merged
merged 1 commit into from
Nov 15, 2021
Merged

future-incompat-report checks both stdout and stderr for color support #10024

merged 1 commit into from
Nov 15, 2021

Conversation

dswij
Copy link
Member

@dswij dswij commented Nov 1, 2021 •
edited
Loading

Closes #9960

@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 1, 2021
@dswij
Copy link
Member Author

dswij commented Nov 1, 2021 •
edited
Loading

Not sure how to test it locally, I tried using the one described in rust-lang/rust#71249 (comment), but it produces no reports are currently visible

@ehuss can you help to give some directions?

@ehuss
Copy link
Contributor

ehuss commented Nov 1, 2021

Can you say more about exactly what you tried? You have to use the -Z flag as in cargo build -Zfuture-incompat-report to populate the report (and make sure to clear target if you previously ran without it).

@dswij
Copy link
Member Author

dswij commented Nov 2, 2021 •
edited
Loading

Oh, works fine for me after clearing target now. I'd have thought that adding the dependency then running build with -Z flag directly would've worked. Thanks!

@dswij dswij marked this pull request as ready for review November 2, 2021 11:27
@ehuss
Copy link
Contributor

ehuss commented Nov 2, 2021

I see that you have marked this ready for review. However, there appears to be several CI errors?

Also, can you say why you added the atty check? That doesn't seem correct to me.

@dswij
Copy link
Member Author

dswij commented Nov 3, 2021 •
edited
Loading

I see that you have marked this ready for review. However, there appears to be several CI errors?

Also, can you say why you added the atty check? That doesn't seem correct to me.

termcolor::StandardStream does not really support checking for piping, while atty check is really great for this specific purpose. FWIU, err_supports_color() is used only before output, so I don't see a concern using atty check specifically for color support. I do think that the atty check does not belong on Shell struct, so I think we can factor it out.

That being said, I just remembered that atty check does not play nicely with CI, so it's breaking on colored output tests.

@ehuss
Copy link
Contributor

ehuss commented Nov 5, 2021

I think there may be some confusion. Cargo has its own tty checking here and here. However, it only checks for stderr as a tty. I don't think we want to change the supports_color function since that already handles TTY, and also handles cli and environment color settings.

I'm not sure exactly how to structure it, but I think the stdout checks will likely need to mirror the existing stderr tty checks.

@dswij
Copy link
Member Author

dswij commented Nov 7, 2021

Ah, you're right, I missed the atty check on stderr.

@ehuss
Copy link
Contributor

ehuss commented Nov 12, 2021

Thanks! Would you mind squashing to one commit?

@ehuss ehuss assigned ehuss and unassigned Eh2406 Nov 12, 2021
@dswij
Copy link
Member Author

dswij commented Nov 15, 2021

Done 🙂

@ehuss
Copy link
Contributor

ehuss commented Nov 15, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 15, 2021

📌 Commit 622b43a has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2021
@bors
Copy link
Contributor

bors commented Nov 15, 2021

⌛ Testing commit 622b43a with merge d2cc298...

@bors
Copy link
Contributor

bors commented Nov 15, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing d2cc298 to master...

@bors bors merged commit d2cc298 into rust-lang:master Nov 15, 2021
@dswij dswij deleted the 9960 branch November 16, 2021 09:51
@ehuss ehuss mentioned this pull request Nov 17, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2021
@bors
Auto merge of rust-lang#90991 - ehuss:update-cargo, r=ehuss
6414e0b 
Update cargo

11 commits in 2e2a16e983f597da62bc132eb191bc3276d4b1bb..ad50d0d266213e0cc4f6e526a39d96faae9a3842
2021-11-08 15:13:38 +0000 to 2021-11-17 18:36:37 +0000
- Warn when alias shadows external subcommand (rust-lang/cargo#10082)
- Implement escaping to allow clean -p to delete all files when directory contains glob characters (rust-lang/cargo#10072)
- Match any error when failing to find executables (rust-lang/cargo#10092)
- Enhance error message for target auto-discovery (rust-lang/cargo#10090)
- Include note about bug while building on macOS in mdbook (rust-lang/cargo#10073)
- Improve the help text of the --quiet args for all commands (rust-lang/cargo#10080)
- `future-incompat-report` checks both stdout and stderr for color support (rust-lang/cargo#10024)
- Remove needless borrow to make clippy happy (rust-lang/cargo#10081)
- Describe the background color of the timing graph (rust-lang/cargo#10076)
- Make ProfileChecking comments a doc comments (rust-lang/cargo#10077)
- Fix test: hash value depends on endianness and bitness. (rust-lang/cargo#10011)
@ehuss ehuss added this to the 1.58.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.

cargo report future-incompatibilities doesn't check for color support correctly
5 participants
@dswij @rust-highfive @ehuss @bors @Eh2406