Reimplement Unix.isatty on Windows#1321
Conversation
The null device is a character device, but is not a console - Unix.isatty would incorrect return true for standard handles redirected to null. The new implementation returns true if and only if the fd points to a console handle.
otherlibs/win32unix/isatty.c
Outdated
| CAMLprim value unix_isatty(value fd) | ||
| { | ||
| DWORD lpMode; | ||
| return (Val_bool(GetConsoleMode(Handle_val(fd), &lpMode))); |
There was a problem hiding this comment.
This relies on the fact that GetConsoleMode fails (i.e. returns 0) iff the handle is neither for an input or output buffer of a console. Are you confident that this always works? I cannot find on MSDN a description of error cases. (I could imagine e.g. that the function doesn't fail on some older versions of Windows even on non-console handles.)
There was a problem hiding this comment.
It's not explicit, I agree, but even for MSDN it would be very strange for a function to be documented as requiring a console handle and not to return ERROR_INVALID_HANDLE if it receives a handle to something else.
There was a problem hiding this comment.
In these useful cuddly open-source Microsoft times, it turns out we can often check these things: https://github.com/dotnet/corefx/blob/master/src/System.Console/src/System/ConsolePal.Windows.cs#L164
There was a problem hiding this comment.
That said, it's interesting that .NET first of all checks that the handle represents a character device.
There was a problem hiding this comment.
This was introduced in dotnet/corefx@8cd8e78. Should we search advice from the author of this commit?
There was a problem hiding this comment.
You can't go wrong by copying the logic of the .NET code...
There was a problem hiding this comment.
Actually, I think we can go wrong - I'm going to risk asserting that the .NET code is actually incorrect. It was added in dotnet/corefx@8cd8e78#diff-6f40ebab17283a57f687629b80f8beb7 and it treats the output of GetFileType as if it were a bitmask, which it is not. So the code they have will unnecessarily call GetConsoleMode for pipes as well.
I have pushed a commit which includes what I believe to a correct version of the test on GetFileType...
| Unix.isatty Unix.stderr = %b" | ||
| (Unix.isatty Unix.stdin) | ||
| (Unix.isatty Unix.stdout) | ||
| (Unix.isatty Unix.stderr) |
There was a problem hiding this comment.
I would feel better if at least one test of Unix.isatty would return true. For example, what about opening /dev/tty or CON (whichever succeeds) and test that?
| @@ -0,0 +1,3 @@ | |||
| Unix.isatty Unix.stdin = false | |||
| Unix.isatty Unix.stdout = false | |||
| Unix.isatty Unix.stderr = false No newline at end of file | |||
There was a problem hiding this comment.
Please terminate this last line (and the corresponding printf) with a newline, it feels lonely without one.
|
The freeze for 4.06 is tomorrow Friday Sept 15th. This PR can go in provided the changes suggested by @alainfrisch and I are made by this time. |
145c6fd to
c09c200
Compare
|
I believe all the comments have been addressed, so merging. |
This reverts commit 6fb2960. This is to be able to test ocamltest without this commit that breaks CI. The branch should not be merged into trunk as long as this commit is here.
This reverts commit 6fb2960. This is to be able to test ocamltest without this commit that breaks CI. The branch should not be merged into trunk as long as this commit is here.
This reverts commit 6fb2960. This is to be able to test ocamltest without this commit that breaks CI. The branch should not be merged into trunk as long as this commit is here.
This reverts commit 6fb2960. This is to be able to test ocamltest without this commit that breaks CI. The branch should not be merged into trunk as long as this commit is here.
This reverts commit 6fb2960. This is to be able to test ocamltest without this commit that breaks CI. The branch should not be merged into trunk as long as this commit is here.
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Co-authored-by: Sonja Heinze <sonjaleaheinze@gmail.com>
Presently:
With this version, it returns
false(as it should).