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

build: --progress tty not applied in script #1288

Open
chrmarti opened this issue May 30, 2022 · 5 comments
Open

build: --progress tty not applied in script #1288

chrmarti opened this issue May 30, 2022 · 5 comments
Labels
area/buildkit kind/enhancement New feature or request

Comments

@chrmarti
Copy link

Description

Steps to reproduce the issue:

  1. Run docker build --progress tty . 2>&1 | cat.

Describe the results you received:

Plain progress output.

Describe the results you expected:

Colored progress output.

Additional information you deem important (e.g. issue happens only occasionally):

Output of docker version:

Client:
 Cloud integration: v1.0.24
 Version:           20.10.14
 API version:       1.41
 Go version:        go1.16.15
 Git commit:        a224086
 Built:             Thu Mar 24 01:49:20 2022
 OS/Arch:           darwin/amd64
 Context:           default
 Experimental:      true

Server: Docker Desktop 4.8.1 (78998)
 Engine:
  Version:          20.10.14
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.16.15
  Git commit:       87a90dc
  Built:            Thu Mar 24 01:46:14 2022
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.5.11
  GitCommit:        3df54a852345ae127d1fa3092b95168e4a88e2f8
 runc:
  Version:          1.0.3
  GitCommit:        v1.0.3-0-gf46b6ba
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

Output of docker info:

Client:
 Context:    default
 Debug Mode: false
 Plugins:
  buildx: Docker Buildx (Docker Inc., v0.8.2)
  compose: Docker Compose (Docker Inc., v2.5.0)
  sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc., 0.6.0)
  scan: Docker Scan (Docker Inc., v0.17.0)

Server:
 Containers: 1
  Running: 0
  Paused: 0
  Stopped: 1
 Images: 6
 Server Version: 20.10.14
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 3df54a852345ae127d1fa3092b95168e4a88e2f8
 runc version: v1.0.3-0-gf46b6ba
 init version: de40ad0
 Security Options:
  seccomp
   Profile: default
  cgroupns
 Kernel Version: 5.10.104-linuxkit
 Operating System: Docker Desktop
 OSType: linux
 Architecture: x86_64
 CPUs: 8
 Total Memory: 7.675GiB
 Name: docker-desktop
 ID: 6D7O:TXWP:VT2F:VRJ2:XZUR:GET6:FCUL:6N5R:L767:XGRJ:YXEM:H2GI
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 HTTP Proxy: http.docker.internal:3128
 HTTPS Proxy: http.docker.internal:3128
 No Proxy: hubproxy.docker.internal
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  hubproxy.docker.internal:5000
  127.0.0.0/8
 Live Restore Enabled: false

Additional environment details (AWS, VirtualBox, physical, etc.):

Docker Desktop for Mac.

@thaJeztah
Copy link
Member

Thanks for reporting; I think currently the "pretty" output is disabled if there's no TTY attached. This likely was done on purpose, due to how the tty output rewrites output (e.g. updates and removes lines by overwriting them), which in case no TTY is attached would print lines multiple times.

That said; it looks like there's a "TODO" in the code to improve this (color would likely still be possible, but work may be needed to omit the progress bars and rewriting);
https://github.com/docker/cli/blob/v20.10.17/cli/command/image/build_buildkit.go#L278-L283

@thaJeztah
Copy link
Member

Let me transfer this ticket to the buildx repository, as the BuildKit client code has been removed in master (the next release will use buildx to back docker build with BuildKit enabled)

@thaJeztah thaJeztah transferred this issue from docker/cli Aug 21, 2022
@thaJeztah
Copy link
Member

Looks like the equivalent of the code I linked above in buildx is here;

case PrinterModeAuto, PrinterModeTty:
if cons, err := console.ConsoleFromFile(out); err == nil {
c = cons
}
}
resumeLogs := logutil.Pause(logrus.StandardLogger())

I don't see a TODO there, but perhaps it's tracked elsewhere; @jedevc @crazy-max ?

@crazy-max crazy-max added kind/enhancement New feature or request and removed area/builder labels Sep 10, 2022
@AndASM
Copy link

AndASM commented Sep 17, 2022

What is the difference between auto and tty if this is an enhancement not a bug? It looks like we have three options for two choices currently.

Personally, when I explicitly ask for tty output, I want tty output. Including all the control codes to move around, draw progress bars, etc. If I wanted it to choose for me, I'd use auto, and if I wanted plain text, I'd use plain.

@jedevc
Copy link
Collaborator

jedevc commented Oct 25, 2022

What is the difference between auto and tty if this is an enhancement not a bug? It looks like we have three options for two choices currently.

This looks like a mistake to me, have opened a PR to correct: #1371. This function is pulled from buildkit, with some extra wrapping. However, they look like they've gotten out of sync.

Personally, when I explicitly ask for tty output, I want tty output. Including all the control codes to move around, draw progress bars, etc. If I wanted it to choose for me, I'd use auto, and if I wanted plain text, I'd use plain.

See above - buildx should (with the PR merged) raise an error if tty is specified and tty is not available.

As for forcing TTY when the output isn't a TTY - this isn't actually possible, at least without completely faking it. ConsoleFromFile comes from containerd. The Console interface it returns is quite complex - specifically, it ensures that the terminal can be resized. The buildkit output requires that the terminal size be known, so it can use the available space.

We could fake this interface by providing dummy values for all those resize functions... but I'm not convinced that's the right way forward. The buildkit TTY output is meant to be viewed from a TTY and doesn't make sense outside of that context. Viewing that output would then be completely wrong on a terminal that is a different size, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/buildkit kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants