Skip to content
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

Add sys.getandroidapilevel() #72926

Closed
vstinner opened this issue Nov 18, 2016 · 24 comments
Closed

Add sys.getandroidapilevel() #72926

vstinner opened this issue Nov 18, 2016 · 24 comments
Labels
3.7 interpreter-core type-feature

Comments

@vstinner
Copy link
Member

@vstinner vstinner commented Nov 18, 2016

BPO 28740
Nosy @malemburg, @vstinner, @xdegaye, @yan12125
PRs
  • #552
  • Files
  • getandroidapilevel.patch
  • sys_getandroidversion.patch
  • getandroidapilevel-3.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2016-12-02.07:46:03.152>
    created_at = <Date 2016-11-18.21:26:35.663>
    labels = ['interpreter-core', 'type-feature', '3.7']
    title = 'Add sys.getandroidapilevel()'
    updated_at = <Date 2017-03-31.16:36:10.815>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:10.815>
    actor = 'dstufft'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-12-02.07:46:03.152>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2016-11-18.21:26:35.663>
    creator = 'vstinner'
    dependencies = []
    files = ['45539', '45545', '45718']
    hgrepos = []
    issue_num = 28740
    keywords = ['patch']
    message_count = 24.0
    messages = ['281169', '281171', '281207', '281210', '281211', '281213', '281214', '281216', '281218', '281223', '281236', '281253', '281375', '282142', '282143', '282150', '282189', '282193', '282201', '282203', '282210', '282211', '282226', '282227']
    nosy_count = 5.0
    nosy_names = ['lemburg', 'vstinner', 'xdegaye', 'python-dev', 'yan12125']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue28740'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 18, 2016

    Android slowly becomes a first-citizen class platform in CPython thanks to Xavier de Gaye and other motivated developers, thanks to all of them :-)

    To fix the issue bpo-28596, we need a function to check if we are running Android. Chi Hsuan Yen proposed to use sysconfig to get the new ANDROID_API_LEVEL configuration option:

    + if sysconfig.get_config_var('ANDROID_API_LEVEL'):

    But I asked to avoid sysconfig to reduce imports at Python startup (especially when the site module is not loaded). I proposed to add a new function to the sys module: sys.getandroidapilevel().

    sys.getandroidapilevel() would only be available on Android, as sys.getwindowsversion() is only available on Windows, and would return ANDROID_API_LEVEL as an integer.

    I'm not sure about the type: should we use a string? A tuple of integers like sys.version_info?

    sys.getwindowsversion() returns a named tuple:
    https://docs.python.org/dev/library/sys.html#sys.getwindowsversion

    I'm sorry, I don't have access to an Android development platform, so I let someone else implement it :-)

    @vstinner vstinner added 3.7 type-feature labels Nov 18, 2016
    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 18, 2016

    Ah, I wrote a patch to implement the function. I used ANDROID_API_LEVEL from pyconfig.h. I chose to simply return an integer. I don't know if it's the most future-proof API :-)

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Nov 19, 2016

    I proposed to add a new function to the sys module: sys.getandroidapilevel()

    This would also help fixing the long standing bpo-16255, and possibly also bpo-16353.

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Nov 19, 2016

    Patch tested with success on the android-24 emulator.

    @yan12125
    Copy link
    Mannequin

    @yan12125 yan12125 mannequin commented Nov 19, 2016

    Maybe time to re-implement android_ver() in bpo-26855 in C.

    @yan12125
    Copy link
    Mannequin

    @yan12125 yan12125 mannequin commented Nov 19, 2016

    I translate the Python version at bpo-26855 to C. Quite new to the C API, hope I'm doing it right :)

    Codes only. Docs and tests later

    sys.getwindowsversion() uses named tuples. Is there a similar need for Android or just plain tuples are fine?

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 19, 2016

    I didn't know that there are build and runtime versions. If the runtime
    version is available, it should be preferred.

    Hum, maybe its better to expose the __system_property_get() function in the
    os module, something similar to os.confstr()?
    https://docs.python.org/dev/library/os.html#os.confstr

    If the OS returns a string, I suggest to return a string in Python too. The
    caller would be responsible to convert it to integer. What do you think of
    that?

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Nov 19, 2016

    __system_property_get() is not a public API, see Android bug report [1].

    In this stackoverflow question, an Android developer confirms this point and suggests to use the file /system/build.prop instead, noting however that "build.prop isn't guaranteed to be stable (or even present)".

    This interesting document [3] describes the Android properties management design.

    I think we must use the reliable build time Android API level and implement sys.getandroidapilevel() (Victor patch) for knowing, in the standard library, whether we are running on an Android platform, and provide a best effort platform.android_ver() for the Android run time version. Things are changing very fast with the Android project and the OEM may add their own changes too.

    [1] https://code.google.com/p/android/issues/detail?id=143627
    [2] http://stackoverflow.com/questions/28413530/api-to-get-android-system-properties-is-removed-in-arm64-platforms
    [3] http://rxwen.blogspot.fr/2010/01/android-property-system.html

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Nov 19, 2016

    The build time Android API level is also a valuable information to Python users (application developers), it may be used at installation time or to detect a version mismatch.

    @yan12125
    Copy link
    Mannequin

    @yan12125 yan12125 mannequin commented Nov 19, 2016

    Both sys.androidapilevel() and platform.android_ver() return information about Android's API level. I guess that would be confusing so I hope to get them unified.

    Techinically it won't be a problem as Chromium is still using it (in a even dirty way) and the plan to remove it is stalled. [1]

    [1] https://bugs.chromium.org/p/chromium/issues/detail?id=392191

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 19, 2016

    I think we must use the reliable build time Android API level and
    implement sys.getandroidapilevel() (Victor patch) for knowing, in the
    standard library, whether we are running on an Android platform, and
    provide a best effort platform.android_ver() for the Android run time
    version. Things are changing very fast with the Android project and the OEM
    may add their own changes too.

    Hum, IMO we should at least use a structure with names for the sys function
    to be able to return more information later.

    Maybe call the function sys.getandroidversion()?

    @malemburg
    Copy link
    Member

    @malemburg malemburg commented Nov 20, 2016

    On 20.11.2016 00:59, STINNER Victor wrote:

    STINNER Victor added the comment:

    > I think we must use the reliable build time Android API level and
    implement sys.getandroidapilevel() (Victor patch) for knowing, in the
    standard library, whether we are running on an Android platform, and
    provide a best effort platform.android_ver() for the Android run time
    version. Things are changing very fast with the Android project and the OEM
    may add their own changes too.

    Hum, IMO we should at least use a structure with names for the sys function
    to be able to return more information later.

    Maybe call the function sys.getandroidversion()?

    Since this is an OS level interface, it should either go into
    the os module or, if it doesn't rely on a C API, into the platform
    module (see e.g. the platform.win32_ver() and .mac_ver() APIs
    as example).

    I don't think the sys module is the right place to put the API,
    since it doesn't have anything to do with the Python system
    internals.

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Nov 21, 2016

    I agree with Marc-Andre, Windows has sys.getwindowsversion() returning system information for the Windows version, and a platform.win32_ver() returning additional version information from the Registry obtained at run time. So it is consistent to add the sys.getandroidapilevel() function returning the version based on the ANDROID_API_LEVEL macro (a build, system level data, that specifies the libc version Python has been built against) and to add the platform.android_ver() version returning the device/emulator run time version obtained from the local properties system.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Dec 1, 2016

    getandroidapilevel-3.patch: Updated version of getandroidapilevel.patch: replace "runtime" with "build time" in the doc :-) Remove also "version > 0" check in support/init.py.

    About the version > 0 check: would it make sense to add the check in configure.ac? I guess that it was Xavier who wrote the test in support/init.py?

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Dec 1, 2016

    I suggest to start to add sys.getandroidapilevel(), and then discuss how to expose (or not) the runtime version.

    Reminder: The first step is to have a working Python on Android. I opened this issue to fix the issue bpo-28596. Getting the runtime version is a new Android-specific version, it can be done later.

    @yan12125
    Copy link
    Mannequin

    @yan12125 yan12125 mannequin commented Dec 1, 2016

    How about renaming sys.implementation._multiarch to sys.implementation.target_architecture and make it public? sys.androidapilevel() sounds too specific to me.

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Dec 1, 2016

    How about renaming sys.implementation._multiarch to sys.implementation.target_architecture and make it public? sys.androidapilevel() sounds too specific to me.

    Please do not hijack this issue. The removal of sys.implementation._multiarch for Android is discussed in bpo-28849 not here.

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Dec 1, 2016

    @Chi Hsuan Yen
    And please, let us not waste any more time on lost battles, this suggestion of using sys.implementation has already been rejected at bpo-27442 (see msg269748) as you must know since you were involved in the discussion there.

    @yan12125
    Copy link
    Mannequin

    @yan12125 yan12125 mannequin commented Dec 1, 2016

    Sorry for mixing different issues and proposing bad alternatives.

    My last hope is that someone looks into lemburg's msg281253:

    "I don't think the sys module is the right place to put the API, since it doesn't have anything to do with the Python system internals."

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Dec 1, 2016

    getandroidapilevel-3.patch: Updated version of getandroidapilevel.patch: replace "runtime" with "build time" in the doc :-) Remove also "version > 0" check in support/init.py.

    LGTM

    About the version > 0 check: would it make sense to add the check in configure.ac? I guess that it was Xavier who wrote the test in support/init.py?

    The version > 0 check was done because sysconfig.get_config_var() returns 0 when a variable is found as '#undef' in pyconfig.h. As a reference, the list of the existing API levels with their corresponding Android versions and names:

    Android Version Released API Level Name
    Android 7.1 October 2016 25 Nougat
    Android 7.0 August 2016 24 Nougat
    Android 6.0 August 2015 23 Marshmallow
    Android 5.1 March 2015 22 Lollipop
    Android 5.0 November 2014 21 Lollipop
    Android 4.4W June 2014 20 Kitkat Watch
    Android 4.4 October 2013 19 Kitkat
    Android 4.3 July 2013 18 Jelly Bean
    Android 4.2-4.2.2 November 2012 17 Jelly Bean
    Android 4.1-4.1.1 June 2012 16 Jelly Bean
    Android 4.0.3-4.0.4 December 2011 15 Ice Cream Sandwich
    Android 4.0-4.0.2 October 2011 14 Ice Cream Sandwich
    Android 3.2 June 2011 13 Honeycomb
    Android 3.1.x May 2011 12 Honeycomb
    Android 3.0.x February 2011 11 Honeycomb
    Android 2.3.3-2.3.4 February 2011 10 Gingerbread
    Android 2.3-2.3.2 November 2010 9 Gingerbread
    Android 2.2.x June 2010 8 Froyo
    Android 2.1.x January 2010 7 Eclair
    Android 2.0.1 December 2009 6 Eclair
    Android 2.0 November 2009 5 Eclair
    Android 1.6 September 2009 4 Donut
    Android 1.5 May 2009 3 Cupcake
    Android 1.1 February 2009 2 Base
    Android 1.0 October 2008 1 Base

    I suggest to start to add sys.getandroidapilevel(), and then discuss how to expose (or not) the runtime version.

    Agreed and thanks for the patch Victor :)

    @xdegaye xdegaye mannequin added the interpreter-core label Dec 1, 2016
    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Dec 2, 2016

    New changeset be70d64bbf88 by Victor Stinner in branch 'default':
    Add sys.getandroidapilevel()
    https://hg.python.org/cpython/rev/be70d64bbf88

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Dec 2, 2016

    The version > 0 check was done because sysconfig.get_config_var() returns 0 when a variable is found as '#undef' in pyconfig.h.

    Oh right, I see. In this case, we don't need to have a special case in sys.getandroidapilevel().

    I pushed getandroidapilevel-3.patch with a change: I added a small unit test. It checks that level > 0. I hesitated to also check for a maximum, but I'm not sure that it makes sense.

    Xavier: can please double test that sys.getandroidapilevel() works on Android? If it's the case, we can move on the issue bpo-28596 :-)

    @xdegaye
    Copy link
    Mannequin

    @xdegaye xdegaye mannequin commented Dec 2, 2016

    Here is the output of getandroidapilevel(), a verbose run of test_sys and a run of the test suite on the Android x86 emulator API 21. All the results are as expected, the failed tests are the usual ones, BTW all of the failed tests have either a patch ready to be commited as soon as the 3.6.1 branch is created, or have a patch ready, pending a review.

    root@generic_x86:/data/data/org.bitbucket.pyona # python
    Python 3.7.0a0 (default:96245d4af0ca+, Dec  2 2016, 07:59:12)
    [GCC 4.2.1 Compatible Android Clang 3.8.256229 ] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import sys
    >>> sys.getandroidapilevel()
    21
    >>>

    =======================================
    root@generic_x86:/data/data/org.bitbucket.pyona # python -m test -v test_sys
    ...
    test_getandroidapilevel (test.test_sys.SysModuleTest) ... ok
    ...
    Ran 45 tests in 2.567s

    OK (skipped=3)
    1 test OK.

    Total duration: 3 sec
    Tests result: SUCCESS

    =======================================
    root@generic_x86:/data/data/org.bitbucket.pyona # python -m test
    ...
    358 tests OK.

    8 tests failed:
    test_asyncio test_cmd_line test_pathlib test_pwd test_site
    test_socket test_tarfile test_warnings

    38 tests skipped:
    test_asdl_parser test_concurrent_futures test_crypt test_curses
    test_dbm_gnu test_dbm_ndbm test_devpoll test_gdb test_grp
    test_idle test_kqueue test_lzma test_msilib
    test_multiprocessing_fork test_multiprocessing_forkserver
    test_multiprocessing_main_handling test_multiprocessing_spawn
    test_nis test_ossaudiodev test_smtpnet test_socketserver test_spwd
    test_startfile test_tcl test_timeout test_tix test_tk
    test_ttk_guionly test_ttk_textonly test_turtle test_urllib2net
    test_urllibnet test_wait3 test_winconsoleio test_winreg
    test_winsound test_xmlrpc_net test_zipfile64

    Total duration: 20 min 31 sec
    Tests result: FAILURE

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Dec 2, 2016

    @xavier: Cool, thanks for checking :-) I don't have access to an Android platform yet.

    @vstinner vstinner closed this Dec 2, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 interpreter-core type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants