Skip to content

Support read write windows#25

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
mat007:support-read-write-windows
Jun 4, 2018
Merged

Support read write windows#25
crosbymichael merged 2 commits intocontainerd:masterfrom
mat007:support-read-write-windows

Conversation

@mat007
Copy link
Copy Markdown
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
Copy Markdown
Contributor

mlaventure commented Jun 1, 2018

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

@mat007
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown
Member

crosbymichael commented Jun 4, 2018

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

2 more reviewers

@ijc ijc ijc approved these changes

@mlaventure mlaventure mlaventure approved these changes

Reviewers whose approvals may not affect merge requirements

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