Windows CLI Ansi Rewrite #13224
Windows CLI Ansi Rewrite #13224
Conversation
Signed-off-by: Brendan Dixon <brendand@microsoft.com>
|
Cool! :) |
|
ping @icecrime |
|
|
| @@ -59,6 +59,8 @@ clone git github.com/go-fsnotify/fsnotify v1.2.0 | |||
|
|
|||
| clone git github.com/go-check/check 64131543e7896d5bcc6bd5a76287eb75ea96c673 | |||
|
|
|||
| clone git github.com/Azure/go-ansiterm | |||
There was a problem hiding this comment.
It's a much smaller pull request now. And in the mean time I've fixed all known bugs. There are no panics or odd behavior in any known real world scenario. The synthetic test vttest doesn't pass, but iTerm doesn't pass that test either.
|
Hi @icecrime @tiborvass @crosbymichael, Gabriel has just updated this pull request. We are now vendoring http://github.com/Azure/go-ansiterm package and the docker codebase only has a little piece of integration code. Please review and let @gabrielhartmann know what you think. 😄 |
|
Thanks guys! |
|
This is a lot better. A LOT better!. There are still some bugs though. Screenshots below. I tried docker pull Ubuntu:15.04 from Linux (against a Linux daemon). Compare with the output on Windows doing the same, targetting both the same Linux daemon, and a Windows daemon for good measure ( ;) ). Note how the Windows output is not quite correct. Using 1.7 master with the commits from this PR cherry-picked on Windows. On Linux, using 1.7 master. |
|
@jhowardmsft apparently it's hard to take a 3-way diff by eye. what's the difference between these 3? |
|
Notice how in Windows, no gap between "15.04 Pulling from Ubuntu" and the next lines. Also notice how c6a row appears twice in the first picture. And how the result of the "second" c6a is still in-progress. You can get the same by pointing the Windows CLI at Ubuntu - just the graph layers weren't consistent between the Linux and Windows daemons. Hence I know it's not a Windows daemon issue. Should have done a better screen-shot ;) If it helps, it's easier to see while the pull is in progress. Here's a sample of a Windows client pointing at a Linux daemon during the pull (after a docker rmi Ubuntu:15.04 so graph is clean). See the duplicate line at the bottom. |
|
One more screenshot - compare with conemu with no emulation from docker cli. Looks more correct (although that's not perfect either), but the vertical spacing is correct certainly which is what seems to be the predominant issue with the new code. (Pointing at a Linux daemon)
|
|
@jhowardmsft I've captured the vertical spacing issue here. Azure/go-ansiterm#1 One of the key benefits of this pull request is that incremental improvements to the terminal emulation can be made against the go-ansiterm package without touch Docker's code except to update the vendor script. This is the first draft of terminal emulation and is not my only work item at the moment. So releasing the code so more than one person can contribute seems like a good idea to me. Given that the current implementation crashes all the time and rarely produces even approximately correct output, I don't think this vertical spacing issue should block the pull request. |
|
I agree that getting this in is definitely beneficial. LGTM 👍 |
|
This change makes Ctrl-C almost worthless. Before this change, Ctrl-C was very responsive. Now, it takes many seconds before the effects of pushing Ctrl-C. To see this, try: This will run for a long time unless interrupted. Try hitting Ctrl-C and you'll see that the results continue scrolling for about 15 seconds or so. |
|
@jstarks Issues 2 and 3 from https://github.com/Azure/go-ansiterm address the Ctrl-C problem. We have agreed this shouldn't block the PR in an offline discussion. @icecrime @tiborvass @crosbymichael: Is there anything left blocking this PR? It would be nice to get this in pretty soon. |
|
Looks like this PR is stalling for no reason. Ping @docker/core-maintainers1, this PR improves the ANSI emulation in Windows CLI a great deal and extracts all the Windows related code to a separate pkg, then vendors it. PTAL. @icecrime @tiborvass @crosbymichael 1 looks like I can't just ping like that. |
|
|
||
| "github.com/Sirupsen/logrus" | ||
| "github.com/docker/docker/pkg/term/winconsole" | ||
| "github.com/docker/docker/pkg/term/windows" | ||
| "github.com/docker/docker/vendor/src/github.com/Azure/go-ansiterm/winterm" |
There was a problem hiding this comment.
This should just be github.com/Azure/go-ansiterm/winterm
There was a problem hiding this comment.
@cpuguy83 Wouldn't this directly reference the Azure/go-ansiterm repo? Shouldn't the references go through the vendor version in the Docker repo? Is there some other mechanism I'm unaware of which intercepts these references? Do you mean only this file should reference the Azure repo?
I'm ok with everything directly referencing the Azure repo, but I thought the Docker team preferred snapshots of vendor code tied to particular releases/tags/commits.
There was a problem hiding this comment.
Yes, the GOPATH on the build env is set to /go:/go/src/github.com/docker/docker/vendor
There was a problem hiding this comment.
@gabrielhartmann the github.com/Sirupsen/logrus above is vendored as well. as @cpuguy83 said, the path of vendored packages is added to GOPATH while building the docker binary and thus we don't need to have the full pkg path.
For windows CLI , the GOPATH must look like: GOPATH=/c/gopath:/c/gopath/src/github.com/docker/docker/vendor (you can find more here)
There was a problem hiding this comment.
@cpuguy83 @ahmetalpbalkan Fixed package references and tested.
The Ansi parser and their associated actions have been decoupled. Now parsing results in call backs to an interface which performs the appropriate actions depending on the environment. Right now there is a test implementation and an initial Windows implementation. This improvement provides a functional Vi experience and the vttest no longer panics. Signed-off-by: Gabriel Hartmann <gabhart@microsoft.com>
|
@cpuguy83 thanks for reviewing. Looks like the commit is updated now. Can we have more core-maintainers looking at this please? The PR has approached to the 1-month mark. 😄 |
|
LGTM But we should to a follow up PR to change the |
|
With this change, docker client can no longer start on Nano Server because of an unnecessary call to the missing GetCurrentConsoleFont (I don't see why GetWinsize needs to return font dimensions on Windows). I've got a fix in microsoft/docker@8652dd3, but it can't be merged because it changes the vendored go-ansiterm. Could one of @gabrielhartmann, @ahmetalpbalkan, @brendandixon take care of this before this gets merged? |
|
@jstarks can you please send the PR to go-ansiterm? Then we can fix term_windows.go & re-vendor the pkg here. |
|
Per @jhowardmsft request |
The Ansi parser and their associated actions have been decoupled. Now parsing results in call backs to an interface which performs the appropriate actions depending on the environment. This improvement provides a functional Vi experience and the vttest no longer panics. This PR replaces docker/docker moby#13224 with the latest console updates. Signed-off-by: John Howard <jhoward@microsoft.com>
The Ansi parser and their associated actions have been decoupled. Now parsing results in call backs to an interface which performs the appropriate actions depending on the environment. This improvement provides a functional Vi experience and the vttest no longer panics. This PR replaces docker/docker moby#13224 with the latest console updates. Signed-off-by: John Howard <jhoward@microsoft.com>
The Ansi parser and their associated actions have been decoupled. Now parsing results in call backs to an interface which performs the appropriate actions depending on the environment. This improvement provides a functional Vi experience and the vttest no longer panics. This PR replaces docker/docker moby#13224 with the latest console updates. Signed-off-by: John Howard <jhoward@microsoft.com>
A large portion of the Windows CLI has been rewritten. Ansi parsing and the actions taken as a result of Ansi commands are now decoupled. There have also been a number of bug fixes in the actions taken in response to Ansi commands in the Windows environment.
Vi now functions as expected and the vttest no longer panics. Further improvement towards full Ansi terminal emulator compliance is needed, but much of the standard most commonly used features are now properly supported.