-
Notifications
You must be signed in to change notification settings - Fork 762
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
Fix windows py spurious stderr failure #1885
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
(Would be good to get eyes on this from whoever wrote it, to know why it was here in the first place.) |
We had similar logic like this in the LSP? I feel like it's bad form to use content in stderr as a bad exit. I agree it'd be nice to get another review. |
Agree. |
@@ -344,8 +344,8 @@ mod windows { | |||
.in_scope(|| Command::new("py").arg("--list-paths").output()) | |||
.map_err(Error::PyList)?; | |||
|
|||
// There shouldn't be any output on stderr. | |||
if !output.status.success() || !output.stderr.is_empty() { | |||
// `py` sometimes prints "Installed Pythons found by py Launcher for Windows" to stderr which we ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's funny that it only does this sometimes. I tried to reproduce this on my machine with the official py
launcher and never managed to get it write to stderr.
I don't mind the change but we may want to log the stderr
output with tracing::debug!()
if it's non empty. It might be helpful when debugging issues related to py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just defensive programming |
Summary
On Windows
10.0.19045
thepy
command prints tostderr
even when working correctly. This means that uv should not treat this as a failure.Fixes #1904
Test Plan
I ran the modified code and it worked. I expect the pull request to run automated tests.