--no-progress replaced with --progress #504

kunalkushwaha · 2018-07-13T07:26:52Z

Fixes #443

Signed-off-by: Kunal Kushwaha kushwaha_kunal_v7@lab.ntt.co.jp

ijc · 2018-07-13T09:51:31Z

cmd/buildctl/build.go

- Usage: "Don't show interactive progress",
+ Name: "console",
+ Usage: "Show console output",
+ Hidden: true,


Why is it hidden?

Also, shouldn't this default to true so the autodetection of a tty on stdio and, if possible, the fancy output, happens by default? (Maybe s/Hidden/Default/g is what you meant?).

Yes, Hidden was meant for Default. Since BoolFlag had no field Value or Default so I used Hidden.

[Update]: I replaced BoolFlag with BoolTFlag, so default is true and in case of Stdio is not terminal, it will just send the build log on output channel.

tonistiigi · 2018-07-25T06:43:54Z

ping @tiborvass

AkihiroSuda · 2018-07-25T07:25:59Z

cmd/buildctl/build.go

@@ -206,7 +206,7 @@ func build(clicontext *cli.Context) error {

 eg.Go(func() error {
 var c console.Console
- if !clicontext.Bool("no-progress") {
+ if clicontext.Bool("console") {


Maybe we can keep no-progress as a hidden flag for compatibility

thaJeztah · 2018-07-25T07:45:25Z

Is auto still an option? Looking at #443 (comment), the reason for naming it --console instead of --no-console was because the flag accepted three values; auto, false, true. If that's no longer the case, --no-console would be more convenient, and would make the boolean's default value ("false") also the default state

kunalkushwaha · 2018-07-25T08:48:09Z

Will update the PR for --console as three value flag (auto, true, false)

kunalkushwaha · 2018-07-26T02:36:28Z

@thaJeztah Now buildkit build --console have same behaviour as docker build --console

tiborvass · 2018-08-02T17:57:58Z

@kunalkushwaha Thank you for your PR and sorry for the delay in answering. I wasn't super happy about --console=false. I'd prefer --console [auto|plain|rich], hopefully that makes more sense. Let me know what you think. Will update in docker CLI too.

cc @AntaresS

tonistiigi · 2018-08-02T18:02:44Z

@tiborvass Why is called console then? What does "rich" mean? Why is that if I want to get more output I need to use "plain" and if I only want the command names I use "rich" (I assume this is how they map).

kunalkushwaha · 2018-08-03T00:59:35Z

rich & plain will be more confusing with console flag.
I guess from rich you meant interactive (progress bar) whereas plain is just log of build progress, which someone may want even terminal is there.

If we want to give user that option, its better to give new output flag as rich|plain, which will be valid in case of console=auto|true

tiborvass · 2018-08-03T05:25:09Z

@tonistiigi because it controls the way the console is used for output. I get your point about rich having less information than plain.

How about --pretty=[auto|true|false] ? Pretty is often associated with colors, so we could assume users will understand that this flag can be turned off to make it less pretty, in exchange for more information?

tonistiigi · 2018-08-03T22:02:28Z

How about --pretty=[auto|true|false]

This seems to almost counter the previously proposed logic. How would you explain pretty=auto.

Pretty is often associated with colors,

Yes, but in BuildKit this has very little to do with colors. The two outputs are for different use-cases and contain different data, not for people who prefer coloring and who don't.

tonistiigi · 2018-08-03T22:05:01Z

My main issue with previous is that I don't think there is such a thing as a rich console. Or at least that name does not indicate the actual behavior of the flag.

tianon · 2018-08-03T22:28:26Z

This was a huge source of confusion for me -- IMO, this flag is currently controlling two separate behaviors. One is build verbosity and the other is progress / "re-writing" output (which wouldn't be awful to have even in the verbose case, it just isn't as necessary).

tiborvass · 2018-08-03T23:47:51Z

@tianon the problem comes from the fact that showing RUN output in parallel in the "rewriting" output, is a really hard problem to solve. It's not impossible we'll have a solution but right now it's clearly out of the picture.

What would have helped to lower the confusion? A --show-logs flag that would conflict if the progress output is chosen? With a nice message explaining that right now that only works with --no-progress mode ?

Btw, the other problem I see with the "progress" name is that it's not like --no-progress doesn't show progress, it shows it differently, in a plain text mode.

I'd be happy to hear more suggestions.

@tonistiigi

Yes, but in BuildKit this has very little to do with colors. The two outputs are for different use-cases and contain different data, not for people who prefer coloring and who don't.

Since we're changing the way we display data... --display ?

I'm super close to being out of ideas.

tianon · 2018-08-04T00:02:56Z

the problem comes from the fact that showing RUN output in parallel in the "rewriting" output, is a really hard problem to solve. It's not impossible we'll have a solution but right now it's clearly out of the picture.

Totally understood, and agree. ❤️

What would have helped to lower the confusion?

Honestly, even just a little bit more documentation would've helped a ton. All I had to go on was the following:

  --console  Show console output (with buildkit only) (true, false, auto) (default auto)

I was wanting the console output of my RUN lines, so naturally I tried --console=true, when in fact what I wanted was the exact opposite.

I think it needs to be clear that this flag (whatever it's named) affects the amount of information displayed as well as the way that information is displayed.

Having one flag that controls terse vs verbose and a separate flag that controls whether we use control codes would be a really good idea IMO (and for now, like you said, having the control codes be temporarily incompatible with the verbose output with an error).

For other software, this usually exhibits as a flag to explicitly disable TTY detection (or explicitly force TTY behavior) like --color=never, --color=always, --color=auto, but as you mentioned this doesn't really have to do with colors and if we mention TTYs we might create more confusion ("is this flag providing a TTY to my RUN lines?"), so we're in a bit of a hard spot.

All that being said, I think --progress is less confusing than --console (since "console" is an ambiguous term in this case, and that ambiguity combined with the lack of documentation is what created my confusion in the first place).

tiborvass · 2018-08-04T01:44:33Z

@tianon thank you so much for your feedback!

@tonistiigi just suggested to me --progress=[auto|plain|tty]. I'm hopeful that with that and a description detailing the fact that exec logs will currently only be shown in plain mode, the UX will be much better.

@kunalkushwaha @tianon @AkihiroSuda @AntaresS what do you think?

AntaresS · 2018-08-04T07:30:19Z

I think it needs to be clear that this flag (whatever it's named) affects the amount of information displayed as well as the way that information is displayed.

I agree with @tianon's point. After reading the whole thread, I think the foundamental question is we want the meaning of this flag to clearly tell users what type of content they are willing to see. Hence, thinking from user perspective I'd prefer either --display or --progress if they are well documented.

idea 1 using --display Sets how buildkit output should look like

--display=[auto|plain|progress]

idea 2 using --progress

--progress=[auto|plain|pretty]

cc @tiborvass

kunalkushwaha · 2018-08-06T00:57:23Z

--progress=[auto|plain|tty]. I'm hopeful that with that and a description detailing the fact that exec logs will currently only be shown in plain mode, the UX will be much better.

For now it sounds good.

showing RUN output in parallel in the "rewriting" output, is a really hard problem to solve. It's not impossible we'll have a solution but right now it's clearly out of the picture.

For better UX, Will apt install kind of output suitable here? i.e. Showing whole build logs in all cases and progress bar at bottom of screen in case tty is attached?.

@tiborvass @tonistiigi @AkihiroSuda

tiborvass · 2018-08-06T22:55:16Z

FYI I opened docker/cli#1276

kunalkushwaha · 2018-08-17T07:10:42Z

Updated as per docker/cli#1276

tiborvass · 2018-08-17T08:21:55Z

cmd/buildctl/build.go

- c = cf
- }
+ progressOpt := clicontext.String("progress")
+ if !(progressOpt == "auto" || progressOpt == "tty" || progressOpt == "plain") {


small nit, but would you mind doing progressOpt != ... && progressOpt != ... ? I think it's easier to read.

tiborvass · 2018-08-17T08:22:21Z

@kunalkushwaha thanks! Apart from small nit, I'm ready to merge this :)

tiborvass · 2018-08-17T09:03:33Z

@kunalkushwaha also would you mind updating the commit message?

kunalkushwaha · 2018-08-17T09:07:30Z

I left for the day. Will try updating PR tomorrow. Regards, Kunal Kushwaha

…

------------------------------ Sent from mobile device

On Fri, Aug 17, 2018, 6:03 PM Tibor Vass ***@***.***> wrote: @kunalkushwaha <https://github.com/kunalkushwaha> also would you mind updating the commit message? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACoXIR8dEkc7d57PHRAF_zzoqQO58cfmks5uRobngaJpZM4VObPD> .

kunalkushwaha · 2018-08-20T01:02:18Z

Updated PR as per suggestion.

thaJeztah · 2018-08-20T08:39:53Z

cmd/buildctl/build.go

 }
+ if cf, err := console.ConsoleFromFile(os.Stderr); err == nil && (progressOpt == "auto" || progressOpt == "tty") {


In what situations do errors occur here? Is an error only generated if "no console" is present (so, ok to ignore if "auto" is selected), or are there other situations?

What if "tty" is selected, and an error occurs? Should that error be returned?

Also wondering if a switch would be slightly clearer; doing so would also prevent running console.ConsoleFromFile() if not needed (i.e., if "plain" was used as option);

switch clicontext.String("progress") { case "auto", "tty": cf, err := console.ConsoleFromFile(os.Stderr) if err != nil { return err } c = cf case "plain": default: return errors.New("Invalid progress value") }

right now, error is not returned if tty is option and is failback on plain.
I will update it as per suggestion.

kunalkushwaha · 2018-08-23T07:50:30Z

Fixed as per suggestion.

return error only in case of tty as auto should fallback to plain output.

tonistiigi · 2018-08-24T22:18:07Z

cmd/buildctl/build.go

+ c = cf
+ case "plain":
+ default:
+ return errors.New("Invalid progress value")


nit: lowercase errors

also print the value in the error message

this make buildctl build output options same as moby. Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>

tiborvass

LGTM

thaJeztah

whoops, forgot to update my review

post-merge LGTM, thanks!!

ijc reviewed Jul 13, 2018

View reviewed changes

kunalkushwaha force-pushed the no-console branch 2 times, most recently from 1d80771 to 99920a5 Compare July 17, 2018 04:38

tonistiigi requested a review from tiborvass July 17, 2018 17:48

AkihiroSuda reviewed Jul 25, 2018

View reviewed changes

kunalkushwaha force-pushed the no-console branch from 99920a5 to 787a20c Compare July 26, 2018 02:20

thaJeztah mentioned this pull request Jul 31, 2018

Enhancement: builder: support progress report when no TTY is available. docker/cli#688

Closed

tonistiigi mentioned this pull request Aug 7, 2018

console output during build #526

Closed

kunalkushwaha force-pushed the no-console branch 2 times, most recently from 1e8688c to 2d40ea2 Compare August 17, 2018 07:10

tiborvass reviewed Aug 17, 2018

View reviewed changes

tiborvass changed the title ~~--no-progress replaced with --console~~ --no-progress replaced with --progress Aug 17, 2018

kunalkushwaha force-pushed the no-console branch from 2d40ea2 to 386fb82 Compare August 20, 2018 00:37

thaJeztah requested changes Aug 20, 2018

View reviewed changes

kunalkushwaha force-pushed the no-console branch from 386fb82 to 716bc8e Compare August 23, 2018 07:44

tonistiigi approved these changes Aug 24, 2018

View reviewed changes

kunalkushwaha force-pushed the no-console branch from 716bc8e to 49c9f75 Compare August 27, 2018 02:19

--no-progress replaced with --progress

49c9f75

this make buildctl build output options same as moby. Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>

tonistiigi assigned tiborvass Aug 28, 2018

tiborvass approved these changes Aug 29, 2018

View reviewed changes

tiborvass merged commit 17e6416 into moby:master Aug 29, 2018

thaJeztah reviewed Aug 30, 2018

View reviewed changes

AntaresS mentioned this pull request Dec 19, 2018

[buildkit] help definition of docker build --console is confusing docker/cli#1133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--no-progress replaced with --progress #504

--no-progress replaced with --progress #504

kunalkushwaha commented Jul 13, 2018

ijc Jul 13, 2018

kunalkushwaha Jul 17, 2018 •

edited

Loading

tonistiigi commented Jul 25, 2018

AkihiroSuda Jul 25, 2018

thaJeztah commented Jul 25, 2018

kunalkushwaha commented Jul 25, 2018

kunalkushwaha commented Jul 26, 2018

tiborvass commented Aug 2, 2018

tonistiigi commented Aug 2, 2018

kunalkushwaha commented Aug 3, 2018

tiborvass commented Aug 3, 2018

tonistiigi commented Aug 3, 2018

tonistiigi commented Aug 3, 2018

tianon commented Aug 3, 2018

tiborvass commented Aug 3, 2018 •

edited

Loading

tianon commented Aug 4, 2018

tiborvass commented Aug 4, 2018

AntaresS commented Aug 4, 2018

kunalkushwaha commented Aug 6, 2018

tiborvass commented Aug 6, 2018

kunalkushwaha commented Aug 17, 2018

tiborvass Aug 17, 2018

tiborvass commented Aug 17, 2018

tiborvass commented Aug 17, 2018

kunalkushwaha commented Aug 17, 2018 via email

kunalkushwaha commented Aug 20, 2018

thaJeztah Aug 20, 2018

thaJeztah Aug 20, 2018

kunalkushwaha Aug 22, 2018

kunalkushwaha commented Aug 23, 2018

tonistiigi Aug 24, 2018

tonistiigi Aug 24, 2018

tiborvass left a comment

thaJeztah left a comment

		}
		if cf, err := console.ConsoleFromFile(os.Stderr); err == nil && (progressOpt == "auto" \|\| progressOpt == "tty") {

--no-progress replaced with --progress #504

--no-progress replaced with --progress #504

Conversation

kunalkushwaha commented Jul 13, 2018

Choose a reason for hiding this comment

kunalkushwaha Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

tonistiigi commented Jul 25, 2018

Choose a reason for hiding this comment

thaJeztah commented Jul 25, 2018

kunalkushwaha commented Jul 25, 2018

kunalkushwaha commented Jul 26, 2018

tiborvass commented Aug 2, 2018

tonistiigi commented Aug 2, 2018

kunalkushwaha commented Aug 3, 2018

tiborvass commented Aug 3, 2018

tonistiigi commented Aug 3, 2018

tonistiigi commented Aug 3, 2018

tianon commented Aug 3, 2018

tiborvass commented Aug 3, 2018 • edited Loading

tianon commented Aug 4, 2018

tiborvass commented Aug 4, 2018

AntaresS commented Aug 4, 2018

kunalkushwaha commented Aug 6, 2018

tiborvass commented Aug 6, 2018

kunalkushwaha commented Aug 17, 2018

Choose a reason for hiding this comment

tiborvass commented Aug 17, 2018

tiborvass commented Aug 17, 2018

kunalkushwaha commented Aug 17, 2018 via email

kunalkushwaha commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalkushwaha commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

kunalkushwaha Jul 17, 2018 •

edited

Loading

tiborvass commented Aug 3, 2018 •

edited

Loading