stage1 compiler emits color information during x.py build even when running with TERM=dumb

[nikomatsakis]

When I run x.py from inside emacs, everything works fine for the stage0 build. But errors that arise in the stage1 build always have color information. This is annoying since the emacs compilation window can't interpret ansi colors, so I get this garbage:

^[[0m^[[38;5;9m^[[1merror[E0503]^[[0m^[[0m^[[1m: cannot use `result` because it was mutably borrowed^[[0m
^[[0m   ^[[0m^[[0m^[[38;5;12m^[[1m--> ^[[0m^[[0mrustc/compiler_builtins_shim/../../libcompiler_builtins/src/float/add.rs:179:46^[[0m
^[[0m    ^[[0m^[[0m^[[38;5;12m^[[1m|^[[0m
^[[0m^[[38;5;12m^[[1m179^[[0m^[[0m ^[[0m^[[0m^[[38;5;12m^[[1m| ^[[0m^[[0m    if round_guard_sticky == 0x4 { result += result & one; }^[[0m
^[[0m    ^[[0m^[[0m^[[38;5;12m^[[1m| ^[[0m^[[0m                                   ^[[0m^[[0m^[[38;5;12m^[[1m----------^[[0m^[[0m^[[38;5;9m^[[1m^^^^^^^[[0m^[[0m^[[38;5;12m^[[1m------^[[0m
^[[0m    ^[[0m^[[0m^[[38;5;12m^[[1m| ^[[0m^[[0m                                   ^[[0m^[[0m^[[38;5;12m^[[1m|^[[0m^[[0m         ^[[0m^[[0m^[[38;5;9m^[[1m|^[[0m
^[[0m    ^[[0m^[[0m^[[38;5;12m^[[1m| ^[[0m^[[0m                                   ^[[0m^[[0m^[[38;5;12m^[[1m|^[[0m^[[0m         ^[[0m^[[0m^[[38;5;9m^[[1muse of borrowed `result`^[[0m
^[[0m    ^[[0m^[[0m^[[38;5;12m^[[1m| ^[[0m^[[0m                                   ^[[0m^[[0m^[[38;5;12m^[[1mborrow of `result` occurs here^[[0m
^[[0m    ^[[0m^[[0m^[[38;5;12m^[[1m| ^[[0m^[[0m   

It seems like the problem is this logic in rustbuild, which is a wee bit forceful:

rust/src/bootstrap/compile.rs

Lines 1004 to 1009 in a04b88d

	if stderr_isatty() && build.ci_env == CiEnv::None {
	// since we pass message-format=json to cargo, we need to tell the rustc
	// wrapper to give us colored output if necessary. This is because we
	// only want Cargo's JSON output, not rustcs.
	cargo.env("RUSTC_COLOR", "1");
	}

Not sure what's the best way to fix this. I'm surprised that stderr is coming up as a "tty" when running from inside the M-x compile shell in emacs, but for sure the TERM variable is dumb. Maybe we should respect that?

cc @Mark-Simulacrum

[nikomatsakis]

This patch solves the problem for me, but I don't know if it's the "right" thing to do:

    if stderr_isatty() && build.ci_env == CiEnv::None {
        let term = env::var("TERM").unwrap_or(String::new());
        if term != "dumb" {
            // since we pass message-format=json to cargo, we need to tell the rustc
            // wrapper to give us colored output if necessary. This is because we
            // only want Cargo's JSON output, not rustcs.
            cargo.env("RUSTC_COLOR", "1");
        }
    }

[Mark-Simulacrum]

Seems fine to me, I guess, but I'm also not sure whether it's the right thing to do -- maybe @BurntSushi happens to know if there's more detection other than "tty" that we should be doing? Is our detection faulty somehow, and that's why we're printing color when we shouldn't be?

Is Cargo doing the right thing by-default with TERM=dumb?

cc @killercup -- this is sort of in the CLI WG area (how do I detect whether color should be emitted?)

[BurntSushi]

From a behavioral standpoint, if TERM=dumb, then you definitely shouldn't emit colors. Ideally whatever color library you are using would do that detection automatically though? I don't have a lot of context on what the right patch is to the compiler though. IIRC, @alexcrichton recently did work in this area?

[Mark-Simulacrum]

In this case "library" is tty detection via copied code from somewhere -- https://github.com/rust-lang/rust/blob/master/src/bootstrap/compile.rs#L973-L994 -- which may be out of date and/or wrong.

[alexcrichton]

@BurntSushi to confirm, what would you expect to happen if TERM=dumb but --color=always is also passed?

[alexcrichton]

Er sorry, I talked a bit with @nikomatsakis and it turns out that rustbuild is passing --color=always, so it's not necessarily only TERM=dumb but rather that rustbuild is also passing --color=always to rustc which causes it to try to colorize things

[BurntSushi]

@alexcrichton Ah in that case I think I would expect the --colors=always to override any environment settings, generally under the principle of "CLI args override env vars." (This is consistent with GNU grep's behavior.)

[Mark-Simulacrum]

To better replicate Cargo's behavior it looks like we'll need to either depend on termcolor (which means a winapi dependency) on copy it's logic from here: https://docs.rs/termcolor/0.3.5/src/termcolor/lib.rs.html#132. I think it's fine to just copy the logic for now...

[alexcrichton]

Ok cool just making sure! In that case I think this is a rustbuild "bug" rather than any other component. Rustbuild will configure an environment variable here which is read by our wrapper rustc script here.

The problem is that rustbuild is passing --message-format json which by default captures stderr (as Cargo wants to wrap rustc's JSON), but we're actually only interested in Cargo's JSON messages, not rustc's json messages. Cargo invokes our wrapper with --message-format json but we actually remove that in our wrapper script. When Cargo invokes rustc with --message-format json though it's streaming both stdout/stderr for JSON, so rustc's stderr file descriptor is not a TTY.

All that put together means that when you run ./x.py build then rustc will never think it's attached to a TTY so it never automatically enables colors. We want colors most of the time, however, so rustbuild attempts to pass --color=always when it can to force rustc to think twice about not passing colors.

---

@Mark-Simulacrum I think this may be as simple as pulling in the atty crate?

[nikomatsakis]

I agree with @alexcrichton's analysis -- though egads what a hack. Anyway, the patch I write above hacks rustbuild.

[Mark-Simulacrum]

I think that patch is ~95% of what we want, though it may need to be expanded for Windows. I'm fine with just landing it though.

@Mark-Simulacrum I think this may be as simple as pulling in the atty crate?

I think our copied unix/windows logic today is sufficient; we need termcolor's logic on TERM=dumb and perhaps a few more things on windows. I personally feel like checking for TERM=dumb will hold us until someone comes by again with this problem at which point we'll probably just switch to termcolor.

[alexcrichton]

@Mark-Simulacrum ah yes indeed!

[nikomatsakis]

This issue only seems to have gotten worse. It now infects stage0 as well:

   Compiling rustc v0.0.0 (file:///home/nmatsakis/versioned/rust-5/src/librustc)
^[[0m^[[1m^[[38;5;9merror[E0428]^[[0m^[[0m^[[1m: the name `ProgramClause` is defined multiple times^[[0m
^[[0m   ^[[0m^[[0m^[[1m^[[38;5;12m--> ^[[0m^[[0mlibrustc/traits/mod.rs:342:1^[[0m
^[[0m    ^[[0m^[[0m^[[1m^[[38;5;12m|^[[0m
^[[0m^[[1m^[[38;5;12m327^[[0m^[[0m ^[[0m^[[0m^[[1m^[[38;5;12m| ^[[0m^[[0mpub struct ProgramClause<'tcx> {^[[0m
^[[0m    ^[[0m^[[0m^[[1m^[[38;5;12m| ^[[0m^[[0m^[[1m^[[38;5;12m------------------------------^[[0m^[[0m ^[[0m^[[0m^[[1m^[[38;5;12mprevious definition of the type `ProgramClause` here^[[0m
^[[0m^[[1m^[[38;5;12m...^[[0m
^[[0m^[[1m^[[38;5;12m342^[[0m^[[0m ^[[0m^[[0m^[[1m^[[38;5;12m| ^[[0m^[[0mpub struct ProgramClause<'tcx> {^[[0m
^[[0m    ^[[0m^[[0m^[[1m^[[38;5;12m| ^[[0m^[[0m^[[1m^[[38;5;9m^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[[0m^[[0m ^[[0m^[[0m^[[1m^[[38;5;9m`ProgramClause` redefined here^[[0m
^[[0m    ^[[0m^[[0m^[[1m^[[38;5;12m|^[[0m
^[[0m    ^[[0m^[[0m^[[1m^[[38;5;12m= ^[[0m^[[0m^[[1mnote^[[0m^[[0m: `ProgramClause` must be defined only once in the type namespace of this module^[[0m

[nikomatsakis]

Should I open a PR with this patch ?

[Mark-Simulacrum]

I've now done so in #49809.

[nikomatsakis]

sg, for now I'm using the patch locally...