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

Console: Fix problems when using CLI from MSYS2/GitBash/ConEmu #1394

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

jstine35
Copy link
Contributor

@jstine35 jstine35 commented Jan 5, 2021

what it does

  • Fix issue where console output is lost when running from MSYS2/GitBash CLI
  • Fix issue where pipe redirections would be overridden and output would always go to the attached console (this affected windows cmd prompt as well as other shell CLIs)
  • Simplifies some logic regarding registering of the standard output writer
  • A best-effort is made to honor any pre-existing pipe redirections which may have been specified by user or an invoking process.

test cases

  • run from GitBash/mintty and observe that help printed to the terminal:
$ duckstation -help
  • run from explorer/Visual Studio and observe that console window is allocated when Console is enabled.

remarks about standard outputs

I renamed some of the ConsoleOutput functions to StandardOutput, though only ones static to the scope of log.cpp for now. The terminology clarity can be helpful. The Console is very much a Microsoft Windows (MSW) term and is rarely used outside of the Windows ecosystem. Better terms are as follows:

  • Terminal (TTY) : this defines a window or socket/device that can receive Standard Output and provide Standard Input
  • Shell (Command Processor) : this defines an application that can receive Standard Input and execute commands, scripts, etc. and usually ensures any processes it creates have their standard pipes attached to the terminal (unless redirected).

When you combine a Terminal with a Shell, you get an interactive Command Prompt we all know. You can run shells without the terminal, and you can run terminals without a shell. Windows Console is a terminal without a shell. Windows Terminal (the new UWP replacement for Console available on the Windows App Store) is also a terminal without a shell.

Anyway, whether it's a terminal or console isn't important. What is important is that it's writing to "standard output" -- this is a construct that can, at the discretion of the invoking user or process, be either:

  • a terminal/console
  • a redirection to a file
  • a redirection to another process
  • a redirection to a socket or FIFO that can then be read asynchronously by other shells and processes (my favorite!)

This was all part of the rationale for me renaming 'ConsoleOutput' to 'StandardOutput'

WIN32 vs. _WIN32

I used _WIN32 to identify the Windows SDK platform. I consider the define preferable as it's a first-class buildsystem define, similar to _MSC_VER / __clang__ / __ANDROID__, etc. and such. By comparison, WIN32 must be manually specified by the makefile/vcxproj. In many newer project templates, I've seen the define missing entirely. My recommendation is to migrate all your instances of WIN32 to _WIN32 at some point.

 - Fix issue where console output is lost when running from MSYS2/GitBash CLI
 - Fix issue where pipe redirections would be overridden and otuput would always go to the attached console (this affected windows cmd prompt as well as other shell CLIs)
 - Simplify some logic regarding registering of the standard output writer
@stenzek
Copy link
Owner

stenzek commented Jan 6, 2021

Hi there! Many thanks for taking the time to clean up this old code, I wrote it probably 10 years ago now and it's definitely a bit crusty :)

Great to handle both the redirected and displayed-to-console options.

With regard to console vs standard output, yeah, very much a Windows-ism. If I remember correctly, the newer conhosts on Win10 understand ANSI escape sequences if you set things up correctly. So while we'd still have to allocate the console, we could remove the extra attribute setting calls, and use the same code on !WIN32+WIN32.

Downside would be garbage output on Win7, but I don't officially support it anyway, and it's not like it stops the application working. Worth it for less code imo.

This was all part of the rationale for me renaming 'ConsoleOutput' to 'StandardOutput'

Sounds reasonable. If the paths get unified then it's probably worth changing it in the UI too.

WIN32 vs. _WIN32

Also reasonable. Might do that some time :)

…teConsoleA code.

Also fixes android build failure (missing unistd.h)
@jstine35
Copy link
Contributor Author

jstine35 commented Jan 7, 2021

I confirmed the ANSI escape codes work fine on Win10 console (this I didn't know until you mentioned it). Moreover, Microsoft is pulling the rug out from under AllocConsole(). It's now depreciated in favor of some very-over-engineered pseudo-console API (or pseudo-terminal -- they confusingly use the names interchangeably).

I tried to read up on it but I have no idea yet how it's actually supposed to work, or how it'll impact things like "where did my printf() go??") later on. So I'm just going to pretend it doesn't exist until something forces me to dive into it (and hopefully that day never comes).

Also, for Win7 users, there's always winpty which acts as a translation layer for making old Windows Console behave like a unix tty.

With all that in mind, I followed your advice and removed the WriteConsole calls entirely. :)

@stenzek
Copy link
Owner

stenzek commented Jan 7, 2021

So I'm just going to pretend it doesn't exist until something forces me to dive into it (and hopefully that day never comes).

Fair enough :)

With all that in mind, I followed your advice and removed the WriteConsole calls entirely. :)

Fantastic, many thanks again for this! I'll review/merge asap (probably tomorrow).

@stenzek stenzek merged commit 6a04803 into stenzek:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants