-
Notifications
You must be signed in to change notification settings - Fork 3k
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
pip completely broken on Windows #3941
Comments
This is completely intentional wrt |
Sorry if I misunderstand, but this is not about distro refusing to work on non-Linux platforms. distro provides information about Linux environments and clearly has no reason, or theoretical ability, to work anywhere else. This is about the fact that pip has made itself utterly dependent on distro, and hence is now broken on all platforms where distro does not work, i.e. anything but Linux. |
Yes, my point is that |
The distro module only works on Linux. Add a condition to only import it there. The condition is the same as that around the only use in user_agent() below. Fixes pypa#3941.
Thanks @chrullrich - duelling PRs :-) I'm not sure my PR is any better than yours, but as you've closed yours I'll merge #3947 once the tests pass. Thanks for the report, and apologies for not getting to it sooner. |
Yours is better; I hadn't thought of importing it where it is used ... |
BTW, @dstufft why aren't the appveyor tests required to pass for new PRs? Is there a setting somewhere that needs to be changed to run them and require them to work? I thought @xavfernandez had done a bunch of work to get the tests working on Windows, but it looks like they've bitrotted. Even if we only had a minimal set of Windows tests, they would have caught this issue, so I'd rather mark a bunch of tests as "skip on Windows" for now, than have no tests at all. If there's a reason for not enabling appveyor, then that's fine, otherwise I'll open an issue for this. |
Are you asking why we don't run Appveyor at all or why are they not required before a PR can merge? AFAICT they aren't running at all though I don't think there is any specific reason for that except maybe I didn't know how to Windows good enough. If they're running and you just want them toggled to required that should be easy too. |
@dstufft I thought it was enabled, but not marked as "required for a merge". But it's possible I'm wrong - I don't know how to navigate Travis' CI config (for instance, where do I find the CI runs for the current head of the master branch? I wasn't even able to find that to confirm Appveyor was running) Anyhow, if you're not aware of a specific reason it's not running, I'll do some digging and enable it. Might need some help from you or whoever knows the pypa account password when it comes to setting up an appveyor account, if it turns out we don't even have that... |
|
Ah, OK, that's why I couldn't find them, I was expecting a link in github somewhere (like we have for PRs). Thanks. |
Fixed via #3947 |
Description:
Since fc4109e (PR #3906), pip does not work anymore on Windows. Other (non-Linux) platforms, too, possibly.
(I first reported this as a comment on bug #3823, but that is closed and so probably nobody noticed.)
What I've run:
platform.linux_distribution()
did not attempt to run an external command to get LSB release information; the new code does, and then stumbles when the attempt to run a non-existing command fails with exit code 1 rather than 127.If an
lsb_release
command returning the correct exit status (127) is provided to simulate the expected failure mode, the next failure is this:Note that the full distro package (as opposed to the version integrated into pip) defends itself aggressively against failing in this manner:
The text was updated successfully, but these errors were encountered: