Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-71052: Change Android'ssys.platform
from "linux" to "android"#116215
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Hum, I think this is a backwards incompatible change, which could result in downstream code breaking. While I think we should probably have a better way to identify Android platforms, I don't know exactly how I feel about this approach. I think at least we should get the input from people shipping and packaging Python on Android.
This comment was marked as outdated.
This comment was marked as outdated.
@freakboy3742 was actually the one who convinced me to go this way – his reasoning is on the PEP's Discourse threadhere. |
As I said in the discussion thread on PEP 738 - I'm ahard +1 on Yes, this is technically backwards incompatible with the current state of Android support in the CPython source tree. However, I'd counter that:
Repeating whatI said in the discuss thread: Yes, technically, Android is a Linux. However, that is a distinction that isn't helpful in practice, because “Linux” isn’t just a kernel - it’s lots of other device expectations, filesystem expectations, userspace expectations, and more. The most practical manifestation of this: If Android is indeed "a Linux", why are any patches needed to CPythonat all? Shouldn't Android "just work" as a Linux, or fall out of trivial configuration checks? Unless I'm mistaken, that there are less conditional branches in the test suite forFreeBSD support than are required for Android. If that isn't an indication that Android isn't "a Linux" in practice, I don't know what is. |
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.
Thanks for chiming in@freakboy3742.
Looks good to me. I had to look twice at thetest_sysconfig
change, though 😄
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Build/2024-03-01-16-44-19.gh-issue-71052.Hs-9EP.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@erlend-aasland: I've applied your suggestions, so I think this should be ready to merge now. |
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.
The PR looks good to me, the only thing missing is updating the documentation.
We should add Android to the "For other systems (...)" list in thesys.platform
documentation (Doc/library/sys.rst
), and add aversionchanged
field noting this change.
After that, I think we are good to merge, thanks@mhsmith!
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
mhsmith commentedMar 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I have made the requested changes; please review again. I've also cleaned up the
|
Thanks for making the requested changes! @erlend-aasland,@FFY00: please review the changes made to this pull request. |
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.
@mhsmith, sorry for the delay, the documentation changes look good to me, thank you!
Uh oh!
There was an error while loading.Please reload this page.
As discussed inPEP 738, although Android is based on Linux, it differs in enough significant ways that a separate name is justified.
Related changes:
I've replaced all instances of
hasattr(sys, "getandroidapilevel")
with asys.platform
check. This is generally easier to read, and more consistent with how the other platforms are handled.I've checked all the places I could find that compare
sys.platform
to "linux", and added "android" where appropriate. These are mostly in the tests, but there are also a few in the stdlib.