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

Require xterm-style virtual terminal on all platforms, including Windows #27487

Merged
merged 6 commits into from
Jan 13, 2021

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented Jan 12, 2021

Since very early versions of Terraform we've been supporting colored output from our CLI in the Microsoft Windows console using go-colorable, which aims to translate in-band virtual terminal sequences into out-of-band classic Windows Console API calls interspersed with calls to write literal characters to the terminal filehandle.

For several years now Windows 10 updates have supported xterm-style virtual terminal sequences in the Windows console, but only after opting in. Somewhat recently we updated go-colorable in #26588 to get a version which would make use of that mechanism if available, but we retained the existing translation layer for console windows where VT mode wasn't available, particularly on Windows 8. However, that effectively meant that we needed to stay constrained by the subset of formatting options supported in the classic console API (which were in turn inspired by the capabilities of VGA text mode) to ensure that the colorable translations would work effectively in consoles without VT mode enabled.

Microsoft now recommends migrating to VT instead of the classic console API, so this change now removes the colorable translation layer altogether and imposes some new (breaking) requirements:

  • Terraform requires that the attached terminal must support UTF-8 input and output on all platforms. Previously Terraform largely limited its output to ASCII and let any user-supplied non-ASCII characters render incorrectly on Windows, because the classic Windows console can only render characters belonging to the system's current ANSI codepage.
  • Terraform assumes that the recipient of its output can understand xterm-style escape sequences on all platforms, unless that's explicitly disabled by setting the -no-color option. Previously Terraform made that requirement except on Windows, where it would use colorable as a translation layer.

The upshot of the above is that future versions of Terraform will require a new enough version of Windows that its console supports both the new Unicode text buffer and virtual terminal mode. These capabilities were introduced and gradually improved throughout the early Windows 10 updates, so we recommend using the latest Windows 10 release for best results. Windows 8 and earlier do not have these new features and so are no longer officially supported, although may still be usable through third-party virtual terminal software or with the -no-color option to disable terminal sequences.

(Nitpicker's Corner: Technically we already dropped the go-colorable translation layer in earlier work in the 0.15 cycle, though it seems that it was unintentional. Prior to this PR we had broken output on Windows even on Windows 10, so this PR could be understood as a fix for that bug, though it's a fix in the form of making the previous behavior obsolete, rather than actually fixing it.)


The main event in this PR is the introduction of the package internal/terminal as a thin abstraction to deal with the special additional console initialization required on Windows. Through that package we can switch opt-in to the modern Windows Console features we need and portably determine whether each of the standard I/O handles is connected to a terminal or to something else.

In order to exercise and test that new implementation, I also did some initial work to start to make use of these new capabilities:

  • I changed command.Meta.StdinPiped to use internal/terminal instead of helper/wrappedstreams, because the latter does not implement correct behavior on Windows. This fixes "terraform console" wonky input handling on Windows #18242 (both of the main oddities described there) because terraform console relies on Meta.StdinPiped to decide whether to run in interactive mode or batch mode, and internal/terminal will now give it a more reliable answer on Windows.
  • I made the diagnostic renderer use the actual terminal width for its text wrapping, where available, thus addressing a long-standing TODO in meta.showDiagnostics.
  • I made the diagnostic renderer use Unicode box drawing characters for the visual separator on the indication of values that are in scope for the related expression, rather than the "ASCII art" we were using before.
  • I made the paragraphs of text in the terraform plan output respect the terminal width when wrapping, rather than being always hard-wrapped as before.
  • I made the terraform plan output use Unicode box drawing characters for its horizontal rule rather than ASCII hyphenminus, and made it fill the full with of the terminal when we're able to determine it.

Over time it'd be nice to make fuller use of our new terminal flexibility, but I had to draw the line somewhere for this first iteration and I probably drew it too late here, with more changes in its scope than I ideally would have included, but I wanted to make sure there were a few things here to test with in case others want to test it.

There are not unit tests for internal/terminal here because that package interacts with real system calls and therefore can't readily be mocked. If you want to do some interactive testing (either on Windows or elsewhere), here are some ways to observe the above:

  • Run terraform console both in interactive mode and batch mode and see how it behaves.
  • Run terraform plan in a configuration which requires at least one change, and observe the new unicode horizontal rule (which should hopefully fill your terminal width) and text wrapping.
  • Run terraform plan piped into less or redirected to a file and see that it selects a reasonable default output width because it's not connected to a terminal. (It still produces colors in that case, because we require an explicit -no-color to turn that off. More on that below.)
  • Run terraform plan in a configuration which produces an error or warning, and hopefully see it wrap at the terminal width rather than at the fixed point as before. Bonus: if you generate an error which includes information about the variables contributing to an expression then you should also see the new Unicode box drawing characters there.
  • And of course, anything else you want to test too. It's important that this hasn't regressed any behaviors I didn't intentionally modify! Try piping output and I/O redirection at your shell too, since that should affect whether Terraform detects the output as a terminal.

This closes #22487, and enables the nicer original incarnation of #27343 (which I'll revert back to if we merge this PR).

Bonus Chatter: Should we disable colors if stdout/stderr isn't a terminal?

While working on this I considered the possibility of having Terraform behave as if -no-color were set automatically if it detects that the relevant output stream isn't a real virtual terminal. However, I ultimately decided against that for the following two reasons.

  1. Terraform is commonly run in automation systems that capture stdout/stderr and render it to a log with interpretation of terminal formatting characters, even though the output streams are in that case typically connected to pipes rather than to a pseudo-terminal. Disabling our colored output for non-terminals would therefore disable the colors for those systems. We've always had the -no-color option to allow explicitly disabling the terminal formatting sequences for situations that don't support them, and so I preserved that precedent here.

    With that said, I hope that in future we'll begin making more extensive use of other terminal capabilities, such as redrawing parts of the output to produce update-in-place progress indicators. Once we start doing things like that, it could be reasonable to enable them only for a true terminal because those operations typically require a real terminal buffer and are thus unlikely to be supported by simple log streamers, even if they happen to support formatting sequences. The metadata about whether stdout and stderr are detected as terminals is available for us to use down the road once we have some real use-cases to evaluate.

  2. Part of our story for allowing Terraform to still be used on Windows 8 (albeit only on a best-effort basis) is allowing it to be used inside third-party virtual terminal fakers, like mintty. This PR does include some heuristics (via the upstream go-isatty) to try to recognize some of these and pretend they are terminals, but classic Windows has no real concept of a pseudo-terminal and so really we end up connected to pipes and we won't be able to heuristically recognize all such implementations. Continuing to emit terminal formatting sequences even when the output isn't detected as a terminal therefore makes the formatting more likely to work in a faked VT, even if we'd otherwise only recognize it as a pipe.

    A notable downside here is that running terraform.exe in a classic Windows Console (without a fake VT) will cause it to emit raw terminal escape sequences into the console buffer, which is not ideal. Terraform running in that context is now a best-effort sort of thing anyway, so that seems like a reasonable compromise. Folks who do want to run Terraform in that context can use -no-color to disable the VT formatting sequences and use the uncolored output instead.

Perhaps we can revisit each of these tradeoffs later, but for now I think continuing to emit VT formatting by default, regardless of the type of output handle, is the best compromise to minimize the impact of this change.

This is a helper package that creates a very thin abstraction over
terminal setup, with the main goal being to deal with all of the extra
setup we need to do in order to get a UTF-8-supporting virtual terminal
on a Windows system.
We need to call into terminal.Init in early startup to make sure that we
either have a suitable Terminal or that we disable attempts to use virtual
terminal escape sequences.

This commit gets the terminal initialized but doesn't do much with it
after that. Subsequent commits will make more use of this.
@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #27487 (e272c5d) into master (62815de) will decrease coverage by 0.02%.
The diff coverage is 35.44%.

Impacted Files Coverage Δ
backend/local/backend.go 48.34% <ø> (ø)
command/format/trivia.go 0.00% <0.00%> (ø)
commands.go 0.68% <0.00%> (-0.01%) ⬇️
main.go 37.14% <17.24%> (-2.49%) ⬇️
backend/local/cli.go 11.11% <30.00%> (+11.11%) ⬆️
command/meta.go 77.15% <70.00%> (+0.19%) ⬆️
backend/local/backend_plan.go 71.42% <100.00%> (+0.34%) ⬆️
command/format/diagnostic.go 64.39% <100.00%> (ø)
command/meta_backend.go 56.90% <100.00%> (+0.07%) ⬆️
command/show.go 54.28% <100.00%> (ø)
... and 5 more

@pkolyvas
Copy link
Contributor

This looks great! Thanks for covering all the bases so thoroughly here.

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

The code changes look great! Works great for me on macOS (Terminal.app and iTerm 2) and Windows 10 (cmd.exe and Powershell). It's a neat side effect to have Terraform now wrapping at terminal width instead of an arbitrary position.

backend/local/cli.go Outdated Show resolved Hide resolved
backend/local/cli.go Outdated Show resolved Hide resolved
Here we propagate in the initialized terminal.Streams from package main,
and then onwards to backends running in CLI mode.

This also replaces our use of helper/wrappedstreams to determine whether
stdin is a terminal or a pipe. helper/wrappedstreams returns incorrect
file descriptors on Windows, causing StdinPiped to always return false on
that platform and thus causing one of the odd behaviors discussed in

Finally, this includes some wrappers around the ability to look up the
number of columns in the terminal in preparation for use elsewhere. These
wrappers deal with the fact that our unit tests typically won't populate
meta.Streams.
We now require the output to accept UTF-8 and we can determine how wide
the terminal (if any) is, so here we begin to make use of that for the
"terraform plan" command.

The horizontal rule is now made of box drawing characters instead of
hyphens and fills the whole terminal width.

The paragraphs of text in the output are now also wrapped to fill the
terminal width, instead of the hard-wrapping we did before.

This is just a start down the road of making better use of the terminal
capabilities. Lots of other commands could benefit from updates like these
too.
We were previously using some ASCII art to create some visual divisions
between parts of the diagnostic output. Now that we are requiring a UTF-8
terminal we can print out box drawing characters instead.
In some terminal emulators, writing a character into the last column on a
row causes the terminal to immediately wrap to the beginning of the next
line, even if the very next character in the stream is a hard newline.
That can then lead to errant blank lines in the final output which make
it harder to navigate the visual hierarchy.

As a compromise to avoid this, we'll format our horizontal rules and
paragraphs to one column less than the terminal width. That does mean that
our horizontal rules won't _quite_ cover the whole terminal width, but
it seems like a good compromise in order to get consistent behavior across
a wider variety of terminal implementations.
@ghost
Copy link

ghost commented Feb 13, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants