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

fix: retain and restore terminal mode on Windows #4369

Closed
wants to merge 2 commits into from
Closed

Conversation

mohammed90
Copy link
Member

Given this is caused by a dependency, not possible to remove the dependency, and not easy to excise the code from the dependency due to their needs, I'm attempting to retain the original terminal mode bits and to be set before exit. Bitwise OR operations aren't reversible, so retaining the original state is the other option.

Fixes #4251

@francislavoie
Copy link
Member

Thanks, looks reasonable. Just need someone affected to confirm it fixes it.

cmd/main.go Outdated Show resolved Hide resolved
@@ -50,6 +50,9 @@ func init() {
// Main implements the main function of the caddy command.
// Call this if Caddy is to be the main() of your program.
func Main() {
if err := resetTerminalState(); err != nil {
Copy link

@egonelbre egonelbre Oct 4, 2021

Choose a reason for hiding this comment

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

Doesn't this disable the terminal change altogether? Wouldn't that break the dependency -- if it's being used? I assume it was enabled for a reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding of their code is they're using it because they have interactive TUI. Running go mod why against the offending dependency shows:

2021-10-04 22:57:09  ~/projects/caddyserver/caddy git:(fix-4251) ✗ $ go mod why go.step.sm/cli-utils/ui
# go.step.sm/cli-utils/ui
github.com/caddyserver/caddy/v2/modules/caddypki
github.com/smallstep/certificates/authority
github.com/smallstep/certificates/kms
github.com/smallstep/certificates/kms/softkms
go.step.sm/cli-utils/ui

softkms uses the go.step.sm/cli-utils/ui package to prompt for password. The only password prompt Caddy has is on run of caddy trust which uses github.com/FiloSottile/mkcert which doesn't use go.step.sm/cli-utils/ui for the prompt.

Choose a reason for hiding this comment

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

Ok, just making sure.

@mholt mholt added under review 🧐 Review is pending before merging upstream ⬆️ Relates to some dependency of this project labels Oct 4, 2021
@mholt
Copy link
Member

mholt commented Oct 4, 2021

Thanks for the patch; as discussed in Slack, we're going to wait on this until we can determine if upstream can fix it first.

@mholt
Copy link
Member

mholt commented Oct 8, 2021

Great, looks like this was fixed upstream (thank you @maraino!) so I will close this PR. Thank you very much for making it Mohammed!

@mholt mholt closed this Oct 8, 2021
@mholt mholt removed the under review 🧐 Review is pending before merging label Oct 8, 2021
@mohammed90 mohammed90 deleted the fix-4251 branch October 8, 2021 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working upstream ⬆️ Relates to some dependency of this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running Caddy breaks terminal, ANSI escape codes are printed
4 participants