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

std: /dev/ptmx in MSYS2 is not recognized as a tty in std::io::IsTerminal #119658

Closed
petrochenkov opened this issue Jan 6, 2024 · 16 comments · Fixed by #119664
Closed

std: /dev/ptmx in MSYS2 is not recognized as a tty in std::io::IsTerminal #119658

petrochenkov opened this issue Jan 6, 2024 · 16 comments · Fixed by #119664
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

How to reproduce:

./x t tests/run-make --test-args emit-to-stdout

That test (tests\run-make\emit-to-stdout\Makefile) checks that rustc's behavior with options like --emit obj is different depending on whether stdout is tty or not.
That test uses /dev/ptmx as stdout and fails when run under MSYS2.

cc @ChrisDenton

@petrochenkov petrochenkov added the C-bug Category: This is a bug. label Jan 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 6, 2024
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 6, 2024

we@WIN-GQ6AS7OLS34 MINGW64 ~
$ rustc --emit obj=- hello.rs 1>/dev/ptmx

we@WIN-GQ6AS7OLS34 MINGW64 ~
$ rustc --emit obj=- hello.rs
error: option `-o` or `--emit` is used to write binary output type `obj` to stdout, but stdout is a tty

error: aborting due to 1 previous error

Cannot check the behavior on Linux right now, but if nobody complains about failing run-make tests, then /dev/ptmx is probably considered a tty there.
UPD: Yep, /dev/ptmx is a tty on Linux and rustc refuses to --emit obj into it.

@ChrisDenton
Copy link
Member

Does it work with MSYS=enable_pcon set in msys2.ini?

For msys2 without pseudoconsole support there's no way to really know if something is a tty or not so we use heuristics based on the pipe name, these may need updated. If msys2 is using the Windows pseudoconsole then it should just work as it should be detected as a normal Windows console handle (unless /dev/ptmx is special in some other way).

@petrochenkov
Copy link
Contributor Author

Does it work with MSYS=enable_pcon set in msys2.ini?

No, it doesn't help.

For msys2 without pseudoconsole support there's no way to really know if something is a tty or not so we use heuristics based on the pipe name, these may need updated

I did assume it was some missing heuristic in fn msys_tty_on.

@ChrisDenton
Copy link
Member

Ah, actually the problem is this check:

// At this point, we *could* have a false negative. We can determine that this is a true
// negative if we can detect the presence of a console on any of the standard I/O streams. If
// another stream has a console, then we know we're in a Windows console and can therefore
// trust the negative.
for std_handle in [c::STD_INPUT_HANDLE, c::STD_OUTPUT_HANDLE, c::STD_ERROR_HANDLE] {
let std_handle = c::GetStdHandle(std_handle);
if !std_handle.is_null()
&& std_handle != handle
&& c::GetConsoleMode(std_handle, &mut out) != 0
{
return false;
}
}

The trouble is even if enable_pcon is set (which it turns out it is by default now) /dev/ptmx is still a pipe instead of a pseduoconsole handle. So unless all std handles are redirected, it will return negative.

@ChrisDenton
Copy link
Member

I created a fix in #119664 by simply removing the above code. This decreases the chances of a false negative at the cost of increasing the chances of a false positive. But I think simply inspecting the pipe name is overall the most reliable way to detect an msys2 tty.

@saethlin saethlin added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 6, 2024
@lygstate
Copy link
Contributor

Why /dev/ptmx is a pipe, can that be checked by environment variable?

@ChrisDenton
Copy link
Member

You'd have to ask msys2 why that is. Maybe it's a bug or maybe there's some reason for it. I don't know.

can that be checked by environment variable

I don't think so.

@petrochenkov
Copy link
Contributor Author

You'd have to ask msys2 why that is. Maybe it's a bug or maybe there's some reason for it. I don't know.

@mati865, maybe you know?

@mati865
Copy link
Contributor

mati865 commented Jan 22, 2024

Unfortunately I don't know much about Cygwin's internals.

@Biswa96
Copy link

Biswa96 commented Jan 22, 2024

Unfortunately I am not familiar with Rust language. I assumed the rust code looks like following C code.

#include <stdio.h>
#include <windows.h>

int main(void)
{
    DWORD mode;
    printf("%d\n", GetConsoleMode(GetStdHandle(STD_INPUT_HANDLE), &mode));
    printf("%d\n", GetConsoleMode(GetStdHandle(STD_OUTPUT_HANDLE), &mode));
    printf("%d\n", GetConsoleMode(GetStdHandle(STD_ERROR_HANDLE), &mode));
}

