Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Disable output colorization on terminals that don't support it #318

Closed
wants to merge 1 commit into from

Conversation

fhars
Copy link

@fhars fhars commented Apr 6, 2022

The current version of the colored crate ignores the capabilities of
the terminal (see colored-rs/colored#108),
which makes the outupt of probe-run unparseable in e.g. emacs
compilation mode. As a temporary workaround, this patch overrides
colorization if probe-run is called from a terminal without color
support.

The current version of the colored crate ignores the capabilities of
the terminal (see colored-rs/colored#108),
which makes the outupt of probe-run unparseable in e.g. emacs
compilation mode. As a temporary workaround, this patch overrides
colorization if probe-run is called from a terminal without color
support.
Copy link
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

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

Hi @fhars, thank you for the patch. I tested it and it seems to work as expected. Can you please add the Cargo.lock file as well?

I have two concerns regarding the added terminfo dependency:

  1. The maintenance of it seems to be currently unclear. See Maintainance of this crate meh/rust-terminfo#20. There are a few people interested in taking over maintenance, but it seems the handover didn't happen yet.
  2. It adds outdated versions for a few dependencies, which some of them are already part of our dependency tree. This partially like a symptom of point 1.

I think it is reasonable to merge this PR anyways, but we should keep an eye on that and if it stays unmaintained eventually look for an alternative crate.

@jonas-schievink
Copy link
Contributor

Does emacs' compilation mode present itself as a terminal (via isatty) to the program (I do hope we support non-isatty mode properly already, so perhaps it doesn't)? Does it set something like TERM=dumb that we could detect instead?

@Urhengulas
Copy link
Member

Does emacs' compilation mode present itself as a terminal (via isatty) to the program (I do hope we support non-isatty mode properly already, so perhaps it doesn't)? Does it set something like TERM=dumb that we could detect instead?

From reading colored-rs/colored#108 I understood that it is setting TERM=dumb. But I don't know if this is the only trigger we want to look out for, or if there is other ones as well?

@Urhengulas
Copy link
Member

@fhars Since it is a temporary fix until colored handles it properly, would it be enough for your purposes to only check TERM=dumb and disable coloring then?

@fhars
Copy link
Author

fhars commented Apr 26, 2022

That would fix the immediate use case of running it in a emacs compilation buffer, but the terminfo database on my system knows about 1332 kinds of terminal that do not have color support. (Although most of these are probably collectors items by now, it's decades since I have seen an operational tek4013).

And given that colored seems to be in deep hibernation for the last years, nothing may last as long as a stopgap measure.

bors bot added a commit that referenced this pull request May 9, 2022
320: Disable terminal colorization if `TERM=dumb` is set r=jonas-schievink a=Urhengulas

This PR fixes the problem `@fhars` described in #318:
> The current version of the colored crate ignores the capabilities of the terminal (see colored-rs/colored#108), which makes the outupt of probe-run unparseable in e.g. emacs compilation mode.

It also supersedes #318 as a fix to the problem. The reason for not going with #318 is that it adds the `terminfo` crate as a dependency which seems unmaintained and with many outdated dependencies.

Co-authored-by: Urhengulas <johann.hemmann@code.berlin>
@Urhengulas
Copy link
Member

Superseded by #320

@Urhengulas Urhengulas closed this May 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants