Always cache colored diagnostics; strip color codes from ccache's stderr output as appropriate by whitslack · Pull Request #596 · ccache/ccache

whitslack · 2020-05-22T02:55:51Z

This pull request resolves #223 and resolves #224.

Implement function to strip ANSI escape sequences from a string.
- Implement a test case of the preceding.
Interpret any colored diagnostics options given to ccache as ccache options, not as compiler options.
Always pass {GCC: -fdiagnostics-color} {Clang: -fcolor-diagnostics} to the real compiler, except…
- Don't pass any color option to the preprocessor if run_second_cpp is true.
- {GCC: If the initial GCC invocation fails and its emitted error message contains unrecognized command-line option and -fdiagnostics-color, then remove the color option and retry the command. Also, remember the failure and omit the color option from any subsequent GCC invocation.}
When copying cached stderr output to ccache's stderr, if the invoking command specified {GCC: -fno-diagnostics-color or -fdiagnostics-color=never} {Clang: -fno-color-diagnostics} or ccache's stderr is not a TTY and the invoking command did not specify {GCC: -fdiagnostics-color or -fdiagnostics-color=always} {Clang: -fcolor-diagnostics}, then use the aforementioned new function to strip ANSI CSI actions K and m.
Implement a test case to demonstrate that color codes are always stored in the cache, regardless of any color options given to ccache, and that color codes are stripped from ccache's stderr output when suppression of colored diagnostics is requested by explicit option given to ccache or when stderr is not a TTY and no explicit color option is given to ccache.
Remove the caveat that was documented in 67fdd3b.

liljenzin · 2020-05-22T09:16:03Z

@whitslack: See my comment here: #224 (comment)

See: ccache#596

This function works like copy_fd but strips out selected ANSI CSI escape sequences. The intention is for this function to be called instead of copy_fd when copying cached compiler stderr output to ccache's stderr when the invoking command specifies -fno-diagnostics-color or -fdiagnostics-color=never or ccache's stderr is not a terminal and the invoking command does not specify -fdiagnostics-color or -fdiagnostics-color=always.

See: ccache#596

Testing -f fails when the file is not a regular file, such as when using Bash's process substitution.

See: ccache#596

test/suites/color_diagnostics.bash

Merge remote-tracking branch 'origin/master' into cache-colored-diagnostics

src/argprocessing.cpp

test/suites/color_diagnostics.bash

src/legacy_util.cpp

src/ccache.cpp

src/legacy_util.cpp

test/run

Quoting Joel Rosdahl: > I intend to relatively soon refactor the code related to stderr > handling so that the stderr data read from the aggregated result file > won't hit the file system as a temporary file. Therefore, stripping > color codes from data read explicitly from a file descriptor does not > quite fit my plans. See: ccache#596 (comment)

See: ccache#596 (comment)

Per request by Joel Rosdahl. See: ccache#596 (comment) Co-authored-by: Joel Rosdahl <joel@rosdahl.net>

The --log-out option was added to script(1) in util-linux 2.35. Since some distributions are still running older versions, remove the use of the option from the color_diagnostics test suite. Equivalent functionality is obtained simply by passing the name of the output file without using the --log-out option. See: ccache#596 (comment)

According to Joel Rosdahl, the intention in 3eb2483 was not to support regular expressions. This commit makes expect_file_{,not_}contains perform literal substring matching only. See: ccache#596 (comment)

“stderr” is a macro in MinGW and cannot be used as a variable name.

If the user running the test suite sets GCC_COLORS in their shell rc file the color_diagnostics tests fail. Fix this by explicitly unsetting GCC_COLORS where needed.

jrosdahl · 2020-06-06T19:50:29Z

Good work.

There are still some CI test failures and I've fixed one of them in e13badd. I also fixed an issue I had locally in b1bd4b7. Feel free to rework them if you like.

There's however one CI test issue left, a unit test segfault in the "Build on Linux with native Clang" job. Disabling the unit test makes the ccache binary segfault in the bash test suite later. There seems to be some bad interaction between Clang 7.0.0, nonstd::string_view and std::regex in the Travis-CI environment. I can't reproduce the problem locally with Clang 7.0.1 on Ubuntu 20.04.

I modified the build to run the unit test with Valgrind to get a backtrace. It looks like this: https://travis-ci.org/github/jrosdahl/ccache/builds/695447083#L470 (followed by a crash further down). Rewriting {edit,strip}_ansi_csi_seqs to work on const std::string& instead of nonstd::string_view fixes the issue. Building without -O2 also makes the issue go away.

The CI setup currently uses the Ubuntu 16.04. I spent some time to upgrade it (not yet on master) to Ubuntu 18.04 (I guess it's time for that anyway) and that fixes the Clang build, but doing so leaves the Ubuntu 16.04 Clang issue unsolved, which feels slightly uncomfortable.

Here are some ways forward:

Dig deeper and try to find the root cause. I don't feel like I have time to do that.
Rewrite to use const std::string& (easy). That would be fine by me.
Rewrite to not use std::regex (more work). That would be fine by me.
Disable the Clang build. I would rather not do that.
Ignore the problem and upgrade to Ubuntu 18.04. I guess that would be OK.

@whitslack: Any ideas or opinions on this?

jrosdahl · 2020-06-07T13:41:03Z

I've updated the CI environment to Ubuntu 18.04 on master now so I'll merge this.

This fixes the Clang-Format CI job.

jrosdahl · 2020-06-07T13:43:41Z

Thanks!

whitslack · 2020-06-07T21:17:22Z

Any ideas or opinions on this?

@jrosdahl: Moot at this point, but my opinion is that neither runtime efficiency nor code legibility/maintainability should be sacrificed to cater to deficient compilers that are obsolete by several major releases. I'm glad you went with upgrading your CI environment. I doubt Ubuntu 16.04 will ever package ccache 4.0 anyway.

jrosdahl · 2020-06-08T18:29:49Z

To clarify, this is not about dropping support for Ubuntu 16.04 (which comes with Clang 3.8) or Clang 7 (which is not even 2 years old). It's about if the Clang 7 installation on Travis-CI's Ubuntu 16.04 is buggy or if there's a lurking bug in ccache (or nonstd::string_view). I'm hoping for the former. Edit: And it's also not about if Ubuntu 16.04 will package ccache 4.0 (which they of course won't) but about supporting users who want to compile and use ccache themselves on legacy systems.

On the subject of portability, I want to continue supporting CentOS 7 which has GCC 4.8.5 which I now realize doesn't implement std::regex. I'll think some more about this, but it might end up in not using std::regex. No shadow falls in you, of course. 🙂

The implementation from ccache#596 uses the spelling “command-line” instead of “command line” (which is what is actually emitted by GCC and specified in ccache#224).

