-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
gh-75729: Fix os.spawn tests not handling spaces on Windows #99150
Conversation
Should this have a NEWS entry? I see some PRs that only change the tests have them, but many (perhaps most?) I spot-checked don't. |
I think a NEWS entry is helpful here because this could affect redistributors or users who are trying to run the tests locally. |
a1c2023
to
2362e81
Compare
Thanks, I added one, but it seems there's something funny going on with GitHub, as when I pushed it failed multiple times with a strange error, even after I reset and re-committed:
I then force pushed and my fork updated, but nothing happened here for several minutes. Finally I rebased and force-pushed again, and it appears to have updated here, but the GHA webhooks still aren't firing yet. |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Does os.exec tests need a similar fix? |
Good question, and at least as far as the scope of this PR, the However, the |
Is this ready to merge? |
It should be as far as I'm aware, yeah—there are some other issues that need another round of discussion, but we can at least fix the tests for now. |
Thanks @CAM-Gerlach for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
GH-103366 is a backport of this pull request to the 3.11 branch. |
…thonGH-99150) * Quote paths in os.spawn tests on Windows so they work with spaces * Add NEWS entry for os spawn test fix * Fix code style to avoid double negative in os.spawn tests Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> --------- (cherry picked from commit a34c796) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
* Quote paths in os.spawn tests on Windows so they work with spaces * Add NEWS entry for os spawn test fix * Fix code style to avoid double negative in os.spawn tests Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> --------- (cherry picked from commit a34c796) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
…thon#99150) * Quote paths in os.spawn tests on Windows so they work with spaces * Add NEWS entry for os spawn test fix * Fix code style to avoid double negative in os.spawn tests Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com> --------- Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
As originally reported in #75729 and confirmed to still be reproducible on the latest Python 3.12, the various
os.spawn*
tests are broken on Windows if the current working directory contains any spaces in its path (e.g. due to spaces in the username), as on Windows the arguments are simply concatenated and not quoted. This PR fixes the immediate issue with the tests by quoting the arguments in question on Windows.