Extend $SYSTEMD_COLORS to switch colors mode #17702

rnhmjoj · 2020-11-23T14:41:46Z

Description

This is my attempt at solving #14495 and #13561 trying to make everyone happy.
The idea, initially proposed in #13561 (comment) is to make SYSTEMD_COLOR be 0, 16, or 256 to mean no colors, base 16 ANSI colors or 256 colors respectively.
This way if, you don't like the hardcoded yellow/blue, you can just export SYSTEMD_COLOR=16 and use your own palette.

Things done:

Read the contributing and style guide
Tested journalctl and other programs follow SYSTEMD_COLOR
Tested the default colors are unchanged
Run tests
Write better documentation/release notes
Get feedback

Questions

A bunch of tests is failing, but is already failing on master. I'm not sure whether my machine is at fault but seems unrelated to my changes. Can you confirm this?

322/602 udev-test                                 TIMEOUT        180.01s
322/602 udev-test                                 TIMEOUT        180.01s
329/602 test-repart                               SKIP           0.02s
357/602 test-boot-timestamps                      SKIP           0.07s
374/602 test-format-table                         FAIL           0.63s (killed by signal 6 SIGABRT)
374/602 test-format-table                         FAIL           0.63s (killed by signal 6 SIGABRT)
401/602 test-string-util                          FAIL           1.33s (killed by signal 6 SIGABRT)
401/602 test-string-util                          FAIL           1.33s (killed by signal 6 SIGABRT)
417/602 test-barrier                              SKIP           0.13s
429/602 test-bpf-devices                          SKIP           0.04s
451/602 test-sleep                                SKIP           0.13s

As of 18f3bc9, setting SYSTEMD_COLORS overrides all other checks: this implies that escape sequences will be printed even in a pipeline. For example try env SYSTEMD_COLORS=1 systemd-analyze --help | less.
Is this intended?
Should I write a test for getenv_int?

cc: @keszybz who liked my proposal

keszybz · 2020-11-30T09:48:42Z

setting SYSTEMD_COLORS overrides all other checks: this implies that escape sequences will be printed even in a pipeline. For example try env SYSTEMD_COLORS=1 systemd-analyze --help | less.
Is this intended?

I'd say yes. The envvars are supposed to override autodetection.
Essentially, this is a shortcoming of less. It should just display colors without making any fuss about it. Less 568 started doing that for url shortcuts (gwsw/less#54), and at this point it makes even less sense to refuse to display colors similarly. https://bugzilla.redhat.com/show_bug.cgi?id=1258741 is a bug from 2015 where people came to the same conclusion.

A bunch of tests is failing, but is already failing on master. I'm not sure whether my machine is at fault but seems unrelated to my changes.

No, the failures are unexpected. It is possible that your environment (in particular envvars) is confusing the tests or exposing some bug. It needs to be resolved though.

keszybz

I like this approach. But backwards compatibility needs to be preserved, so that things that would be parsed as true before are still parsed as such and cause the optimum color pallete to be enabled. Only select numbers like 256 and 16 should be treated as unique. (Strictly speaking, this too is a break of compatibility, since "256" would be treated as true before, but let's assume that people are unlikely to have used such specific values.) Other numbers should be treated the same as "1" too.

src/basic/env-util.c

man/less-variables.xml

rnhmjoj · 2020-11-30T10:03:56Z

No, the failures are unexpected. It is possible that your environment (in particular envvars) is confusing the tests or exposing some bug. It needs to be resolved though.

The environment is as clean as it can get: I've made a very minimal Arch Linux installation into a qemu qcow2 image and I have been running it with qemu-kvm. Maybe the VM is causing the failures.

keszybz · 2020-11-30T10:12:49Z

Maybe the VM is causing the failures.

That's unlikely. How are the tests failing exactly? Did you try running them by hand from the command line:

$ build/test-format-table
$ build/test-string-util

rnhmjoj · 2020-11-30T13:31:33Z

This is the output of the failed tests:

Tests output

[root@archlinux systemd]# build/test-format-table
ONE          TWO THREE
xxx          yyy   yes
a long field YYY    no
ONE                TWO             THREE

xxx                yyy               yes

a long field       YYY                no
ONE TWO T...

xxx yyy  yes

... YYY   no
Assertion 'streq(formatted, "ONE TWO THR…\n" "xxx yyy  yes\n" "a … YYY   no\n")' failed at src/test/test-format-table.c:417, function main(). Aborting.

Aborted (core dumped)

[root@archlinux systemd]# build/test-string-util

/* test_string_erase /

/ test_free_and_strndup /

test_free_and_strndup_one: "(null)", "abc", 0 (expect "", yes)

test_free_and_strndup_one: "", "abc", 0 (expect "", no)

test_free_and_strndup_one: "", "abc", 1 (expect "a", yes)

test_free_and_strndup_one: "a", "abc", 2 (expect "ab", yes)

test_free_and_strndup_one: "ab", "abc", 3 (expect "abc", yes)

test_free_and_strndup_one: "abc", "abc", 4 (expect "abc", no)

test_free_and_strndup_one: "abc", "abc", 5 (expect "abc", no)

test_free_and_strndup_one: "abc", "abc", 5 (expect "abc", no)

test_free_and_strndup_one: "abc", "abc", 4 (expect "abc", no)

test_free_and_strndup_one: "abc", "abc", 3 (expect "abc", no)

test_free_and_strndup_one: "abc", "abc", 2 (expect "ab", yes)

test_free_and_strndup_one: "ab", "abc", 1 (expect "a", yes)

test_free_and_strndup_one: "a", "abc", 0 (expect "", yes)

test_free_and_strndup_one: "", "", 0 (expect "", no)

test_free_and_strndup_one: "", "", 1 (expect "", no)

test_free_and_strndup_one: "", "", 2 (expect "", no)

test_free_and_strndup_one: "", "", 0 (expect "", no)

test_free_and_strndup_one: "", "", 1 (expect "", no)

test_free_and_strndup_one: "", "", 2 (expect "", no)

test_free_and_strndup_one: "", "", 2 (expect "", no)

test_free_and_strndup_one: "", "", 1 (expect "", no)

test_free_and_strndup_one: "", "", 0 (expect "", no)

test_free_and_strndup_one: "", "(null)", 0 (expect "(null)", yes)

test_free_and_strndup_one: "(null)", "foo", 3 (expect "foo", yes)

test_free_and_strndup_one: "foo", "foobar", 6 (expect "foobar", yes)

/ test_ascii_strcasecmp_n /

/ test_ascii_strcasecmp_nn /

/ test_cellescape */

Assertion 'streq(cellescape(buf, 4, "\020"), "…")' failed at src/test/test-string-util.c:179, function test_cellescape(). Aborting.

Aborted (core dumped)

It looks like it could have something to do with my changes but it happens even on master, so I ruled that out.

keszybz · 2020-11-30T13:52:08Z

This is the output of the failed tests:
Tests output
It looks like it could have something to do with my changes but it happens even on master, so I ruled that out.

Try running the tests under gdb and compare the expected and actual strings.

Uhm, this will complicate a bit the parsing. I can't simply use getenv_int anymore.

Something like this:

int getenv_colors(int *res) {
        const char *e;
        int r, val;

        e = getenv("SYSTEMD_COLORS");
        if (!e)
                return -ENXIO;
      
        r = safe_atoi(e, &val);
        if (r < 0) {
              r = parse_bool(e, &val);
              if (r < 0)
                    return r;
        }

        *res = val == 1 ? 256 : val;
        return 0;
}

poettering

i must say i am not a fan of the complexity this adds, for so little reason. but ok.

it's amazing how much energy people can put into things just for the ability to run things with a crazy palette...

src/basic/terminal-util.c

rnhmjoj · 2020-12-01T16:30:55Z

it's amazing how much energy people can put into things just for the ability to run things with a crazy palette...

Yeah, it's certainly not the most efficient way to solve the "problem": this is mostly a pretext to hack on systemd and learn how to contribute. Btw, my palette is not crazy ;)

src/basic/terminal-util.h

src/basic/terminal-util.c

yuwata · 2020-12-04T03:24:32Z

src/basic/terminal-util.c

+        if (cached_colors_enabled < 0) {
+              cached_colors_enabled = get_color_mode() > 0;
        }


Please drop unnecessary {}. Also, please fix the indentation. We use 8ch indentation.

BTW, I think it is not necessary to cache colors_enabled. How about to introduce an inline function in terminal-util.h:

static inline bool colors_enabled(void) { return get_color_mode() > 0; }

Please drop unnecessary {}. Also, please fix the indentation. We use 8ch indentation.

Sorry, I messed this up. Fixed.

BTW, I think it is not necessary to cache colors_enabled

Wouldn't this result in get_color_mode being executed much more frequently?
Also, what about underline_enabled? That it also cached.

yuwata · 2020-12-04T03:31:39Z

src/basic/log.c

@@ -405,7 +405,7 @@ static int write_to_console(
        if (show_location) {
                const char *lon = "", *loff = "";
                if (show_color) {
-                        lon = ANSI_HIGHLIGHT_YELLOW4;
+                        lon = ansi_highlight_yellow4();


Is this correct? It is conditioned by show_color boolean, which may be different from SYSTEMD_COLORS.

I'm not sure but I think it should always match colors_enabled, which is based on get_color_mode.
If I'm wrong, there could be a case with no colors even if show_colors is true, but that requires either NO_COLOR or SYSTEMD_COLORS=0, which makes sense to me.

@yuwata this is the last outstanding comment - any update? Does the explanation make sense?

yuwata · 2020-12-04T03:32:49Z

src/basic/terminal-util.c

@@ -1377,15 +1373,15 @@ void get_log_colors(int priority, const char **on, const char **off, const char

        if (priority <= LOG_ERR) {
                if (on)
-                        *on = ANSI_HIGHLIGHT_RED;
+                        *on = ansi_highlight_red();


Also, is this correct?

yuwata · 2020-12-04T03:32:58Z

src/basic/terminal-util.c

                if (off)
                        *off = ANSI_NORMAL;
                if (highlight)
                        *highlight = ANSI_HIGHLIGHT;

        } else if (priority <= LOG_WARNING) {
                if (on)
-                        *on = ANSI_HIGHLIGHT_YELLOW;
+                        *on = ansi_highlight_yellow();


src/shared/logs-show.c

poettering · 2020-12-10T19:48:10Z

needs rebase

src/basic/terminal-util.c

man/less-variables.xml

poettering · 2020-12-10T19:57:20Z

looks pretty good to me, just minor nits above

src/shared/logs-show.c

keszybz · 2020-12-11T11:06:17Z

Almost there.

keszybz

Hmm, something seems wrong. I'm using test-dhcp6-client as a test.
SYSTEMD_LOG_COLOR=1 SYSTEMD_LOG_LOCATION=1 build/test-dhcp6-client prints a bunch of lines with the location in yellow.
But SYSTEMD_LOG_COLOR=16 ... and SYSTEMD_LOG_COLOR=256 ... both use just the foreground white.
I got the variable name wrong. But it still doesn't seem to work as expected:
SYSTEMD_LOG_COLOR=1 SYSTEMD_COLOR=16 SYSTEMD_LOG_LOCATION=1 build/test-dhcp6-client
and
SYSTEMD_LOG_COLOR=1 SYSTEMD_COLOR=256 SYSTEMD_LOG_LOCATION=1 build/test-dhcp6-client
and
SYSTEMD_LOG_COLOR=1 SYSTEMD_COLOR=0 SYSTEMD_LOG_LOCATION=1 build/test-dhcp6-client
look exactly the same. Is this expected?

man/less-variables.xml

src/basic/terminal-util.c

This commit extends $SYSTEMD_COLORS to an enum variable (compared to a simple boolean) which specifies the "colors mode". This means that, in addition to disabling colors altogether, it's now possible to restrict the console output to 16 or 256 colors only.

There is no need to cache colors_enabled because the function is now simply calling get_color_mode, which is already cached.

rnhmjoj · 2020-12-15T19:44:29Z

Hmm, something seems wrong. I'm using test-dhcp6-client as a test.

I'll take a look, I've only tested journalctl plus a couple of other tools but not this one.

poettering · 2020-12-15T19:53:22Z

lgtm. good to merge, if it works

poettering · 2020-12-16T18:26:33Z

tested it and seems to work, including the dhcp test, after fixing the env var name in the cmdlines @keszybz posted.

Let's get this baby merged then.

Follow-up for: systemd#17702 Alsoe, see earlier review comment: systemd#17702 (review)

Follow-up for: #17702 Alsoe, see earlier review comment: #17702 (review)

rnhmjoj · 2020-12-17T09:01:21Z

@keszybz I couldn't figure out what the cause of test failure is. If you're still interested, I created a torrent with the VM disk (about 1GB) and instructions to reproduce. Here's the magnet:
magnet:?xt=urn:btih:e6eecaf02977f819737040b1494237cb6b4b46fa.

Follow-up for: #17702 Alsoe, see earlier review comment: systemd/systemd#17702 (review)

keszybz requested changes Nov 30, 2020

View reviewed changes

src/basic/env-util.c Outdated Show resolved Hide resolved

man/less-variables.xml Outdated Show resolved Hide resolved

poettering requested changes Dec 1, 2020

View reviewed changes

src/basic/terminal-util.c Outdated Show resolved Hide resolved

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks util-lib labels Dec 1, 2020

yuwata requested changes Dec 4, 2020

View reviewed changes

poettering added the needs-rebase label Dec 10, 2020

poettering requested changes Dec 10, 2020

View reviewed changes

src/basic/terminal-util.c Outdated Show resolved Hide resolved

src/basic/terminal-util.c Show resolved Hide resolved

man/less-variables.xml Outdated Show resolved Hide resolved

dnicolodi reviewed Dec 11, 2020

View reviewed changes

src/shared/logs-show.c Show resolved Hide resolved

bluca added needs-review and removed needs-rebase reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 14, 2020

keszybz requested changes Dec 15, 2020

View reviewed changes

man/less-variables.xml Outdated Show resolved Hide resolved

man/less-variables.xml Outdated Show resolved Hide resolved

man/less-variables.xml Outdated Show resolved Hide resolved

keszybz removed the needs-review label Dec 15, 2020

poettering requested changes Dec 15, 2020

View reviewed changes

src/basic/terminal-util.c Show resolved Hide resolved

src/basic/terminal-util.c Outdated Show resolved Hide resolved

bluca added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 15, 2020

bluca mentioned this pull request Dec 15, 2020

Color support in logs #5428

Closed

rnhmjoj added 2 commits December 15, 2020 19:29

tree-wide: avoid direct use of color macros

25e4608

github-actions bot added documentation journal labels Dec 15, 2020

basic/term-util: inline colors_enabled function

dcdd903

There is no need to cache colors_enabled because the function is now simply calling get_color_mode, which is already cached.

poettering merged commit e4dde4e into systemd:master Dec 16, 2020

poettering added a commit to poettering/systemd that referenced this pull request Dec 16, 2020

terminal-util: use 256 color mode for PID 1 output, too

e438ef3

Follow-up for: systemd#17702 Alsoe, see earlier review comment: systemd#17702 (review)

poettering mentioned this pull request Dec 16, 2020

terminal-util: use 256 color mode for PID 1 output, too #18002

Merged

keszybz pushed a commit that referenced this pull request Dec 17, 2020

terminal-util: use 256 color mode for PID 1 output, too

ddbf960

Follow-up for: #17702 Alsoe, see earlier review comment: #17702 (review)

Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Aug 25, 2021

terminal-util: use 256 color mode for PID 1 output, too

853de8c

Follow-up for: #17702 Alsoe, see earlier review comment: systemd/systemd#17702 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend $SYSTEMD_COLORS to switch colors mode #17702

Extend $SYSTEMD_COLORS to switch colors mode #17702

rnhmjoj commented Nov 23, 2020 •

edited

keszybz commented Nov 30, 2020

keszybz left a comment

rnhmjoj commented Nov 30, 2020

keszybz commented Nov 30, 2020

rnhmjoj commented Nov 30, 2020 •

edited

keszybz commented Nov 30, 2020

poettering left a comment

rnhmjoj commented Dec 1, 2020

yuwata Dec 4, 2020

rnhmjoj Dec 4, 2020

yuwata Dec 4, 2020

rnhmjoj Dec 4, 2020

bluca Dec 14, 2020

yuwata Dec 4, 2020

yuwata Dec 4, 2020

poettering commented Dec 10, 2020

poettering commented Dec 10, 2020

keszybz commented Dec 11, 2020

keszybz left a comment •

edited

rnhmjoj commented Dec 15, 2020

poettering commented Dec 15, 2020

poettering commented Dec 16, 2020

rnhmjoj commented Dec 17, 2020

Extend $SYSTEMD_COLORS to switch colors mode #17702

Extend $SYSTEMD_COLORS to switch colors mode #17702

Conversation

rnhmjoj commented Nov 23, 2020 • edited

Description

Things done:

Questions

keszybz commented Nov 30, 2020

keszybz left a comment

Choose a reason for hiding this comment

rnhmjoj commented Nov 30, 2020

keszybz commented Nov 30, 2020

rnhmjoj commented Nov 30, 2020 • edited

keszybz commented Nov 30, 2020

poettering left a comment

Choose a reason for hiding this comment

rnhmjoj commented Dec 1, 2020

yuwata Dec 4, 2020

Choose a reason for hiding this comment

rnhmjoj Dec 4, 2020

Choose a reason for hiding this comment

yuwata Dec 4, 2020

Choose a reason for hiding this comment

rnhmjoj Dec 4, 2020

Choose a reason for hiding this comment

bluca Dec 14, 2020

Choose a reason for hiding this comment

yuwata Dec 4, 2020

Choose a reason for hiding this comment

yuwata Dec 4, 2020

Choose a reason for hiding this comment

poettering commented Dec 10, 2020

poettering commented Dec 10, 2020

keszybz commented Dec 11, 2020

keszybz left a comment • edited

Choose a reason for hiding this comment

rnhmjoj commented Dec 15, 2020

poettering commented Dec 15, 2020

poettering commented Dec 16, 2020

rnhmjoj commented Dec 17, 2020

rnhmjoj commented Nov 23, 2020 •

edited

rnhmjoj commented Nov 30, 2020 •

edited

keszybz left a comment •

edited