Redirect using C output instead of C++ output #8391
Conversation
src/log.cpp
Outdated
| // redirect to a file | ||
| // IMPORTANT: apparently redirecting stderr/stdout will also redirect std::cerr/std::cout, but the reverse is not true | ||
| // redirecting std::cerr/std::cout will *not* redirect stderr/stdout | ||
| if(setvbuf(stdout, NULL, _IONBF, 2) == -1) { |
There was a problem hiding this comment.
Why do you want to set stdout to be unbuffered?
There was a problem hiding this comment.
Otherwise the stdout/std::cout output doesn't make it to the log file, at least for the example in wesnoth.cpp where it almost immediately terminates with an assert. stderr/std::cerr is unbuffered by default.
There was a problem hiding this comment.
Hmm… std::cout and/or the C FILE API might have a software buffer on top of that, so maybe it's fine…
| // IMPORTANT: apparently redirecting stderr/stdout will also redirect std::cerr/std::cout, but the reverse is not true | ||
| // redirecting std::cerr/std::cout will *not* redirect stderr/stdout |
There was a problem hiding this comment.
I guess this is technically true, but… the way it's stated is maybe a tad misleading.
Redirecting std::cerr/std::cout is in the iostream, replacing their stream buffer with something else. Redirecting stderr/stdout is done at a lower level, essentially making both streams be the same stream.
I'm not sure what's a better way to put this…
Looks like this is still being worked on? |
|
It should at least build now. The unit tests won't run since the assert is there in wesnoth.cpp, but that's fine for now. |
Wedge009
added
the
Engine
|
I'm not sure what I'm supposed to test. I haven't tried non-ASCII path because I get the assertion failure in Win10 with VS 2022. On Win7 with VS 2019 I don't even get that far - nothing opens, nothing displays. |
|
The test is about whether the logfile is createsd and contains all messages incluusing the assertion error message. |
|
@Wedge009 the assertion failure is intended - part of the test is whether the text of the assertion is correctly added to the log file. |
|
Sorry I missed that detail. Without even considering the complication of non-ASCII log paths, I don't seem to get anything recorded in the log file. Just an empty log file is created and that's all. Your code is tested as-is in Win10 with VS2022 compilation, I haven't made any modifications. |
|
Weird. How are you running it? |
|
Same as usual, from the VS IDE. Same result when running from the command-line. The only argument used is specifying the |
|
Is there anything printed on the command line? |
|
No, because everything should be redirected to the log by default. If I set |
|
If you comment out lines 198-228, and then uncomment lines 230-240, does it work? |
|
0, 1, and 3 are printed in the log file, followed by the normal log. Assertion is not printed - again, it's shown as a dialogue. I didn't try moving my user directory to a path with non-ASCII characters. Is there an easy way to tell Wesnoth to use a specific directory for the log file? |
|
It sounds to me that the assertion not being printed is intentional – Windows is trying to be smart and show it to the user in a more direct way. |
|
Looks like it's just how the VC++ library wants to implement it: |
|
The assert in wesnoth.cpp on line 1131 can just be removed then - no point in testing it on Windows. The logs directory is just the "logs" folder inside the userdata folder, so setting a custom userdata folder will move the logs too. Out of curiosity, are you able to build 1.16 as well? I'm wondering if the stdout redirecting ever worked, since the lines 230-240 should be essentially what the Windows logging to file was doing for 1.16. |
|
Looks like Windows making the distinction between console vs gui applications is being a pain again - Windows GUI apps make |
|
I pulled your changes, reverting to a clean build, no modifications. No more assertion dialogue. Still zero-byte log file generated. Running with I'm afraid I'm not able to keep testing ad-hoc changes but I'll try to help with testing as I'm able. |
|
Pushed another attempt - if the previous code didn't work then I'm guessing this won't either, probably for the same reason. But maybe we'll get lucky and it's at least a lot simpler. |
|
Well... using your branch unmodified, I get 2 and 4 printed in the log file... and nothing else. |
|
Well, that's... different, at least. Pushed another attempt - ideally you should see 2 and 4 in the ".out.log" file and everything else in the regular ".log" file. |
|
Yes, 2 and 4 are in .out.log. 0, 1, 3, followed by normal log text are in .log. |
|
Alright, that's what I'm going to go with then, unless someone knows a way to get |
|
There weren't any assertion messages after your changes. No dialogue, no printing in the log. I assume that's okay/expected? I haven't tried this at all on Linux. |
|
Oh, I just remembered your request to test non-ASCII characters. I ran |
|
I repeated the |
|
I can reproduce the issue on Linux as well - the build info reports the logs directory as it should (aka it uses the
Yeah, I removed it since it wasn't relevant to testing on Windows. |
I checked that as well - it's reporting the logs directory corresponding with the
Oh, yeah, so that's what happened. I thought it was just not writing anything at all. |
|
Looks like the issue is that the log file setup happens before the command line options are parsed. Not sure if it makes sense to move that earlier or not though. |
|
1.16 writes the log file in the correct location, though? Is the current set-up for 1.17 that much different? (I know you did a lot of work on the log redirection previously.) |
|
For 1.16 it would initially write the log file to the Windows temp directory, then move it to the logs directory after the userdata folder is setup, wherever that ends up being. For 1.17 it just writes the log file directly to the logs directory. |
|
Alright, the log file not using a userdata folder provided via the command line option should be fixed. |
|
I hate that there's a separate .out.log file… |
|
Yeah, it's dumb. Unfortunately, Windows makes it the only option I know of to work correctly. |
|
If it's only needed on Windows, why do the other OSes need to suffer too? (Unless I misread and it's only being done on Windows.) |
|
It's only being done for Windows. Other OSes will just use |
|
Specification of |
|
Nice, glad that worked on the first attempt. Thanks for testing all this out! |
Okay… still bad, but less bad at least. I guess we can leave that as something to work out at some unspecified time in the future… |
Apparently redirecting stdout/stderr also results in std::cout/std::cerr being redirected, but not the reverse. This is not compatible with using boost's tee. Fixes wesnoth#8108 Fixes wesnoth#8255
|
I'm wondering about the choice of name for .out.log. What actually goes in there, anyway? |
|
Any output to stdout/std::cout would end up in the .out.log file. Nothing enabled by default in Wesnoth writes to either of those, though there's some stuff that's commented/ |
|
Tested on Win7 - logs working, redirecting to directory with non-ASCII characters also works. As a bonus, presumably because boost tee is no longer used, #8024 is fixed by this too. |
|
Nice - I'll merge this tomorrow if nothing else comes up then. |
Apparently redirecting stdout/stderr also results in std::cout/std::cerr being redirected, but not the reverse. This is not compatible with using boost's tee.
Fixes #8108
Fixes #8255
The uncommented code in log.cpp results in the following being logged to file:
The commented code in log.cpp results in the following being logged to file (stdout/std::cout is missing):
The commented code is based on the 1.16 code from log_windows, so I'm not sure why stdout doesn't get redirected correctly (it's neither in the log file nor present on the terminal - I've no idea where it's getting sent to):
wesnoth/src/log_windows.cpp
Line 382 in 165d2ba
@Wedge009 would you be able to test if this works the same on Windows 10 1903 or later where the path to the log file contains non-ascii characters?