Skip to content

Fix #28814: use emulation for ConEmu and ConsoleZ #37600

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 15, 2018
Merged

Fix #28814: use emulation for ConEmu and ConsoleZ #37600

merged 1 commit into from
Aug 15, 2018

Conversation

edrevo
Copy link
Contributor

@edrevo edrevo commented Aug 7, 2018

- What I did

I remove a special-case handling of the ConEmu and ConsoleZ console emulators, which was causing problems with pipes. ConEmu and ConsoleZ will now be handled like any other console emulator in windows: by using emulated streams that handle VT codes inside Go.

- How I did it
By removing an if block

- How to verify it
I verified it on Windows 10 1803 (17134.112) by using ConEmu 180626 and ConsoleZ 1.18.3.18143

> echo 'FROM hello-world' | .\build\docker-windows-amd64.exe build -t temp -
Sending build context to Docker daemon  2.048kB
Step 1/1 : FROM hello-world
 ---> 2cb0d9787c4d
Successfully built 2cb0d9787c4d
Successfully tagged temp:latest
SECURITY WARNING: You are building a Docker image from Windows against a non-Windows Docker host. All files and directories added to build context will have '-rwxr-xr-x' permissions. It is recommended to double check and reset permissions for sensitive files and directories.
~\git\cli [master ≡ +0 ~1 -0 !]
> echo 'FROM hello-world' | docker build -t temp -
unable to prepare context: failed to peek context header from STDIN: Función incorrecta.

I have also verified that removing this block does not produce a regression on #25475

- Description for the changelog
Fix pipe handling in ConEmu and ConsoleZ

@edrevo edrevo mentioned this pull request Aug 7, 2018
Closed
@edrevo
Fix moby#28814
ea65a0b 
Signed-off-by: Ximo Guanter Gonzálbez <joaquin.guantergonzalbez@telefonica.com>
@edrevo
Copy link
Contributor Author

edrevo commented Aug 7, 2018

The failure in the z check seems unrelated to this PR:

09:39:41 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
09:39:41 
09:39:41 [d8094e6524686] waiting for daemon to start
09:39:41 [d8094e6524686] daemon started
09:39:41 
09:39:41 [d012e5bac33b2] waiting for daemon to start
09:39:41 [d012e5bac33b2] daemon started
09:39:41 
09:39:41 [da7a1822540d0] waiting for daemon to start
09:39:41 [da7a1822540d0] daemon started
09:39:41 
09:39:41 [d8094e6524686] exiting daemon
09:39:41 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded
09:39:41 [d012e5bac33b2] exiting daemon
09:39:41 [da7a1822540d0] exiting daemon

This was referenced Aug 7, 2018
Open
Open
@yongtang yongtang requested a review from lowenna August 11, 2018 16:45
@lowenna
Copy link
Member

lowenna commented Aug 13, 2018

I'm not sure about this fix not ever using either of these consoles. These two consoles being handled differently have been there for a long time - Conemu for several years, ConsoleZ was added more recently (circa 2 years ago). From the perspective that removal won't affect Windows native console, the change is fine by me. However, I can't say it LGTM without some weigh-in from both ConsoleZ and Conemu folks, and especially how it might affect their use on down-level (meaning all the way from Win7 to Win10RS4/1709) clients.

@thaJeztah @johnstep @Maximus5 @cpuguy83 WDYT?

@edrevo
Copy link
Contributor Author

edrevo commented Aug 14, 2018 •
edited
Loading

Thanks for the review @jhowardmsft! The code I am removing is certainly old, but so is the bug that it fixes, which was opened only a month after the original code was added.

Removing this code means that ConEmu/ConsoleZ will interact with a Docker client that behaves exactly the same way as it does in raw conhost. Since both of these consoles are designed to be drop-in replacements for the native console in Windows, it seems like having Docker behave the same way in all consoles is a bullet-proof way of avoiding console-specific bugs. As @Maximus5 says in this comment Maximus5/ConEmu#958 (comment), if Docker isn't working on ConEmu and it is behaving exactly the same as in raw conhost, there is a bug in ConEmu. But, if docker is doing special things for ConEmu and it is not working, then it is a problem in docker.

thaJeztah
thaJeztah approved these changes Aug 14, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM

largely the same for me; I'm not a Windows user, and don't use these, so can't say a lot about the change 😅

@Maximus5
Copy link

Let me clarify this.

ConEmu may act in two ways:

  1. Full emulation of conhost. That means that ConEmu just shows what conhost renders: docker -> WinAPI -> conhost -> ReadConsoleOutput in ConEmu -> drawing in VirtualConsole. And keyboard/mouse input is sent into docker input queue as WinAPI console events.
  2. POSIX mode, which may be implemented either by cygwin/msys connector (you don't run non cygwin/msys tools in this case) or just a switch -new_console:p. In that case ConEmu will "render" output from console application (docker) and post keyboard/mouse events as ANSI sequences usual to Unix terminal applications.

The problem of the first way - ConEmu can't manage ANSI output sequences and users will never get advantages like 256 or true-color output. With exception of that (256-color mode), docker will act in ConEmu exactly as in raw console.

Second way gives users true-color output and some ConEmu specific features. But users have to run docker with special ConEmu's switch or we need to hardcode this mode in both docker and ConEmu.

lowenna
lowenna approved these changes Aug 14, 2018
View reviewed changes
Copy link
Member

@lowenna lowenna left a comment

Choose a reason for hiding this comment

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

SGTM too

@codecov
Copy link

codecov bot commented Aug 14, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@f57f260). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #37600   +/-   ##
=========================================
  Coverage          ?   35.63%           
=========================================
  Files             ?      611           
  Lines             ?    44962           
  Branches          ?        0           
=========================================
  Hits              ?    16022           
  Misses            ?    26727           
  Partials          ?     2213

@yongtang yongtang merged commit 74c43af into moby:master Aug 15, 2018
@edrevo
Copy link
Contributor Author

edrevo commented Aug 16, 2018

@thaJeztah, do I need to open a PR in docker/cli to update the dependency on moby? Or will the change trickle down auto-magically?

@edrevo edrevo deleted the conemu-fix branch August 16, 2018 07:24
@tiborvass tiborvass mentioned this pull request Aug 17, 2018
Merged
@xenoscopic xenoscopic mentioned this pull request Oct 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@lowenna lowenna lowenna approved these changes

Assignees
No one assigned
Labels
platform/windows status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@edrevo @lowenna @Maximus5 @thaJeztah @yongtang @GordonTheTurtle