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.

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

Support read write windows #25

Merged
merged 2 commits into from
Jun 4, 2018
Merged

Support read write windows #25

merged 2 commits into from
Jun 4, 2018

Conversation

mat007
Copy link
Contributor

@mat007 mat007 commented Jun 1, 2018

The windows implementation now follows the unix implementation more closely:
. the actual os.File passed is now used to create the console.
. Read and Write are now implemented by forwarding to the underlying os.File

This fixes the following panic:

C:\dev\src\github.com\containerd\containerd>bin\ctr i pull --user=me docker.io/library/hello-world:latest
Password: panic: not implemented on windows

goroutine 1 [running]:
github.com/containerd/containerd/vendor/github.com/containerd/console.(*master).Read(0xc0422b8000, 0xc0422ba000, 0x1000, 0x1000, 0x410473, 0x1c13f8, 0xaf1157)
        C:/dev/src/github.com/containerd/containerd/vendor/github.com/containerd/console/console_windows.go:153 +0x40

mat007 added 2 commits June 1, 2018 18:18
@mat007
Fix some typos in comments
6fe6f36
@mat007
Fix reading from and writing to console on windows
a7ba593 
The windows implementation now follows the unix implementation more closely:
. the actual `os.File` passed is now used to create the console.
. `Read` and `Write` are now implemented by forwarding to the underlying `os.File`
@crosbymichael crosbymichael requested a review from mlaventure June 1, 2018 20:59
@mlaventure
Copy link
Contributor

What's the behavior is I do a ConsoleFromFile(os.Stdout) and I send some VT100 control chars?

@mat007
Copy link
Contributor Author

mat007 commented Jun 2, 2018 •
edited
Loading

Starting with Windows 10 TH2 (v1511) terminals (cmd.exe and powershell) support Console Virtual Terminal Sequences.

This seems to be working great with cmd:
40878789-4e8c611e-6696-11e8-868d-84262456d675
and powershell (not the best choice of colors here though…):
40878807-a5a6eadc-6696-11e8-987d-6081b0eb2090

mlaventure
mlaventure approved these changes Jun 3, 2018
View reviewed changes
Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

@johnstep do you mind having a look too?

ijc
ijc approved these changes Jun 4, 2018
View reviewed changes
Copy link

@ijc ijc left a comment

Choose a reason for hiding this comment

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

FWIW LGTM too.

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 9290d21 into containerd:master Jun 4, 2018
crosbymichael added a commit to crosbymichael/containerd that referenced this pull request Jun 5, 2018
@crosbymichael
Bump console for windows tty handling
0cfca1e 
ref: containerd/console#25

Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael crosbymichael mentioned this pull request Jun 5, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ijc ijc ijc approved these changes

@mlaventure mlaventure mlaventure approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mat007 @mlaventure @crosbymichael @ijc