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

Migrate from the `term` crate to `termcolor` #4162

Merged
merged 1 commit into from Jun 14, 2017

Conversation

@alexcrichton
Copy link
Member

commented Jun 13, 2017

The API of termcolor fits what the system gives us much more nicely and should
be well battle-tested from ripgrep. Additionally we don't really need huge
terminfo parsers, that wasn't every really the intention of the color support
here.

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2017

@rust-highfive rust-highfive assigned matklad and unassigned brson Jun 13, 2017

@rust-highfive

This comment has been minimized.

Copy link

commented Jun 13, 2017

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@matklad

This comment has been minimized.

Copy link
Member

commented Jun 13, 2017

Additionally we don't really need huge
terminfo parsers, that wasn't every really the intention of the color support
here.

Yay! Does that mean that I can trick Cargo to emit ansi-espace codes for colors on windows by just setting the TERM evn var?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2017

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2017

☔️ The latest upstream changes (presumably #4163) made this pull request unmergeable. Please resolve the merge conflicts.


pub fn get_verbose(&self) -> Verbosity {
self.verbosity
};

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

Indentation is off

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

Dammit, I've pressed "Add single comment" instead of "Start a review" again :(

What is more convenient for you as an author of pull request: a ton of single comments, or a single review?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jun 13, 2017

Author Member

I personally prefer a single review because it avoids the case where I start to replying to your comments halfway through your review, which I personally am not a fan of when I review!

@matklad
Copy link
Member

left a comment

Super! I absolutely love changes which reduce the number of lines, while leaving the tests mostly intact!

}
}
ColorChoice::Never
};

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

Looks like all the logic in this function be folded into a single match without explicit returns (apart from #[cfg] functions of course).

},
Err(_) => NoColor(out),
impl Choice {
fn to_color_choice(&self) -> ColorChoice {

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

This is purposefully different from termcolor logic, presumably to preserve previous behavior. It might be worthwhile to think about if we should expose auto/always/never/always_ansi from termcolor exactly.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jun 13, 2017

Author Member

The major difference I found was that termcolor doesn't consider whether stderr is a tty, meaning that if stderr is a pipe it'll still color by default (b/c of the TERM env var). Cargo's previous beahvior (which I'd like to preserve) is to avoid colorizing pipes.

Mind elaborating on what you're thinking though?

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

I am not an expert in these things, but if we and termcolor do different things then perhaps at least one is wrong :)

There's also a difference for windows, where the term color in always mode would output ansi escapes if console API is not available, but term is set. If we are happy with this behavior, we can avoid calling winapi functions here and let termcolor do this.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jun 13, 2017

Author Member

Oh I wouldn't necessarily say one is correct or one is wrong, perhaps just different use cases. In any case, though, cc @BurntSushi on this topic. This is a case where it appears that termcolor colorizes when stdout/stderr is a pipe but Cargo/rustc avoid colorization today. As a result there's a bit of wrapper logic for telling termcolor what to do, where if Cargo detects a pipe it passes down Never.

@matklad I'm thinking that force-ansi-on-windows would perhaps be a new flag we could add to --color, although as a future implementation detail.

This comment has been minimized.

Copy link
@BurntSushi

BurntSushi Jun 13, 2017

Member

Basically, termcolor punts on the issue of pipes vs ttys and instead leaves that up to the atty crate. The atty crate was updated recently to detect pipes "correctly" on Windows, including in both a native Windows console and in MSYS2 terminals.

ripgrep provides a --color flag which is a pretty straight-forward exposure of the termcolor::ColorChoice type, including an ansi option which forces ANSI escapes regardless. The logic has one small tweak for handling pipes (using atty) when auto is used, which is the default: https://github.com/BurntSushi/ripgrep/blob/f2d1c582a803a237924093be2f5e0ee45e8782ee/src/args.rs#L666-L686

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jun 14, 2017

Author Member

So sounds like there's a future refactoring to use atty here, but otherwise not much difference? What do you think @matklad?

This comment has been minimized.

Copy link
@matklad

matklad Jun 14, 2017

Member

Sounds reasonable @alexcrichton ! I didn't see initially that ripgrep uses atty higher in the stack.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jun 14, 2017

Author Member

I've filed #4166 to track the usage of atty

err: Shell,
verbosity: Verbosity
#[derive(PartialEq, Clone, Copy)]
pub enum Choice {

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

I find name Choice a bit too generic. I'd personally named it ColorChoice and used termcolor::ColorChoice for the other one.

pub enum Choice {
Always,
Never,
CargoAuto,
}

impl MultiShell {

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

An excellent observation that we can strip all the multishell complexity because we are only using (and should be using) only stderr anyways! However, perhaps MultiShell can be renamed to simply a Shell?

@@ -461,7 +461,7 @@ pub fn install_list(dst: Option<&str>, config: &Config) -> CargoResult<()> {
let dst = metadata(config, &dst)?;
let list = read_crate_list(dst.file())?;
let mut shell = config.shell();
let out = shell.out();
let out = shell.err();

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

This could in theory break some scripts, and it is also now inconsistent with cargo --list which prints to stdout. I think that we can do just println here, like cargo --list does.

@@ -84,9 +84,7 @@ fn simple() {
assert_that(cargo_process("search").arg("postgres")
.arg("--host").arg(registry().to_string()),
execs().with_status(0)
.with_stderr("\
[UPDATING] registry `[..]`")

This comment has been minimized.

Copy link
@matklad

matklad Jun 13, 2017

Member

Hm, looks like these [UPDATING] lines should still be present in the output somewhere?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jun 13, 2017

Author Member

Ah I just changed the clauses later to with_stderr_contains to avoid matching this output, which we in theory didn't really need to match anyway.

@alexcrichton alexcrichton force-pushed the alexcrichton:termcolor branch from ca41f8a to c88d8d7 Jun 13, 2017

Migrate from the `term` crate to `termcolor`
The API of `termcolor` fits what the system gives us much more nicely and should
be well battle-tested from ripgrep. Additionally we don't really need huge
terminfo parsers, that wasn't every really the intention of the color support
here.

@alexcrichton alexcrichton force-pushed the alexcrichton:termcolor branch from c88d8d7 to f8fb0a0 Jun 13, 2017

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2017

Updated!

@matklad

This comment has been minimized.

Copy link
Member

commented Jun 14, 2017

bors r+

@matklad

This comment has been minimized.

Copy link
Member

commented Jun 14, 2017

Sigh, the wrong bors.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

📌 Commit f8fb0a0 has been approved by matklad

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

⌛️ Testing commit f8fb0a0 with merge d917378...

bors added a commit that referenced this pull request Jun 14, 2017
Auto merge of #4162 - alexcrichton:termcolor, r=matklad
Migrate from the `term` crate to `termcolor`

The API of `termcolor` fits what the system gives us much more nicely and should
be well battle-tested from ripgrep. Additionally we don't really need huge
terminfo parsers, that wasn't every really the intention of the color support
here.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

📌 Commit f8fb0a0 has been approved by matklad

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing d917378 to master...

@bors bors merged commit f8fb0a0 into rust-lang:master Jun 14, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:termcolor branch Jun 14, 2017

@matklad matklad referenced this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.