Added msys2/cygwin terminal detection support #485

ereOn · 2017-03-06T21:29:35Z

This PR adds support for MSYS/Cygwin terminal support detection on Windows (#484).

The strategy for the regular terminal is still to use GetConsoleMode to detect the classic cmd.exe console.

If that fails, instead of failing directly, we now fallback to the following sequence:

If the io.Writer's handle is a pipe handle, we move to 2.
If the pipe's name matches \\(cygwin|msys)-\w+-pty\d?-(to|from)-master, MSYS/Cygwin terminal is detected and we return true.

If any of the two points above fails, we return false like the previous implementation.

The exact same check is done in Vim, the popular UNIX text-editor.

I also added a test that validates (on Windows only) that STDOUT is detected as a terminal.

ereOn · 2017-03-06T21:33:39Z

log/term/terminal_windows_test.go

+}
+
+func TestIsTerminal(t *testing.T) {
+	handle, err := windows.GetStdHandle(windows.STD_OUTPUT_HANDLE)


The test directly fetches STDOUT using GetStdHandle because go test redirects its output (and so fails) but only if not run with -v which makes for a surprising behavior otherwise !

Please add this as a comment in the code.

ChrisHines · 2017-03-22T23:38:00Z

log/term/terminal_windows.go

 	"syscall"
 	"unsafe"
+
+	"golang.org/x/sys/windows"


I would rather avoid this enormous dependency since we're only using a few of its functions and they are not difficult to implement locally.

Doesn't this come with Go itself ?

I mainly used that instead of reinventing the wheel but I guess I can reimplement those in there if you think it is better form.

No, it requires using go get to retrieve and would need to be vendored by those that vendor their dependencies.

Done. I removed all the reference to external modules. I discovered that syscall actually already has a lot of useful stuff we needed, including for handling UTF16 names (on which Windows heavily relies).

ChrisHines · 2017-03-22T23:41:55Z

log/term/terminal_windows_test.go

+}
+
+func TestIsTerminal(t *testing.T) {
+	handle, err := windows.GetStdHandle(windows.STD_OUTPUT_HANDLE)


Please add this as a comment in the code.

ChrisHines · 2017-03-22T23:51:15Z

I have verified that this PR fixes IsTerminal for git bash on Windows. I discovered, however, that NewColorWriter does not work correctly for a git bash terminal anymore. It turns out that git bash handles ANSI color codes natively, and all of the syscalls in colorwriter_windows.go that are enabled when IsTerminal works properly for git bash break NewColorWriter. So we should address the logic in NewColorWriter to make sure it works properly on MSYS2/Cygwin in combination with the IsTerminal fix here.

ereOn · 2017-03-23T18:17:29Z

@ChrisHines I changed everything you mentionned except for your last comment about Git bash. I will look at this right now.

I noticed a weird behavior of go test: whenever it is launched without -v or ran indirectly (like go test ./...), os.Stdout is not a terminal. I had to explicitely open CONOUT$ (which directly points at the console output) to write a reliable test.

All the code now uses only syscall which avoid a useless dependency (thanks for the advice on this).

Let me know what you think of this.

ereOn · 2017-03-23T18:39:33Z

Updated the PR to restore the behavior.

I manually tested it with: https://gist.github.com/ereOn/32aafdf41c34b9749a90f464b2c96498.

ChrisHines

This looks really nice. I'm glad you broke out IsConsole and IsMSYSTerminal because I thought that would be a good approach to make NewColorWriter work well.

I've tested it in all three types of terminals and they all work great. I just have a few nits about the comments I'd like you to address and then we can merge this PR.

ChrisHines · 2017-03-24T01:26:00Z

log/term/terminal_windows.go

-	r, _, e := syscall.Syscall(procGetConsoleMode.Addr(), 2, fw.Fd(), uintptr(unsafe.Pointer(&st)), 0)
-	return r != 0 && e == 0
+
+	// If the terminal is a Windows console, succeed right away.


This comment seems obsolete. I would say it's not needed anymore.

Indeed. I had to push in a hurry, forgot about those comments. Fixed.

ChrisHines · 2017-03-24T01:26:31Z

log/term/terminal_windows.go

+	// If the terminal is a Windows console, succeed right away.
+	err := syscall.GetConsoleMode(handle, &st)
+
+	return (err == nil && st != 0)


I would like to see a comment explaining st and the significance of comparing it to 0.

ChrisHines · 2017-03-24T01:36:20Z

log/term/terminal_windows.go

+		return false
+	}
+
+	// The terminal is not a cmd.exe terminal, let's try to detect MSYS2 terminals.


