Detect if stdout is a console in quickjs-libc #642
Conversation
| putchar(' '); | ||
| str = JS_ToCStringLen(ctx, &len, argv[i]); | ||
| if (!str) | ||
| return JS_EXCEPTION; |
There was a problem hiding this comment.
Functional change which IMO is an improvement: exceptions no longer cause truncated output
| JS_FreeCString(ctx, str); | ||
| s = JS_ToCString(ctx, argv[i]); | ||
| if (s) { | ||
| dbuf_printf(&b, "%s%s", &" "[!i], s); |
There was a problem hiding this comment.
My brain is not parsing those arguments, what's the sorcery here? 😅
There was a problem hiding this comment.
It prints a space only when i != 0. When i == 0, it passes a pointer to the nul byte because !0 == 1 :)
quickjs-libc.c
Outdated
| #ifdef _WIN32 | ||
| handle = (HANDLE)_get_osfhandle(/*STDOUT_FILENO*/1); // don't CloseHandle | ||
| if (GetFileType(handle) != FILE_TYPE_CHAR) | ||
| goto fallback; | ||
| if (!GetConsoleMode(handle, &mode)) | ||
| goto fallback; | ||
| handle = GetStdHandle(STD_OUTPUT_HANDLE); | ||
| if (handle == INVALID_HANDLE_VALUE) | ||
| goto fallback; | ||
| mode = GetConsoleOutputCP(); | ||
| SetConsoleOutputCP(CP_UTF8); | ||
| WriteConsoleA(handle, b.buf, b.size, NULL, NULL); | ||
| SetConsoleOutputCP(mode); | ||
| FlushFileBuffers(handle); | ||
| goto done; | ||
| fallback: | ||
| #endif |
There was a problem hiding this comment.
For casual readers of this code that are unfamiliar with legacy system intricacies, could you add a comment explaining why we need to special case console output on Windows?
I wish we used a more general approach such as detecting the condition once at startup and setting up the system handle 1 or the stdout stream to have the expected behavior for all output calls. Hacking this behavior in js_print seems incorrect, we should have a lower level API that wraps all output to stdout.
There was a problem hiding this comment.
I'll add that comment.
detecting the condition once at startup
That's worse because it doesn't handle runtime changes (and from 15 years of node and libuv bug reports I can tell you people do do stuff like reopening stdout at runtime)
There was a problem hiding this comment.
OK, another potential issue: shouldn't you move the call to FlushFileBuffers(handle) before restoring the console mode with SetConsoleOutputCP(mode)? flushing the buffers before the mode change might also be necessary.
This hack really stinks: what if QuickJS is run in a different shell (like cygwin of the old days) or via a remote connection to a Windows host?
There was a problem hiding this comment.
shouldn't you move the call to FlushFileBuffers(handle) before restoring the console mode with SetConsoleOutputCP(mode)?
I don't think it matters but I wouldn't mind being proven wrong.
what if QuickJS is run in a different shell (like cygwin of the old days) or via a remote connection to a Windows host?
We'll just wait for the bug reports to come in :-)
Use regular libc stdio (fwrite) when stdout is redirected, don't call WriteConsoleA because that circumvents the redirection. Fixes: quickjs-ng#635
Use regular libc stdio (fwrite) when stdout is redirected, don't call WriteConsoleA because that circumvents the redirection.
Fixes: #635