Skip to content

driver: Use atty instead of rolling our own - #80897

Merged
bors merged 1 commit into
rust-lang:masterfrom
camelid:atty
Jan 12, 2021
Merged

driver: Use atty instead of rolling our own#80897
bors merged 1 commit into
rust-lang:masterfrom
camelid:atty

Conversation

@camelid

@camelidcamelid commented Jan 11, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#80888.

Rationale:

  • atty is widely used in the Rust ecosystem
  • We already use it (in rustc_errors and other places)
  • We shouldn't be rolling our own TTY detector when there's a
    widely-used, well-tested package that we can use

@camelidcamelid added A-driver Area: rustc_driver that ties everything together into the `rustc` compiler C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 11, 2021
@rust-highfive

rust-highfive commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

r? @ecstatic-morse

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2021
@camelid

camelid commented Jan 11, 2021

Copy link
Copy Markdown
MemberAuthor

r? @jyn514 maybe?

@camelidcamelid added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jan 11, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
camelid marked this pull request as draft January 11, 2021 02:18
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid

camelid commented Jan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Forgot to update Cargo.lock 🤦

jyn514
jyn514 reviewed Jan 11, 2021
View reviewed changes
Comment threadcompiler/rustc_driver/src/lib.rs Outdated

@jyn514jyn514Jan 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also use atty in bootstrap and librustc_errors?

@camelidcamelidJan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find the duplicates it's referring to. rustc_errors seems to use atty already, and I don't see anything in bootstrap. If you know of a spot that has a duplicate, I'm happy to update it!

@jyn514jyn514Jan 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found library/test/src/helpers/isatty.rs, but it might be tricky to convert that one since it's part of the runtime. This seems fine for now.

@camelidcamelidJan 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on a Git blame, these functions were added way back in #42732. Initially that PR added the dependency isatty, but Mark-Simulacrum requested that it be removed in #42732 (comment) since it depended on something with long build times.

@jyn514

jyn514 commented Jan 12, 2021

Copy link
Copy Markdown
Member

@camelid#80887 was merged :)

@jyn514jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2021
@camelid

camelid commented Jan 12, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks for reminding me!

@camelid
driver: Use atty instead of rolling our own
8c43160
Rationale:
- `atty` is widely used in the Rust ecosystem
- We already use it (in `rustc_errors` and other places)
- We shouldn't be rolling our own TTY detector when there's a
widely-used, well-tested package that we can use
@camelid

camelid commented Jan 12, 2021

Copy link
Copy Markdown
MemberAuthor

Rebased.

@camelidcamelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2021
@camelid
camelid marked this pull request as ready for review January 12, 2021 04:01
@jyn514

jyn514 commented Jan 12, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 8c43160 has been approved by jyn514

@borsbors 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 Jan 12, 2021
@JohnTitorJohnTitor mentioned this pull request Jan 12, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2021
@bors
Auto merge ofrust-lang#80939- JohnTitor:rollup-pymns4q, r=JohnTitor
b6b4616
Rollup of 8 pull requests
Successful merges:
- rust-lang#79757 (Replace tabs earlier in diagnostics)
- rust-lang#80600 (Add `MaybeUninit` method `array_assume_init`)
- rust-lang#80880 (Move some tests to more reasonable directories)
- rust-lang#80897 (driver: Use `atty` instead of rolling our own)
- rust-lang#80898 (Add another test case for rust-lang#79808)
- rust-lang#80917 (core/slice: remove doc comment about scoped borrow)
- rust-lang#80927 (Replace a simple `if let` with the `matches` macro)
- rust-lang#80930 (fix typo in trait method mutability mismatch help)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit d682a68 into rust-lang:masterJan 12, 2021
@rustbotrustbot added this to the 1.51.0 milestone Jan 12, 2021
@camelid
camelid deleted the atty branch January 12, 2021 20:14
@jyn514jyn514 mentioned this pull request Jan 23, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@jyn514jyn514jyn514 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jyn514jyn514

Labels

A-driverArea: rustc_driver that ties everything together into the `rustc` compilerC-cleanupCategory: PRs that clean code up or issues documenting cleanup.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.51.0

Development

Successfully merging this pull request may close these issues.

Use atty crate instead of rolling our own

7 participants

@camelid@rust-highfive@rust-log-analyzer@jyn514@bors@ecstatic-morse@rustbot