It's not clear to me how we know it's not a cmd.exe terminal at this point.

Yep indeed. That was a leftover too. Removed.

ereOn · 2017-03-24T12:23:09Z

@ChrisHines I addressed all the remaining comment issues. Thanks for the feedback !

ChrisHines · 2017-03-24T23:12:15Z

Thanks for the contribution. Good work!

ereOn mentioned this pull request Mar 6, 2017

IsTerminal does not properly detect MSYS2 terminals on Windows. #484

Closed

ereOn commented Mar 6, 2017

View reviewed changes

ChrisHines requested changes Mar 22, 2017

View reviewed changes

ereOn force-pushed the isterminal_msys2_support branch from 0ef6cea to c7967ca Compare March 22, 2017 23:46

ereOn force-pushed the isterminal_msys2_support branch 5 times, most recently from b11d065 to 4c2de73 Compare March 23, 2017 18:14

ereOn force-pushed the isterminal_msys2_support branch from 2dbbb85 to ef721fa Compare March 23, 2017 18:38

ChrisHines requested changes Mar 24, 2017

View reviewed changes

Added msys2/cygwin terminal detection support

a62210c

ereOn force-pushed the isterminal_msys2_support branch from ef721fa to a62210c Compare March 24, 2017 12:22

ChrisHines approved these changes Mar 24, 2017

View reviewed changes

ChrisHines merged commit 5d93a23 into go-kit:master Mar 24, 2017

ereOn deleted the isterminal_msys2_support branch March 24, 2017 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added msys2/cygwin terminal detection support #485

Added msys2/cygwin terminal detection support #485

ereOn commented Mar 6, 2017 •

edited

Loading

ereOn Mar 6, 2017

ChrisHines Mar 22, 2017

ereOn Mar 23, 2017

ChrisHines Mar 22, 2017

ereOn Mar 22, 2017

ChrisHines Mar 23, 2017

ereOn Mar 23, 2017

ChrisHines Mar 22, 2017

ChrisHines commented Mar 22, 2017 •

edited

Loading

ereOn commented Mar 23, 2017 •

edited

Loading

ereOn commented Mar 23, 2017 •

edited

Loading

ChrisHines left a comment

ChrisHines Mar 24, 2017

ereOn Mar 24, 2017

ChrisHines Mar 24, 2017

ereOn Mar 24, 2017

ChrisHines Mar 24, 2017

ereOn Mar 24, 2017

ereOn commented Mar 24, 2017

ChrisHines commented Mar 24, 2017

Added msys2/cygwin terminal detection support #485

Added msys2/cygwin terminal detection support #485

Conversation

ereOn commented Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisHines commented Mar 22, 2017 • edited Loading

ereOn commented Mar 23, 2017 • edited Loading

ereOn commented Mar 23, 2017 • edited Loading

ChrisHines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ereOn commented Mar 24, 2017

ChrisHines commented Mar 24, 2017

ereOn commented Mar 6, 2017 •

edited

Loading

ChrisHines commented Mar 22, 2017 •

edited

Loading

ereOn commented Mar 23, 2017 •

edited

Loading

ereOn commented Mar 23, 2017 •

edited

Loading