fix: allocate console and set stderr to 2 for subsequent logging

[pzhlkj6612]

For this issue:

• Logging to STDERR by libfmt makes the game crash #14 (comment)

Please test!

Refs:

• Windows: stdout/stderr file descriptors invalid with GUI application ocaml/ocaml#9252 (comment)
• AllocConsole: backspace and newline characters incorrectly printed after "system" call https://web.archive.org/web/20230601042758/https://social.msdn.microsoft.com/Forums/windows/en-US/286059e8-9163-4679-9663-efc63c5d9c53/allocconsole-backspace-and-newline-characters-incorrectly-printed-after-quotsystemquot-call?forum=vcgeneral#68a73761-7a40-4d4c-a15e-90902a1244ee-isAnswer

.

[pzhlkj6612]

Putting things in header file is painful, so the current code looks that awkward.

[pzhlkj6612]

Well, I may need to create a "logging.cpp"... Are our executables able to use that?

[shmocz]

Thanks!

The problem has been mitigated in 4141a82. Technically it doesn't fix writing to stderr, but just enforces that everything's logged to a file instead of stderr.

At the moment, I'm not sure if the benefits from this fix outweigh the added complexity:

1. This makes logging functions dependent on Windows API, but the issue concerns just in-game logging (stderr works just fine in tests).
2. If logging to file is enabled, what should the behavior be like? Open the console AND write to file? Not open the console at all, which is essentially the current behaviour?
3. What if another DLL (e.g. yrpp-spawner, Phobos) allocates a console? Is the behaviour well defined?

[pzhlkj6612]

Thank you for the detailed reply. Let me explain:

1. This makes logging functions dependent on Windows API, but the issue concerns just in-game logging (stderr works just fine in tests).

Hmm, we are trying to use CUI in a GUI program (i.e., the game). that's normal and natural on Linux, but a bit inconvenient on Windows:

• How can I output to the parent console window from a Win32 C++ application? - Stack Overflow https://stackoverflow.com/q/52825867
• Problem using AllocConsole() and marshalling - Stack Overflow https://stackoverflow.com/q/4349187
  • Adding Console I/O to a Win32 GUI App https://web.archive.org/web/20070513064449/http://www.halcyon.com/%7East/dload/guicon.htm

I'm sorry I haven't run any tests until now. I guess those tests are able to run without a game. Hence, I need to make things more clear, that is:

• Writing logs to stderr on any platforms.
• Calling Windows API to set stderr to 2 on Windows in the first place.

2. If logging to file is enabled, what should the behavior be like? Open the console AND write to file? Not open the console at all, which is essentially the current behaviour?

We can define the behaviour. I think we can support both two log sinks. For example:

• logFilename: filenane, writing logs to a file.
• consoleLog: boolean, writing logs to the console.

3. What if another DLL (e.g. yrpp-spawner, Phobos) allocates a console? Is the behaviour well defined?

It should be fine if their implementation is the same as "CnCNet/yr-patches". ¹ Subsequent AllocConsole() calls in other programs will fail, but it does not mean that the terminal is broken, but means that the current process has already allocated a console. In our use cases, we've taken that job.

Unfortunately, Phobos' implementation may need to be modified before using my new code if they allocate console after us. ² I haven't digged into Phobos, though.

At the moment, I'm not sure if the benefits from this fix outweigh the added complexity:

...

Let me refactor my new code based on the latest codebase. I need to redesign it.

It deserves a fix.

Furthermore, recently I also did something about AllocConsole() in "CnCNet/yr-patches" to make console happy. ³

Footnotes

1. https://github.com/CnCNet/yr-patches/blob/586a84b284c493930f930030f8923aace01d6d49/src/yr_util.c#L25 ↩

2. https://github.com/Phobos-developers/Phobos/blob/bed263a56463ff829fc47d3202c66212d3a72398/src/Utilities/Debug.cpp#L124 ↩

3. Support writing logs to Git-Bash mintty terminal; reduce the number of "GetStdHandle()" calls https://github.com/CnCNet/yr-patches/pull/35 ↩