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

Add terminal color choosing support #2508

Closed
wants to merge 1 commit into from

Conversation

ulyssessouza
Copy link

@ulyssessouza ulyssessouza commented Dec 7, 2021

This adds the capability of choosing the color to print in the terminal by just setting an envvar called BUILDKIT_TERM_COLOR.

Note that this is a "follow-up" of #2368 making it more flexible so that the users can choose it's preferred color depending on their terminal themes and preferences.

It supports the following values:

"default"

"black"
"red"
"green"
"yellow"
"blue"
"magenta"
"cyan"
"white"

"light-black"
"light-red"
"light-green"
"light-yellow"
"light-blue"
"light-magenta"
"light-cyan"
"light-white"



func getTermColor() aec.ANSI {
envColorString := os.Getenv("BUILDKIT_TERM_COLOR")
Copy link
Member

Choose a reason for hiding this comment

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

Is there a convention for this? I know NO_COLOR is a thing but this looks somewhat weird as there isn't just one color. You are changing the color of the completed item but iirc we have 4 possible different colors used by this progressbar.

Copy link
Author

Choose a reason for hiding this comment

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

AFAIC there is not convention on terminal color naming. I just took the ones from "github.com/morikuni/aec" in lower case.

This PR only affects the "typical buildkit blue" color and not the other 3. It's not a complete theme (I actually like the idea of having it 😺).

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
@tonistiigi
Copy link
Member

@crazy-max wdyt?

I think we could definitely support NO_COLOR and if there is a known convention for passing additional colors via env it would probably be ok. Only one I know about is LS_COLORS but that uses a different format. Do we want to define a similar key=index: format for this? https://geoff.greer.fm/lscolors/ . All this could also be in aec level though.

Currently just having a special env for overriding one specific color seems too hacky to me.

@crazy-max
Copy link
Member

crazy-max commented Dec 10, 2021

Yes agree that we should support NO_COLOR, probably upstream on https://github.com/morikuni/aec like go-ansi has done.

I'm not aware of a convention but we should also support cancel and error states (probably also warning #2498 in the future). Maybe with a key like you said @tonistiigi BUILDKIT_TERM_COLOR=run=blue,cancel=yellow,error=red?

@crazy-max
Copy link
Member

As discussed using LS_COLORS lgtm. e.g., LS_COLORS=ru=blue:ca=yellow:er=red:wa=orange

@tonistiigi
Copy link
Member

So not LS_COLORS directly but similar logic. BUILDKIT_COLORS. Or maybe could be AEC_COLORS=31=34: as well.

Keys do not need to be 2 chars and there is no "orange".

@spkane
Copy link

spkane commented Feb 9, 2022

@ulyssessouza Is this PR being actively worked on still, based on the feedback? I would love to see this functionality implemented in buildkit.

@ulyssessouza
Copy link
Author

@spkane I'll finish that soon... Just trying to make a bit of time here... Wouldn't take much time

@spkane
Copy link

spkane commented Jul 8, 2022

@ulyssessouza I took a stab at this in another PR (#2954), because I had some time and the desire to try and implement this. I'd appreciate hearing what you think.

@crazy-max
Copy link
Member

fixed by #2954

@crazy-max crazy-max closed this Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants