Make terminal colors configurable #2954

spkane · 2022-07-08T20:51:48Z

This pull request is inspired by the work and conversation here: #2508

I did not want to stomp over the existing work, but since it has been many months and I had some time, I thought I would try my hand at it. Let me know if this is a reasonable starting place.

The NO_COLOR implementation needs more work, as it is using ANSI still, but just the default off-white for everything. I'd also be happy to pull it out of here for the moment, just so we can get the general color configuration support merged in.

UPDATED: The NO_COLOR implementation is now correctly avoiding the use of any ANSI colors.

This at least partially addresses some of the features here: docker/roadmap#301

cc/ @ulyssessouza @tonistiigi @crazy-max

Default colors (non-windows):

User-defined colors using names:

User-defined colors using RGB values and names:

NO_COLOR defined (even with user-defined colors also set):

Example Usage

From the root of this repo:

BUILDKIT_COLORS=run=123,20,245:error=orange:cancel=blue:warning=white
 ./bin/buildctl build --frontend gateway.v0 --opt source=docker/dockerfile --local context=. --local dockerfile
=.

NOTE: Once this is merged we can also update this PR against https://no-color.org/ to add buildkit to the projects that support NO_COLOR. UPDATED: Done!

util/progress/progressui/colors.go

jedevc · 2022-07-11T11:06:48Z

util/progress/progressui/init.go

+
+func init() {
+	_, noColorIsSet = os.LookupEnv("NO_COLOR")
+	_, buildkitColorIsSet = os.LookupEnv("BUILDKIT_COLORS")


I'm guessing the NO_COLOR option comes from https://no-color.org/? Might be useful to document that in a comment around it.

Would be good to respect it, but I think the right behavior might be to have BUILDKIT_COLORS override it if set (specific options should override general options imo)? I also think a BUILDKIT_COLORS=none option would be good, as a buildkit-specific way of forcing no colors - that could also extend into allowing multiple different "presets" in the future, if that was something we wanted to support.

Adding a comment makes sense. I was going back and forth a bit on which variable should take precedence, and personally felt that NO_COLOR seemed like it should be the winner, as it seemed a bit more likely that an environment might have BUILDKIT_COLORS defined and then a specific script or pipeline would potentially disable it for one reason or another, but I am happy to change it if the general consensus is that BUILDKIT_COLORS should win.

util/progress/progressui/init.go

spkane · 2022-07-11T22:43:23Z

Added an example of using RGB values into the first comment.

I'd be interested in some more discussion around the suggestion that @jedevc made, to reverse which env var wins when both BUILDKIT_COLORS and NO_COLOR is set.
Errors are currently being printed as warnings, and then the system is falling back to defaults, as it doesn't feel like these should be fatal, but I am wondering what others think about this. Also, Is there a better way to report these errors to the console?
~~At the moment NO_COLOR still uses aec default colors, which is probably not quite right.~~ UPDATED: NO_COLOR is completely disabling ANSI colors now.

README.md

util/progress/progressui/colors.go

jedevc · 2022-07-12T10:10:41Z

util/progress/progressui/colors.go

+			logrus.Warnf("Could not parse BUILDKIT_COLORS component: %s", strings.Join(parts, "="))
+			continue
+		}
+		if strings.Contains(parts[1], "=") {


Not quite sure what case this catches? I think this would get caught anyways a bit further down, since it isn't a valid rgb or per-defined color.

This catches "warning=yellow=red" or something similar. You are right that it will get caught later, but this error message points more directly at the problem since it will print the whole field and say that it is not parseable. I'd prefer to leave this, instead of falling down to the unknown color error message.

If this is identical to the above if branch, we can merge them to be if len(parts) != 2 || strings.Contains(parts[1], "=") {

util/progress/progressui/colors.go

spkane · 2022-07-13T18:28:48Z

@jedevc Cleaned things up a little more and did some additional testing. Let me know if we are in a good enough place to get this in yet. Thanks!

.gitignore

thaJeztah · 2022-07-14T13:12:40Z

Some random thoughts;

first of all, the "hard to read" colors effectively depend on the terminal's color profile. While it's true that the defaults aren't great (dark blue on a dark background is really hard to read), the user's terminal should have options to customise this. If blue isn't readable, the same would already be the case for ls (with coloring enabled).
that said, I can see some value in the ability to customize, but (thinking out loud); should this be a global configuration that's part of the docker CLI? (Configurable in ~/.docker/config.json, and possibly an env-var?). At least, I think we should already think of "future directions" in that respect (our ultimate goal is to provide a consistent experience for all docker xx commands).
Not sure what the best approach would be in that case for the "names"; depending on "to what level of granularity do we want this to be configurable;
- could be either "specific" (build.run, build.error, (compose.build.run ??))
- or some more generic terms (progress, warning, error, ...)
- could be "both" (error is the global option, but can be overridden for build through build.error)
looking at existing tools, for example, ls, there appear to be some existing examples for customisation; LSCOLORS / LS_COLORS, CLICOLOR, COLORTERM ("yay" for BSD vs GNU though), as well as dircolors; unfortunately those seem to be very specific to ls, and while dircolors provides a "database", it doesn't appear to be extendable for other uses.
So, while those are not directly applicable, we could look if there's common conventions (e.g. the format used for LS_COLORS looks to use colons as separator (:), uses numeric values for POSIX colors, and allows both fore- and background colors to be set (LS_COLORS='rs=0:di=01;34:...'))
To be clear; I don't think the numeric values are "convenient" to use, but mostly wondering what existing conventions exist, just in case we're re-inventing the wheel and we could piggyback on existing conventions (which could potentially allow users to copy/reuse configurations they already have)
Related to the above, happy to see NO_COLOR, which appears to be(come) more of a standard (not sure if other vars like COLORTERM also should be taken into account, although it looks like it's not available everywhere yet.

Finally; w.r.t. the implementation;

I see a majority of the code currently lives in util/progress/progressui/. While that makes sense for the purpose in this PR, perhaps we should think of making it a separate colors (e.g.) package that contains the reusable bits (parsing the env-var's value, creating a map with colors, and defining the color-names)
Functions like parseKeys() would not be part of that (although perhaps there would be a mechanism to initialise a color map "database" and register keys)
Ideally, the package would not do logging (an error could be returned, leaving it to the caller to handle, which could be "log as warning")
I see the package adds support for RGB colors; that slightly brings me back to my earlier comment about the POSIX colors (usually?) being configurable, but also: do we need feature detection to know if it's supported? (see COLORTERM mentioned earlier, but perhaps other suitable ways) What happens if it's not? (do we need a fallback?)

spkane · 2022-07-15T19:06:36Z

Some random thoughts;

first of all, the "hard to read" colors effectively depend on the terminal's color profile. While it's true that the defaults aren't great (dark blue on a dark background is really hard to read), the user's terminal should have options to customise this. If blue isn't readable, the same would already be the case for ls (with coloring enabled).

Users can certainly adjust the background of their terminal, however, if we presume they have the color that they like, then they are unlikely to change it because one program picks a color that is un-readable on a common background choice. At least adjusting the blue to a somewhat lighter blue that is readable on white and black would be a good place to start.

that said, I can see some value in the ability to customize, but (thinking out loud); should this be a global configuration that's part of the docker CLI? (Configurable in ~/.docker/config.json, and possibly an env-var?). At least, I think we should already think of "future directions" in that respect (our ultimate goal is to provide a consistent experience for all docker xx commands).

I could absolutely see Docker having support for this across the board, and being able to pass that configuration to the underlying tools/plugins. It seems like underlying tools and libraries like buildkit (and buildctl) need to have their own support for this (even if it comes from a shared library), so this customization can be done even in the absence of the docker client.

Not sure what the best approach would be in that case for the "names"; depending on "to what level of granularity do we want this to be configurable;

could be either "specific" (build.run, build.error, (compose.build.run ??))

or some more generic terms (progress, warning, error, ...)

could be "both" (error is the global option, but can be overridden for build through build.error)

Eventually, both option seems ideal here.

There are some "standard" names that each plugin/application uses internally, like run and error, and then when being configured from a docker config you would use the standard name to set the default (e.g. run=cyan) or the plugin id and that name (e.g. buildkit.run=white), if you want to override the color for only that.

looking at existing tools, for example, ls, there appear to be some existing examples for customisation; LSCOLORS / LS_COLORS, CLICOLOR, COLORTERM ("yay" for BSD vs GNU though), as well as dircolors; unfortunately those seem to be very specific to ls, and while dircolors provides a "database", it doesn't appear to be extendable for other uses.

Based on the discussion in #2508, I used the basic idea of LS_COLORS to guide this, while trying to keep pretty simple and something that would work on any platform.

So, while those are not directly applicable, we could look if there's common conventions (e.g. the format used for LS_COLORS looks to use colons as separator (:), uses numeric values for POSIX colors, and allows both fore- and background colors to be set (LS_COLORS='rs=0:di=01;34:...'))

This is using : as a separator, but is using named colors or RGB colors since that is what the underlying library appears to use most directly.

BUILDKIT_COLORS=run=123,20,245:error=orange:cancel=blue:warning=white

To be clear; I don't think the numeric values are "convenient" to use, but mostly wondering what existing conventions exist, just in case we're re-inventing the wheel and we could piggyback on existing conventions (which could potentially allow users to copy/reuse configurations they already have)

I think that we have covered many of the more obvious ones here, and I don't think LS_COLORS can be directly utilized and relied upon to even exist on all platforms.

Related to the above, happy to see NO_COLOR, which appears to be(come) more of a standard (not sure if other vars like COLORTERM also should be taken into account, although it looks like it's not available everywhere yet.

It might make sense to check COLORTERM for truecolor if there is some code that should be adjusted due to this support being missing.

Finally; w.r.t. the implementation;

I see a majority of the code currently lives in util/progress/progressui/. While that makes sense for the purpose in this PR, perhaps we should think of making it a separate colors (e.g.) package that contains the reusable bits (parsing the env-var's value, creating a map with colors, and defining the color-names)

Functions like parseKeys() would not be part of that (although perhaps there would be a mechanism to initialise a color map "database" and register keys)

Ideally, the package would not do logging (an error could be returned, leaving it to the caller to handle, which could be "log as warning")

@thaJeztah This all makes sense. Is this something that would need to happen before a merge? Or as a further iteration on this?

Personally, I'd like to at least see a stop-gap get in until there is a more thorough solution that talks to all the good points about future requirements that you have brought up, but to be fair, I am mostly trying to solve an issue that has been plaguing me and I know that you need to consider the broader implications.

I'm happy to do a bit more work on this, but I likely won't have the time (or experience) to make this into a robust library for the whole ecosystem.

I see the package adds support for RGB colors; that slightly brings me back to my earlier comment about the POSIX colors (usually?) being configurable, but also: do we need feature detection to know if it's supported? (see COLORTERM mentioned earlier, but perhaps other suitable ways) What happens if it's not? (do we need a fallback?)

So, I think this is at least as compatible as the current code is, since buildkit uses ANSI colors without any checks.

I did a quick check on how various terminals behave:

The built-in mac terminal does not appear to properly support 24-bit color (as tested with pastel colorcheck - https://github.com/sharkdp/pastel), yet the colors generated from this PR seem to work fine.

For iterm2 which does appear to support 24-biot colors fine, it also works:

README.md

util/progress/progressui/init.go

spkane · 2022-07-19T19:47:10Z

Note: This PR adds a warning color based on some earlier discussions that referenced #2498.

However colorWarning is not actually being used by anything at the moment. It could be pulled out of here or left for future use as people see fit.

tonistiigi

colorWarning is causing an issue in the CI so needs to be plugged in or fixed in some other way. I guess if we leave it in there should be a public method to access this color as some warnings are printed outside of progressbar as well.

I don't like with colorWarning that we are defaulting to the RGB value. This will look off in properly configured terminals where the RGB does not work together with other colors. We should leave defaults only to the predefined names. If there isn't a better option then just leave it to yellow or we can use a different light mode for cancel vs warning.

util/progress/progressui/init.go

spkane · 2022-07-20T01:18:18Z

I made some changes that I think will alleviate the CI issues, and I just switched the default warning color to yellow. It seems like the best choice to stick with the named, 3-bit colors. And orange ends up being yellow when translated to a 3-bit color.

spkane · 2022-07-20T03:44:44Z

@tonistiigi colorWarning is switched back to yellow and all 41 checks are passing now.

jedevc

This is looking good IMO - just a few nits.

.gitignore

README.md

jedevc · 2022-07-20T10:54:00Z

util/progress/progressui/colors.go

+			logrus.Warnf("Could not parse BUILDKIT_COLORS component: %s", strings.Join(parts, "="))
+			continue
+		}
+		if strings.Contains(parts[1], "=") {


If this is identical to the above if branch, we can merge them to be if len(parts) != 2 || strings.Contains(parts[1], "=") {

util/progress/progressui/colors.go

Signed-off-by: Sean P. Kane <spkane00@gmail.com>

spkane · 2022-07-20T16:05:44Z

@jedevc I moved the .gitignore note into https://github.com/moby/buildkit/pull/2972/files and incorporated all your other suggestions.

spkane · 2022-07-20T16:08:30Z

When this is merged we can also update this PR against https://no-color.org/ to add buildkit to the projects that support NO_COLOR.

jcs/no_color#191

tonistiigi · 2022-07-20T01:35:25Z

util/progress/progressui/display.go

@@ -108,6 +108,7 @@ type job struct {
 	name        string
 	status      string
 	hasError    bool
+	hasWarning  bool


Set it as well. Or if you prefer to do follow-up then add a comment that this is not set yet.

Currently, iiuc the warnings use the default color.

@tonistiigi Is there a reliable way that I can generate a buildkit warning during a build using buildctl that I could use for testing?

I'm not sure whether I'll be able to wire the warning functionality all the way through, but I can certainly give it a try. If not, I'll go ahead and add the comment and leave it for a follow-up PR.

I went ahead and added a comment to hasWarning for the moment.

Signed-off-by: Sean P. Kane <spkane00@gmail.com>

spkane · 2022-07-20T23:02:07Z

@tonistiigi @jedevc I think this is good to merge at this point. Please let me know if there is anything else that you need from me.

I'd also be interested in knowing roughly when a new release of buildkit might be cut (v0.10.4 ?) and how long it usually takes for commits here to get into the released docker CLI, etc.

tonistiigi · 2022-07-20T23:23:59Z

This is a new feature so would not be picked for v0.10.4 but needs to wait for v0.11. Buildx release should be coming soon though https://github.com/docker/buildx/pulls?q=is%3Aopen+is%3Apr+milestone%3Av0.9.0 and we can consider vendoring client libs from master to get the latest updates for it.

spkane · 2022-07-21T15:38:02Z

@tonistiigi Thank for getting this merged and letting me know what the release schedule looks like!

spkane mentioned this pull request Jul 8, 2022

Add terminal color choosing support #2508

Closed

spkane force-pushed the terminal-colors branch from e7275a2 to 6019bf1 Compare July 8, 2022 21:04

jedevc requested changes Jul 11, 2022

View reviewed changes

spkane force-pushed the terminal-colors branch from 6fc28da to 730fe40 Compare July 11, 2022 22:28

spkane requested a review from jedevc July 11, 2022 22:43

jedevc reviewed Jul 12, 2022

View reviewed changes

spkane requested a review from jedevc July 13, 2022 18:28

thaJeztah reviewed Jul 14, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

spkane requested a review from thaJeztah July 18, 2022 15:33

tonistiigi reviewed Jul 18, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

util/progress/progressui/init.go Outdated Show resolved Hide resolved

util/progress/progressui/init.go Outdated Show resolved Hide resolved

util/progress/progressui/init.go Outdated Show resolved Hide resolved

spkane force-pushed the terminal-colors branch from 844f810 to 3878359 Compare July 19, 2022 19:29

spkane mentioned this pull request Jul 19, 2022

[Feature] Add direct support for the NO_COLOR and COLORTERM environment variable into **aec** morikuni/aec#3

Open

spkane requested a review from tonistiigi July 19, 2022 21:21

spkane force-pushed the terminal-colors branch from 10f68e1 to fed245f Compare July 19, 2022 21:25

tonistiigi reviewed Jul 19, 2022

View reviewed changes

util/progress/progressui/init.go Outdated Show resolved Hide resolved

spkane requested a review from tonistiigi July 20, 2022 03:43

jedevc reviewed Jul 20, 2022

View reviewed changes

spkane mentioned this pull request Jul 20, 2022

Add gitignore note #2972

Merged

Configurable terminal colors

cd48499

Signed-off-by: Sean P. Kane <spkane00@gmail.com>

spkane force-pushed the terminal-colors branch from 82a4c40 to cd48499 Compare July 20, 2022 16:03

spkane requested a review from jedevc July 20, 2022 16:04

spkane mentioned this pull request Jul 20, 2022

Add buildkit jcs/no_color#191

Merged

jedevc approved these changes Jul 20, 2022

View reviewed changes

tonistiigi approved these changes Jul 20, 2022

View reviewed changes

Add 'hasWarning' comment

509444f

Signed-off-by: Sean P. Kane <spkane00@gmail.com>

tonistiigi approved these changes Jul 20, 2022

View reviewed changes

tonistiigi merged commit ebb1e82 into moby:master Jul 20, 2022

spkane deleted the terminal-colors branch July 21, 2022 15:38

spkane mentioned this pull request Jul 21, 2022

Update buildkit w/ customizable output colors, etc. docker/buildx#1226

Merged

spkane mentioned this pull request Sep 7, 2022

[FEATURE] Update console color output to match the new color customization support in buildkit docker/compose#9818

Open

jedevc mentioned this pull request Jan 31, 2023

progress: keep color enabled with NO_COLOR empty #3556

Merged

def- mentioned this pull request Jun 30, 2023

build: Build all cargo executables at once MaterializeInc/materialize#20221

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make terminal colors configurable #2954

Make terminal colors configurable #2954

spkane commented Jul 8, 2022 •

edited

jedevc Jul 11, 2022

spkane Jul 11, 2022

spkane commented Jul 11, 2022 •

edited

jedevc Jul 12, 2022

spkane Jul 13, 2022 •

edited

jedevc Jul 20, 2022

spkane commented Jul 13, 2022

thaJeztah commented Jul 14, 2022

spkane commented Jul 15, 2022 •

edited

spkane commented Jul 19, 2022

tonistiigi left a comment

spkane commented Jul 20, 2022 •

edited

spkane commented Jul 20, 2022

jedevc left a comment •

edited

jedevc Jul 20, 2022

spkane commented Jul 20, 2022

spkane commented Jul 20, 2022 •

edited

tonistiigi Jul 20, 2022

spkane Jul 20, 2022 •

edited

spkane Jul 20, 2022

spkane commented Jul 20, 2022

tonistiigi commented Jul 20, 2022

spkane commented Jul 21, 2022

Make terminal colors configurable #2954

Make terminal colors configurable #2954

Conversation

spkane commented Jul 8, 2022 • edited

Example Usage

jedevc Jul 11, 2022

Choose a reason for hiding this comment

spkane Jul 11, 2022

Choose a reason for hiding this comment

spkane commented Jul 11, 2022 • edited

jedevc Jul 12, 2022

Choose a reason for hiding this comment

spkane Jul 13, 2022 • edited

Choose a reason for hiding this comment

jedevc Jul 20, 2022

Choose a reason for hiding this comment

spkane commented Jul 13, 2022

thaJeztah commented Jul 14, 2022

spkane commented Jul 15, 2022 • edited

spkane commented Jul 19, 2022

tonistiigi left a comment

Choose a reason for hiding this comment

spkane commented Jul 20, 2022 • edited

spkane commented Jul 20, 2022

jedevc left a comment • edited

Choose a reason for hiding this comment

jedevc Jul 20, 2022

Choose a reason for hiding this comment

spkane commented Jul 20, 2022

spkane commented Jul 20, 2022 • edited

tonistiigi Jul 20, 2022

Choose a reason for hiding this comment

spkane Jul 20, 2022 • edited

Choose a reason for hiding this comment

spkane Jul 20, 2022

Choose a reason for hiding this comment

spkane commented Jul 20, 2022

tonistiigi commented Jul 20, 2022

spkane commented Jul 21, 2022

spkane commented Jul 8, 2022 •

edited

spkane commented Jul 11, 2022 •

edited

spkane Jul 13, 2022 •

edited

spkane commented Jul 15, 2022 •

edited

spkane commented Jul 20, 2022 •

edited

jedevc left a comment •

edited

spkane commented Jul 20, 2022 •

edited

spkane Jul 20, 2022 •

edited