Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
FFY00 merged 3 commits intopython:mainfrommhsmith:android-sys-platform
Mar 11, 2024

Conversation

mhsmith
Copy link
Member

@mhsmithmhsmith commentedMar 1, 2024
edited
Loading

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 ofhasattr(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 comparesys.platform to "linux", and added "android" where appropriate. These are mostly in the tests, but there are also a few in the stdlib.

Copy link
Member

@FFY00FFY00 left a 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.

cc@freakboy3742@misl6

erlend-aasland reacted with eyes emoji
@erlend-aasland

This comment was marked as outdated.

@mhsmith
Copy link
MemberAuthor

@freakboy3742 was actually the one who convinced me to go this way – his reasoning is on the PEP's Discourse threadhere.

FFY00 reacted with thumbs up emoji

@freakboy3742
Copy link
Contributor

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.

As I said in the discussion thread on PEP 738 - I'm ahard +1 onsys.platform == "android".

Yes, this is technically backwards incompatible with the current state of Android support in the CPython source tree. However, I'd counter that:

  1. CPython doesn't currently haveofficial support for Android. It has defacto support by way of "not breaking the build", one additional API entry point (sys.getandroidapilevel()), and a handful of other conditional branches in code; but it's not a Tier 3 platform. To the extent that the ecosystem supports Android currently, that support isvery ad hoc, and almost entirely performed by the people tagged in this thread (or at most one degree of separation from those people). There are no Android packages on PyPI.

  2. The current state of detecting Android is the source ofso many headaches in the rest of the ecosystem. Yes there is code that will "just run" because Android reports as Linux - but that code is almost always so trivial that it would likely also work on iOS - it's really testing for POSIX, not Linux. Any package that does anything even remotely complicated requires patching to run on Android - at the very least in the build system, and often in user-space code as well. The fact thatsys.platform reports as Linux means we need to use workaroundslike this one everywhere that we do platform detection, because we can't just use a dictionary lookup. The only other platforms that cause this much headache are the ones that include the OS version insys.platform - which is a pattern that (thankfully) appears to be slowly undergoing deprecation.

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.

willingc, erlend-aasland, misl6, and FFY00 reacted with thumbs up emoji

Copy link
Contributor

@erlend-aaslanderlend-aasland left a 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 😄

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@mhsmith
Copy link
MemberAuthor

@erlend-aasland: I've applied your suggestions, so I think this should be ready to merge now.

Copy link
Member

@FFY00FFY00 left a 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!

@bedevere-app
Copy link

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 phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mhsmith
Copy link
MemberAuthor

mhsmith commentedMar 5, 2024
edited
Loading

I have made the requested changes; please review again.

I've also cleaned up thesys.platform documentation as follows:

  • Removed the reference to "append platform-specific components tosys.path". This has not been true since Python 3.7 (https://bugs.python.org/issue28046).
  • Added iOS as specified in PEP 730.
  • Sorted the table in alphabetical order.
  • Moved the table to the top, since it now contains all supported platforms [EDIT – except FreeBSD].
  • Removed recommendation to usestartswith on Linux and AIX. The last Python version to require this for Linux was 3.2, and the last for AIX was 3.7, both of which are now obsolete.

@bedevere-app
Copy link

Thanks for making the requested changes!

@erlend-aasland,@FFY00: please review the changes made to this pull request.

Copy link
Member

@FFY00FFY00 left a 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!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@FFY00FFY00FFY00 approved these changes

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@mhsmith@erlend-aasland@freakboy3742@FFY00

[8]ページ先頭

©2009-2025 Movatter.jp