That program prints 1 (TRUE) for all handles in msys2 environment with conpty enabled.
With MSYS=disable_pcon environment variable, that program prints 0 (FALSE) for all handles.

@ChrisDenton
Copy link
Member

@Biswa96 if that program is called isconsole then doing the following:

./isconsole 2>/dev/ptmx

prints:

1
1
0

@Biswa96
Copy link

Biswa96 commented Jan 22, 2024

@tyan0 may provide some information about that.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
Fix tty detection for msys2's `/dev/ptmx`

Our "true negative" detection assumes that if at least one std handle is a Windows console then no other handle will be a msys2 tty pipe. This turns out to be a faulty assumption in the case of redirection to  `/dev/ptmx` in an msys2 shell. Maybe this is an msys2 bug but in any case we should try to make it work.

An alternative to this would be to replace the "true negative" detection with an attempt to detect if we're in an msys environment (e.g. by sniffing environment variables) but that seems like it'd be flaky too.

Fixes rust-lang#119658
@tyan0
Copy link

tyan0 commented Jan 22, 2024

/dev/ptmx is a device for native msys2 (cygwin) programs. I guess you are using native windows rust compiler. In that case, the program built is also native windows program.

/dev/ptmx is a tty for native msys2 program, however it is just a pipe for native windows program. So, the result is as the author reported.

You can confirm the behaviour with the code:

#include <stdio.h>
#include <unistd.h>
int main() {
    printf("%d\n", isatty(0));
    printf("%d\n", isatty(1));
    printf("%d\n", isatty(2));
    return 0;
}

If you compile this using /usr/bin/gcc and run ./a.exe 2>/dev/ptmx, the result is:

1
1
1

however, with /mingw64/bin/gcc, the result is:

64
64
0

I do not think there is much meaning to check behaviour with /dev/ptmx for native windows program, because it does not provide any meaningful functions for native windows program.

@bors bors closed this as completed in 67d0936 Jan 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 23, 2024
Rollup merge of rust-lang#119664 - ChrisDenton:mingw-pty, r=thomcc

Fix tty detection for msys2's `/dev/ptmx`

Our "true negative" detection assumes that if at least one std handle is a Windows console then no other handle will be a msys2 tty pipe. This turns out to be a faulty assumption in the case of redirection to  `/dev/ptmx` in an msys2 shell. Maybe this is an msys2 bug but in any case we should try to make it work.

An alternative to this would be to replace the "true negative" detection with an attempt to detect if we're in an msys environment (e.g. by sniffing environment variables) but that seems like it'd be flaky too.

Fixes rust-lang#119658
@Biswa96
Copy link

Biswa96 commented Jan 23, 2024

I am bit curious why a test for native Windows platform runs a command like program > /dev/ptmx. Should not that test be excluded in Windows?

@ChrisDenton
Copy link
Member

Test file is here: https://github.com/rust-lang/rust/blob/master/tests/run-make/emit-to-stdout/Makefile

On Windows our "run-make" tests use msys2 (or git bash, but same difference) so the Makefile is cross-platform, This particular test is testing that emitting binary data to a tty correctly fails. This is definitely something that should be tested on Windows. I'm not totally sure why /dev/ptmx is used for this but I'd assume it's so that the tty is independent of the one we're using for normal output (otherwise it would be annoying if the test fails and binary data is actually written to the tty).

See also #111626 which introduced the test. cc @pjhades

@pjhades
Copy link
Contributor

pjhades commented Jan 23, 2024

This particular test is testing that emitting binary data to a tty correctly fails.

Yes, that's the purpose of adding such (failure) tests.

I am bit curious why a test for native Windows platform runs a command like program > /dev/ptmx. Should not that test be excluded in Windows?

I'm not totally sure why /dev/ptmx is used for this but I'd assume it's so that the tty is independent of the one we're using for normal output (otherwise it would be annoying if the test fails and binary data is actually written to the tty).

If I remember correctly, in the test I initially tried

obj: $(OUT)
	$(RUSTC) --emit obj=- $(SRC) 2>$(OUT)/$@ || true
	diff $(OUT)/$@ emit-obj.stderr

but found that the tests would then fail, because stdout was already redirected in the test environment, so the expected error didn't happen. I used /dev/ptmx because that was the one that worked on all the platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants