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.

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

Simplify checkIfTerminal for Windows #1088

Merged
merged 1 commit into from May 19, 2020
Merged

Conversation

tklauser
Copy link
Contributor

@tklauser tklauser commented Jan 12, 2020

Instead of relying on EnableVirtualTerminalProcessing from
github.com/konsorten/go-windows-terminal-sequences which just calls
GetConsoleMode, sets ENABLE_VIRTUAL_TERMINAL_PROCESSING and calls
SetConsoleMode with the new modified mode, implement it directly inside
checkIfTerminal. This also avoids the duplicate call to GetConsoleMode
and gets rid of an external dependency.

Instead of relying on EnableVirtualTerminalProcessing from
github.com/konsorten/go-windows-terminal-sequences which just calls
GetConsoleMode, sets ENABLE_VIRTUAL_TERMINAL_PROCESSING and calls
SetConsoleMode with the new modified mode, implement it directly inside
checkIfTerminal. This also avoids the duplicate call to GetConsoleMode.
@tklauser
Copy link
Contributor Author

tklauser commented May 6, 2020

Rebased on latest master. Would be nice to get this merged in order to get rid of an additional dependency (which is unused on every platform but Windows). Is there anything else needed from my side? /cc @dgsb

@thaJeztah
Copy link
Collaborator

nice! Was recently working on something similar (didn't open a PR for that one yet; thanks for reminding me 😂 https://github.com/moby/term/compare/master...thaJeztah:deprecate_isconsole?expand=1)

Copy link
Collaborator

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Collaborator

@dgsb ptal

@dgsb
Copy link
Collaborator

dgsb commented May 18, 2020

hello we are indeed getting rid of a dependency but we get another instead which seems a way bigger package. What is the advantage of that ?

@tklauser
Copy link
Contributor Author

hello we are indeed getting rid of a dependency but we get another instead which seems a way bigger package. What is the advantage of that ?

github.com/sirupsen/logrus already depends on golang.org/x/sys for all other platforms than Windows. So it would streamline this dependency for all platforms. Also, x/sys/windows is maintained by the Go project and the official replacement for the syscall package, see https://github.com/golang/go/blob/2b70ffe9307c0992e28513ba25081d767b5937b2/src/syscall/syscall.go#L21-L25, so it's well maintained.

I agree the dependency itself is bigger but the Go linker will strip away any unused symbols, so in terms of binary size we should only pay for functionality we actually use.

@thaJeztah
Copy link
Collaborator

golang.org/x/sys is quite commonly used in projects (and, as mentioned, already a dependency for the linux builds), so from my perspective should not be a blocker. As discussed with @tklauser in moby/term#10 (comment), implementing using "syscall" instead is possible (if there's concerns about adding this dependency for windows-only projects), but may not be "recommended" given that the syscall package is in maintenance mode.

@dgsb
Copy link
Collaborator

dgsb commented May 19, 2020

Ok, makes sense

@dgsb dgsb merged commit 0de04f1 into sirupsen:master May 19, 2020
@tklauser tklauser deleted the simplify-windows branch May 19, 2020 14:57
cgxxv pushed a commit to cgxxv/logrus that referenced this pull request Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants