-
-
Notifications
You must be signed in to change notification settings - Fork 9.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
Use sys_platform instead of platform_system #4007
Conversation
This is more compatible as a marker and so will break fewer people.
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.
LGTM 🍰
I should note that this fix does not entirely remove the issue: sufficiently old pips will still fail to install Requests. We're just moving the goalposts on what counts as "sufficiently old". |
I'm in favor of breaking less people's builds as long as people using really old |
@Lukasa are you aware of what the specific pip version requirement would be if this got merged? |
Codecov Report
@@ Coverage Diff @@
## master #4007 +/- ##
=======================================
Coverage 89.74% 89.74%
=======================================
Files 15 15
Lines 1940 1940
=======================================
Hits 1741 1741
Misses 199 199 Continue to review full report at Codecov.
|
@thieman I'm not. I think it actually boils down to a setuptools requirement, but setuptools from as early as 2014 should handle this I think. |
My proposal is that we push a 2.14.1 with this fix, and then see whether things calm down on #4006. |
Just to let know I tried with pip 8.0.0 to 8.1.1, and it works. |
Due to an old pip version, we got bit by the problem fixed in https://github.com/kennethreitz/requests/pull/4007 This happened in CCI-MOC#767 (though unfortunately the evidence is gone since I restarted the build).
Still broken on Travis https://travis-ci.org/stripe/stripe-python/jobs/230553497#L498 |
@ofek thanks for pointing this out. It looks like (some or all of) the Travis-CI instances are running on |
This is more compatible as a marker and so will break fewer people. Potentially resolves #4006.