The CCACHE_PREFIX test logs execution of prefixes. After 1c6ccf1 (ccache#596) the compiler and prefixes can be run twice if GCC doesn’t support -fdiagnostics-color and the test case is not prepared for that. Fix this by letting prefix-a truncate the log file.

The CCACHE_PREFIX test logs execution of prefixes. After 1c6ccf1 (#596) the compiler and prefixes can be run twice if GCC doesn’t support -fdiagnostics-color and the test case is not prepared for that. Fix this by letting prefix-a truncate the log file.

whitslack added a commit to whitslack/ccache that referenced this pull request May 25, 2020

Always request colored diagnostics from compiler; strip output as needed

2fa45a3

See: ccache#596

whitslack force-pushed the cache-colored-diagnostics branch from 54e0c70 to 2fa45a3 Compare May 25, 2020 19:48

whitslack added a commit to whitslack/ccache that referenced this pull request May 28, 2020

Add color_diagnostics test suite

1539969

See: ccache#596

whitslack added a commit to whitslack/ccache that referenced this pull request May 28, 2020

doc: delete caveat about colored diagnostics messages

414f3ce

See: ccache#596

whitslack added a commit to whitslack/ccache that referenced this pull request May 28, 2020

doc: add a NEWS item for the new colored diagnostics behavior

9cb24d4

See: ccache#596

whitslack marked this pull request as ready for review May 28, 2020 22:57

whitslack added 7 commits May 28, 2020 18:58

Always request colored diagnostics from compiler; strip output as needed

69b6f5c

See: ccache#596

test/run: test -e for existence, not -f

e83deac

Testing -f fails when the file is not a regular file, such as when using Bash's process substitution.

test/run: add expect_file_not_contains()

a9dda38

Add color_diagnostics test suite

d407519

See: ccache#596

doc: delete caveat about colored diagnostics messages

03072e0

See: ccache#596

doc: add a NEWS item for the new colored diagnostics behavior

d19cd72

See: ccache#596

whitslack force-pushed the cache-colored-diagnostics branch from 9cb24d4 to d19cd72 Compare May 28, 2020 23:02

whitslack commented May 28, 2020

View reviewed changes

test/suites/color_diagnostics.bash Outdated Show resolved Hide resolved

whitslack mentioned this pull request May 28, 2020

Make colored warnings work for GCC out of the box #224

Closed

doc/NEWS.adoc: fix merge conflict

19874d7

Merge remote-tracking branch 'origin/master' into cache-colored-diagnostics

jrosdahl reviewed Jun 3, 2020

View reviewed changes

whitslack and others added 7 commits June 4, 2020 15:00

argprocessing: Use enum class for tri-state color_diagnostics

9f24dc6

See: ccache#596 (comment)

ccache.cpp: combine nested if statements

bf01065

Per request by Joel Rosdahl. See: ccache#596 (comment) Co-authored-by: Joel Rosdahl <joel@rosdahl.net>

test/run: expect_file_{,not_}contains: do not support regexps

163bf67

According to Joel Rosdahl, the intention in 3eb2483 was not to support regular expressions. This commit makes expect_file_{,not_}contains perform literal substring matching only. See: ccache#596 (comment)

Fix MinGW-w64 cross compilation

e13badd

“stderr” is a macro in MinGW and cannot be used as a variable name.

Make color_diagnostics tests robust against GCC_COLORS

b1bd4b7

If the user running the test suite sets GCC_COLORS in their shell rc file the color_diagnostics tests fail. Fix this by explicitly unsetting GCC_COLORS where needed.

Merge branch 'master' into pr-596

5498720

Run clang-format

c0044cf

This fixes the Clang-Format CI job.

jrosdahl merged commit 1c6ccf1 into ccache:master Jun 7, 2020

jrosdahl added the issue: feature New feature label Aug 6, 2020

jrosdahl added this to the 4.0 milestone Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always cache colored diagnostics; strip color codes from ccache's stderr output as appropriate#596

Always cache colored diagnostics; strip color codes from ccache's stderr output as appropriate#596
jrosdahl merged 17 commits intoccache:masterfrom
whitslack:cache-colored-diagnostics

whitslack commented May 22, 2020 •

edited

Loading

Uh oh!

liljenzin commented May 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrosdahl commented Jun 6, 2020

Uh oh!

jrosdahl commented Jun 7, 2020

Uh oh!

jrosdahl commented Jun 7, 2020

Uh oh!

whitslack commented Jun 7, 2020

Uh oh!

jrosdahl commented Jun 8, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

whitslack commented May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liljenzin commented May 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrosdahl commented Jun 6, 2020

Uh oh!

jrosdahl commented Jun 7, 2020

Uh oh!

jrosdahl commented Jun 7, 2020

Uh oh!

whitslack commented Jun 7, 2020

Uh oh!

jrosdahl commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

whitslack commented May 22, 2020 •

edited

Loading

jrosdahl commented Jun 8, 2020 •

edited

Loading