-
Notifications
You must be signed in to change notification settings - Fork 468
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
look for stdout instead of stderr for plain output #2434
Conversation
Signed-off-by: kasperkantz <kasperkantz@outlook.com>
Buildx should already support |
@jedevc currently it is not behaving as expected? this pr is trying to make it helps with some ci systems like appveyor (where currently the output gets cluttered), as NO_COLOR is more widespread than BUILDKIT_PROGRESS. |
This is what the option does today in buildkit. I don't think that |
ok, removed NO_COLOR bit |
@@ -308,7 +308,7 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) | |||
} | |||
|
|||
var term bool | |||
if _, err := console.ConsoleFromFile(os.Stderr); err == nil { | |||
if _, err := console.ConsoleFromFile(os.Stdout); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you're trying to achieve with this change. This func just checks if provided "file" is a console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you meant to have progress output to stdout this is not something we are going to do. Progress, diagnostics or logs go to stderr. See also moby/buildkit#1186 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not that? currently docker build ... 2>&1 | tee
switches to plain text and docker build ... | tee
does not. with this change it would in the latter case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stdout is used for build result with -o
. This change makes it so that buildx -o type=tar
does not produce TTY output anymore. Use --progress
to control what kind of output you wish to achieve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i've opened appveyor/build-images#152. the issue was with docker-build cluttering the logs in appveyor ci, because unlike github actions, appveyor does not run the job with redirected output and it keeps on printing the whole dockerfile content with every progress line... 60k+ lines in logs from RUN apt install ...
alone in debian container.
expecting everyone to specify the progress=plain is bit counter-intuitive. perhaps docker can check for CI=ignorecase(true)
and CI=1
well-known environment variable for CI systems in the wild and make the decision for the user? avid users can still optin to (broken) tty logs.
make the primary decision based on 'stdout is redictected' instead of stderr; like many software.
e.g.
docker build ... | tee
should be enough to make it realize term is a nay instead ofdocker build ... 2>&1 | tee
.