use golang.org/x/term for terminal detection#1484
use golang.org/x/term for terminal detection#1484thaJeztah wants to merge 2 commits intosirupsen:masterfrom
Conversation
go.mod
Outdated
| require ( | ||
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/pmezard/go-difflib v1.0.0 // indirect | ||
| golang.org/x/sys v0.13.0 // indirect |
There was a problem hiding this comment.
Keeping the version used before, but technically this could be v0.2.0
There was a problem hiding this comment.
Pull request overview
This PR consolidates platform-specific terminal detection code by replacing custom implementations with the standard golang.org/x/term package. The change simplifies maintenance by removing 10 platform-specific files and replacing them with a single, cross-platform implementation in text_formatter.go.
Changes:
- Replaced platform-specific terminal detection functions with
golang.org/x/term.IsTerminal - Removed 10 platform-specific terminal detection files (Windows, Unix, BSD, Solaris, App Engine, etc.)
- Updated dependencies: added
golang.org/x/term v0.2.0and movedgolang.org/x/systo indirect dependency
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| text_formatter.go | Replaced checkIfTerminal() call with inline type switch using term.IsTerminal() |
| terminal_check_windows.go | Deleted - Windows-specific terminal detection and VT processing |
| terminal_check_unix.go | Deleted - Unix-specific terminal detection using ioctl |
| terminal_check_bsd.go | Deleted - BSD-specific terminal detection |
| terminal_check_solaris.go | Deleted - Solaris-specific terminal detection |
| terminal_check_appengine.go | Deleted - App Engine-specific behavior (always returned true) |
| terminal_check_notappengine.go | Deleted - Non-App Engine wrapper function |
| terminal_check_no_terminal.go | Deleted - Fallback for js/nacl/plan9 |
| terminal_check_js.go | Deleted - JavaScript-specific fallback |
| terminal_check_wasi.go | Deleted - WASI-specific fallback |
| terminal_check_wasip1.go | Deleted - WASIP1-specific fallback |
| go.mod | Added golang.org/x/term v0.2.0, moved golang.org/x/sys to indirect |
| go.sum | Added checksums for golang.org/x/term v0.2.0 and updated golang.org/x/sys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
text_formatter.go
Outdated
| switch v := entry.Logger.Out.(type) { | ||
| case *os.File: | ||
| f.isTerminal = term.IsTerminal(int(v.Fd())) // #nosec G115 -- Ignore "integer overflow conversion"; Fd() is OS-provided, not user-controlled | ||
| default: | ||
| f.isTerminal = false | ||
| } |
There was a problem hiding this comment.
The removal of terminal_check_appengine.go changes the behavior for App Engine environments. Previously, checkIfTerminal would return true in App Engine environments. With the new implementation, it will return false (since App Engine's output is typically not an os.File). This could be a breaking change for users who relied on the old behavior. Consider documenting this change or verifying that returning false is the intended behavior for App Engine.
There was a problem hiding this comment.
Ah, right, hm... wonder if appengine is still a thing, and if it still needs the special build-tag; I see it was added in
d1cbc2e to
87d959b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
terminal_check.go
Outdated
|
|
||
| "golang.org/x/term" | ||
| ) | ||
|
|
||
| func checkIfTerminal(w io.Writer) bool { | ||
| if f, ok := w.(*os.File); ok { | ||
| return term.IsTerminal(int(f.Fd())) // #nosec G115 -- Fd() is OS-provided, not user-controlled; overflow not attacker-influenced |
There was a problem hiding this comment.
On Windows the previous implementation enabled ENABLE_VIRTUAL_TERMINAL_PROCESSING via SetConsoleMode when w was *os.File. Switching to term.IsTerminal drops that side effect, which can regress ANSI color output when users set ForceColors without wrapping output via go-colorable. Consider reintroducing a Windows-specific path that enables VT processing (or explicitly document that Windows users must use go-colorable / enable VT themselves).
| "golang.org/x/term" | |
| ) | |
| func checkIfTerminal(w io.Writer) bool { | |
| if f, ok := w.(*os.File); ok { | |
| return term.IsTerminal(int(f.Fd())) // #nosec G115 -- Fd() is OS-provided, not user-controlled; overflow not attacker-influenced | |
| "runtime" | |
| "syscall" | |
| "unsafe" | |
| "golang.org/x/term" | |
| ) | |
| var ( | |
| kernel32 = syscall.NewLazyDLL("kernel32.dll") | |
| getConsoleMode = kernel32.NewProc("GetConsoleMode") | |
| setConsoleMode = kernel32.NewProc("SetConsoleMode") | |
| ) | |
| func enableVirtualTerminalProcessing(f *os.File) { | |
| if runtime.GOOS != "windows" { | |
| return | |
| } | |
| const ENABLE_VIRTUAL_TERMINAL_PROCESSING = 0x0004 | |
| handle := uintptr(f.Fd()) // #nosec G115 -- Fd() is OS-provided, not user-controlled; overflow not attacker-influenced | |
| var mode uint32 | |
| r1, _, _ := getConsoleMode.Call(handle, uintptr(unsafe.Pointer(&mode))) | |
| if r1 == 0 { | |
| return | |
| } | |
| if mode&ENABLE_VIRTUAL_TERMINAL_PROCESSING != 0 { | |
| return | |
| } | |
| _, _, _ = setConsoleMode.Call(handle, uintptr(mode|ENABLE_VIRTUAL_TERMINAL_PROCESSING)) | |
| } | |
| func checkIfTerminal(w io.Writer) bool { | |
| if f, ok := w.(*os.File); ok { | |
| isTerminal := term.IsTerminal(int(f.Fd())) // #nosec G115 -- Fd() is OS-provided, not user-controlled; overflow not attacker-influenced | |
| if isTerminal { | |
| enableVirtualTerminalProcessing(f) | |
| } | |
| return isTerminal |
2b42261 to
c5f4554
Compare
Using a low version of this dependency, as the `IsTerminal` function was not modified in newer versions; golang/term@v0.2.0...v0.40.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
c5f4554 to
e013d10
Compare
3934ae5 to
6900582
Compare
6900582 to
a0bdca4
Compare
Using a low version of this dependency, as the
IsTerminalfunction was not modified in newer versions;golang/term@v0.2.0...v0.40.0