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

CmdLineArgs: Determine whether to color output based on TERM variable #13486

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Jul 19, 2024

To avoid cluttering up the output with ANSI escape sequences in terminals that do not support them (e.g. Xcode), this adds a heuristic that checks the TERM environment variable.

@fwcd fwcd added the ios label Jul 19, 2024
@fwcd fwcd mentioned this pull request Jul 19, 2024
54 tasks
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I have the same issue when watching the console output in Eclipse. So I don't think this a compile time issue. Can we detect it at runtime?

@daschuer
Copy link
Member

Is the CMake output correct colored?
CMake uses this:
https://github.com/Kitware/CMake/blob/d146baa5561914f87cec6d27df9163bfa57614d9/Utilities/cmpdcurses/pdcurses/color.c
I have not jet understood to be honest.

@fwcd
Copy link
Member Author

fwcd commented Jul 19, 2024

CMake logs aren't shown in Xcode (at least I haven't found where). I think it's somewhat of an IDE-specific issue, but since iOS apps are rarely run outside of Apple's tools (Xcode/Console.app), I don't think it makes sense to use ANSI colors if those terminals don't support them.

@daschuer
Copy link
Member

How about the compiler output? is that colored?

@daschuer
Copy link
Member

@fwcd
Copy link
Member Author

fwcd commented Jul 19, 2024

IIRC Xcode hides the raw compiler output in a submenu, so I'd have to check whether that supports ANSI colors. The runtime console is different though.

@daschuer
Copy link
Member

clang seems also be able to detect color capability:

-f[no-]color-diagnostics

    This option, which defaults to on when a color-capable terminal is detected, controls whether or not Clang prints diagnostics in color.

@fwcd
Copy link
Member Author

fwcd commented Jul 19, 2024

I consider that to be a separate issue, this is about Mixxx' runtime logs. I agree that we might want to put more effort into detecting the terminal though. We might be able to detect if we run in Xcode via environment variables.

On the other hand, I think defaulting to non-colored output on iOS is probably safer since apps have little control over how and where the OS aggregates those logs.

@daschuer
Copy link
Member

@fwcd
Copy link
Member Author

fwcd commented Jul 20, 2024

Ah interesting, checking the TERM variable seems like a good option on Unix

@fwcd fwcd changed the base branch from main to 2.5 July 21, 2024 17:06
@fwcd fwcd changed the title CmdLineArgs: Disable colored logs on iOS CmdLineArgs: Determine whether to color output based on TERM variable Jul 21, 2024
@fwcd
Copy link
Member Author

fwcd commented Jul 21, 2024

I have updated the PR to determine whether a terminal supports colors based on the TERM variable. The implementation is effectively an adapted version of the LLVM check.

The PR also targets 2.5 now, since this is a rather minor fix that might be useful there too.

@fwcd
Copy link
Member Author

fwcd commented Jul 21, 2024

The sporadic macOS CI failures (e.g. this one) are a known issue btw: actions/runner-images#7522

Re-running the workflow usually fixes it, but of course that isn't ideal. For my m1xxx build I use a workaround which kills the XProtect process that supposedly interferes with cpack's DMG creation and retry the step a few times as a fallback, but of course that is still far from being a clean solution: https://github.com/fwcd/m1xxx/blob/36fd9a2a352a7a1d81435de5863dc34af6f2ac27/.github/workflows/build.yml#L221-L240

@fwcd fwcd requested a review from daschuer July 22, 2024 15:41
Comment on lines 41 to 44
return term == "ansi" || term == "cygwin" || term == "linux" ||
term.startsWith("screen") || term.startsWith("xterm") ||
term.startsWith("vt100") || term.startsWith("rxvt") ||
term.endsWith("color");
Copy link
Member

Choose a reason for hiding this comment

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

please also add alacritty and xterm-256color.

Copy link
Member Author

Choose a reason for hiding this comment

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

xterm-256color is already handled by the last condition. Is alacritty the full string or is it suffixed with -...color too?

Copy link
Member

Choose a reason for hiding this comment

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

It's the full string:

$ echo $TERM
alacritty
$

Copy link
Member

Choose a reason for hiding this comment

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

on second thought, let's invoke tput colors to check for color support instead. If the command is not available, we just don't use colors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's probably a cleaner solution indeed. Only downside is that we'd have to spawn a process at every launch.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should check for platforms where we can't spawn processes (Wasm/iOS) too, but otherwise that looks good I'd say. I'll have a look at integrating it later

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I'm not sure if this would work on Windows (though the TERM heuristic probably wouldn't either), @JoergAtGithub do you by chance know if there's tput there or if there is another way of checking for colors?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm unfamilar with this topic

Copy link
Member

@Holzhaus Holzhaus Jul 24, 2024

Choose a reason for hiding this comment

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

On Windows I think we need to call GetConsoleMode() and check if the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag is set.

Also see https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences.

Copy link
Member

Choose a reason for hiding this comment

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

This could be useful: build2/build2#312 (comment)

@daschuer
Copy link
Member

I am reluctant of using tput. tput is part of ncurses, and just a thin wrapper around a library call. The process call at the startup is kind of fragile and requires a good portion of error handling.

Instead let's follow CMake and use PDCurses that is for my understanding a wrapper around ncurses but works also for other targets. https://github.com/wmcbrine/PDCurses

https://github.com/Kitware/CMake/blob/d146baa5561914f87cec6d27df9163bfa57614d9/Utilities/cmpdcurses/pdcurses/color.c

@daschuer
Copy link
Member

daschuer commented Jul 25, 2024

@fwcd
Copy link
Member Author

fwcd commented Aug 18, 2024

Both pdcurses and ncurses require adding a dependency. tput requires spawning a process. While the custom TERM check is not particularly elegant, I'm not sure if it's worth going through the trouble over something as minor as this to support a tiny fraction of use cases that could easily be amended. The worst case of the current implementation is that we don't use colors on a terminal that might support it, but is that really an issue?

IMO we could look into something in the future, but I'd love to move my iOS branch closer to upstream to avoid having to patch around these minor things.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

I'm okay with merging this in the current state. I'm also weary of adding new dependencies.

For simplicity, we could consider opting for something like this in the future: http://bixense.com/clicolors/ so that users have a way to force ANSI colors in case the autodetection is not working.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I am also fine with the current state. Thank you.

@daschuer daschuer merged commit c6de1e1 into mixxxdj:2.5 Aug 19, 2024
13 checks passed
@fwcd fwcd deleted the disable-log-colors-ios branch August 19, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants