classification
Title: cleanup the stdlib and tests with regard to sys.platform usage
Type: Stage: patch review
Components: Library (Lib), Tests Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Michael.Felt, steve.dower, vstinner
Priority: normal Keywords: patch

Created on 2019-04-13 12:25 by Michael.Felt, last changed 2019-04-25 07:31 by Michael.Felt.

Pull Requests
URL Status Linked Edit
PR 12843 open Michael.Felt, 2019-04-15 15:16
Messages (13)
msg340155 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-13 12:25
Back in 2012 (issue12326 and issue12795), and just recently (issue36588) sys.platform has been modified (and documented) to not return the platform version.

Additionally, the recommendation is to use the form sys.platform.startswith(<platform>) - to continue to be backwards compatible.

IMHO - looking forward - Python3.8 and later - we should not be using the recommendation for 'backwards-compatibility' in our code (so this PR will not be considered for back-porting) - in our stdlib, tests, and - should it occur - in "core" code. We should be testing for equality.

Further, imho, the change should not be sys.platform == <platform> but should be platform.system() == <Platform>, or platform.system() in ('AIX', 'Darwin', 'Linux') -- and adjust the list so that the most frequently used platform is tested first (e.g., performance-wise ('Linux', 'Darwin', 'AIX') would better reflect platform importance.

OR - should the change just continue to use sys.platform - even though this is a build-time value, not a run-time value.

I propose to do this in separate PR - one for each platform of AIX, Darwin and Linux.

(I would also add Windows, but that would be to replace the equivalence of sys.platform == 'win32' with platform.system() == 'Windows', and perhaps, os.name == 'nt' with platform.system() == 'Windows'. Reaction from other platforms dependent on os.name == 'nt' (cygwin?) would be helpful.)

Finally, while I do not want to rush this - I would like to try and target getting this complete in time for Python3.8
msg340160 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-13 14:05
I tried to add constants to test.support once to identify operating systems, nbut I had to revert the change. I am not sure that there is any problem here. Leaving the code unchanged is also fine :-)
msg340217 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-14 16:04
I took a peak at test.support.

a) I see that while many tests import test.support, or from test.support import <something> - not all tests use this.

b) I see that only 35 .py files in Lib/test have the string sys.platform.startswith, and there are 76 files that have sys.platform (so, there are roughly 40 files that have sys.platform without startswith).

I can start by adding _AIX to test.support and adding (as needed) from test.support import _AIX (and later _Linux, _Darwin) in the "35" files. If that seems to be working - and looking - proper the other 40 files could be added to the change.

Is this a good way to get started?
msg340233 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-14 20:32
On 14/04/2019 18:04, Michael Felt wrote:
> Is this a good way to get started?

So, as an example - seems to be many attributes in test/support/__init__.py

diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py
index 5bd15a2..e20567f 100644
--- a/Lib/test/support/__init__.py
+++ b/Lib/test/support/__init__.py
@@ -101,6 +101,8 @@ __all__ = [
     # network
     "HOST", "IPV6_ENABLED", "find_unused_port", "bind_port",
"open_urlresource",
     "bind_unix_socket",
+    # platform
+    "is_aix",
     # processes
     'temp_umask', "reap_children",
     # logging
@@ -815,6 +817,7 @@ requires_bz2 = unittest.skipUnless(bz2, 'requires bz2')
 requires_lzma = unittest.skipUnless(lzma, 'requires lzma')

 is_jython = sys.platform.startswith('java')
+is_aix = platform.system() == 'AIX'

 is_android = hasattr(sys, 'getandroidapilevel')

diff --git a/Lib/test/test_c_locale_coercion.py
b/Lib/test/test_c_locale_coercion.py
index 35272b5..0685ed8 100644
--- a/Lib/test/test_c_locale_coercion.py
+++ b/Lib/test/test_c_locale_coercion.py
@@ -10,6 +10,7 @@ import unittest
 from collections import namedtuple

 from test import support
+is_aix = support.is_aix
 from test.support.script_helper import (
     run_python_until_end,
     interpreter_requires_environment,
@@ -40,7 +41,7 @@ if sys.platform.startswith("linux"):
         # TODO: Once https://bugs.python.org/issue30672 is addressed,
we'll be
         #       able to check this case unconditionally
         EXPECTED_C_LOCALE_EQUIVALENTS.append("POSIX")
-elif sys.platform.startswith("aix"):
+elif is_aix:
     # AIX uses iso8859-1 in the C locale, other *nix platforms use ASCII
     EXPECTED_C_LOCALE_STREAM_ENCODING = "iso8859-1"
     EXPECTED_C_LOCALE_FS_ENCODING = "iso8859-1"

I had originally been thinking using _AIX, but the convention seems to
be is_xyzsomething.

Comments welcome.
msg340250 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-15 09:49
My previous attempt was: https://github.com/python/cpython/pull/7800

Serhiy Storchaka and Ned Deily were unable about this change:

* https://github.com/python/cpython/pull/7800#issuecomment-398688441
* https://github.com/python/cpython/pull/7800#issuecomment-399614918
* https://github.com/python/cpython/pull/7800#issuecomment-400134294

*If* a change is done, I would prefer to do it for Linux, macOS and Windows as well.
msg340251 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-15 09:50
support.is_android has two flaws:

* it's a constant: it must be spelled as UPPER CASE
* I dislike "is_" prefix: "MS_WINDOWS" constant is commonly used, and it doesn't start with "is_".

In my PR, I used support.ANDROID.
msg340273 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-15 14:14
On 15/04/2019 11:50, STINNER Victor wrote:
> STINNER Victor <vstinner@redhat.com> added the comment:
>
> support.is_android has two flaws:
>
> * it's a constant: it must be spelled as UPPER CASE
> * I dislike "is_" prefix: "MS_WINDOWS" constant is commonly used, and it doesn't start with "is_".

I do not like the is_xxx form either, but I am low in the (name) picking
order. I like ALL_CAPS because it is easier to recognize as a constant,

I have the habit of using _ (underscore) before a name - but the clear
consensus is to not use that for constants coming from support.test.

As to being 'backported', even manually - that is something I would work
on. I am an old dog - and this seems like a good enough bone for me.

So, I'll finish up for AIX - except I would like to underline again
something I have come across a few times (but cannot find right now) -
and that is to base these constants not on the platform being built on,
but the platform being run on (i.e., platform.system()). I expect there
may be specific tests that are relevant during the build moment, or
perhaps, "later", when using something such as 'pip' to add a module.

As, relatively speaking, a new-comer to Python, I see this as a vast
improvement to the readability (and clarity) of the code.

As to new tests, modifications, etc. it will become part of the PR
review to be sure this becomes and stays the standard.

Anyway, I'll get started with AIX - not that many - and I hope with all
the constant definitions being moved to one place that should simplify
maintenance (and perhaps even back-porting).

>
> In my PR, I used support.ANDROID.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue36624>
> _______________________________________
>
msg340328 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-16 10:02
OK. I have been chewing my bone. I hope not too much indigestion.

Who has a pointer for the antacid?

Getting base branch for PR ... origin/master
Getting the list of files that have been added/changed ... 72 files
Fixing Python file whitespace ... Traceback (most recent call last):
  File "../git/python3-3.8/Tools/scripts/patchcheck.py", line 285, in <module>
    main()
  File "../git/python3-3.8/Tools/scripts/patchcheck.py", line 253, in main
    normalize_whitespace(python_files)
  File "../git/python3-3.8/Tools/scripts/patchcheck.py", line 35, in call_fxn
    result = fxn(*args, **kwargs)
  File "../git/python3-3.8/Tools/scripts/patchcheck.py", line 149, in normalize_whitespace
    fixed = [path for path in file_paths if path.endswith('.py') and
  File "../git/python3-3.8/Tools/scripts/patchcheck.py", line 150, in <listcomp>
    reindent.check(os.path.join(SRCDIR, path))]
  File "/data/prj/python/git/python3-3.8/Tools/scripts/reindent.py", line 138, in check
    if r.run():
  File "/data/prj/python/git/python3-3.8/Tools/scripts/reindent.py", line 203, in run
    for _token in tokens:
  File "/data/prj/python/git/python3-3.8/Lib/tokenize.py", line 521, in _tokenize
    raise TokenError("EOF in multi-line statement", (lnum, 0))
tokenize.TokenError: ('EOF in multi-line statement', (694, 0))
make: 1254-004 The error code from the last command is 1.

In other words - I have not changed the file 'complaining', but have changed many files. Likely, a new issue - however, I would like to move forward with this one.
msg340334 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-16 12:05
Never mind - typos in the files I did work on. iow, I found a way to get the filename, and am cleaning up the errors.
msg340680 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-04-22 19:14
I like this, though I don't like using the platform module here and would prefer sys.platform to be canonical (until there's a need to differentiate - e.g. some tests coming for Windows ARM32 will need to be more specific).

In general, I'd like fewer tests to be platform specific and make more functionality "just work" across platforms, or at least platform families. I feel like more precise skips don't actually help with that - they make it too easy to say "this functionality just doesn't work here" instead of trying to make it work.
msg340715 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-23 11:10
On 22/04/2019 21:14, Steve Dower wrote:
> Steve Dower <steve.dower@python.org> added the comment:
>
> I like this, though I don't like using the platform module here and would prefer sys.platform to be canonical (until there's a need to differentiate - e.g. some tests coming for Windows ARM32 will need to be more specific).

I thought I would try `platform` as that already seemed to be more
portable over different versions (e.g., linux3, linux4, linux5, aix4,
aix5, aix7 coming from sys.platform while platform.system() was already
'Linux' and 'AIX' respectively). Personally, it makes difference to me.
Being 'runtime' rather than 'buildtime' seemed more precise - tests that
are not meant to be build-time dependent use run-time status while
leaving sys.platform for concerns that are directly related to builds
(e.g., cross-platform-builds; when adding modules using eggs or pip
where the module may want a "build-platform" dependency; etc).

In either case - I do believe in a 'canonical' statement - that could be
(later) documented in a PEP (e.g., update to PEP 8 if that is also
applicable to test writing).

Looking forward - question - should the same canon be applied within the
core? Here is "merely" looking at Lib/test

>
> In general, I'd like fewer tests to be platform specific and make more functionality "just work" across platforms, or at least platform families. I feel like more precise skips don't actually help with that - they make it too easy to say "this functionality just doesn't work here" instead of trying to make it work.

Agreed. Although there shall always be some platform differences. Some
"platform functions" will be absent or at least different.

Not directly related perhaps, but is a function absent as a "platform"
function when it is only available after a third-party (aka "asis")
library is installed? I, personally, have a hard time identifying what
is really "core" - asin - must be present for Python to be Python,
versus must be present to support a more (or less) commonly used 'module'.

I'll wait a bit for any other comments - and I am curious about thoughts
for the platforms 'ignored', e.g., hpux, cygwin, vmax(?), and a few more.

Maybe the "common wisdom" is that the exceptions for the special
platforms that are here - should just be removed - looking forward.
Perhaps being restored because someone working on (supporting) that
platform requests it being restored. I would hope this would also help
to further clean up the tests.

>
> ----------
> nosy: +steve.dower
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue36624>
> _______________________________________
>
msg340736 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-04-23 15:53
> Being 'runtime' rather than 'buildtime' seemed more precise - tests that
> are not meant to be build-time dependent use run-time status while
> leaving sys.platform for concerns that are directly related to builds

This is a good point - perhaps we need both?

Many of the current test skips are related to build-time switches, but indeed, some relate to the runtime environment.

One aspect I'm keeping in mind here is that with the Windows Subsystem for Linux, many lines are becoming blurred with respect to which build of Python people are using (I'm already getting bug reports from people who thought they were getting the Windows build but actually the Linux one, or vice versa). And mismatching builds causes some weird problems. Being able to clearly identify "Windows build + WSL environment" or "Linux build + WSL environment" will likely matter more over time.

It would be nice to have all the checks centralized, perhaps a set of decorators in a test.requires module?

@test.requires.linux_platform
@test.requires.darwin_platform
@test.requires.macos_runtime(10, 9)
def test_my_test(self): ...

(It would require some clever decorator design, but we can make those work like "ORs" rather than "ANDs".)

Does it provide any benefit? I think it's clever, but that doesn't mean it's worthwhile :) Using skipIf and skipUnless with test.support.CONSTANTS is just as readable and just as obvious (once we're using them consistently).

<End brainstorming>
msg340824 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-04-25 07:31
On 23/04/2019 17:53, Steve Dower wrote:
> Steve Dower <steve.dower@python.org> added the comment:
>
>> Being 'runtime' rather than 'buildtime' seemed more precise - tests that
>> are not meant to be build-time dependent use run-time status while
>> leaving sys.platform for concerns that are directly related to builds
> This is a good point - perhaps we need both?
>
> Many of the current test skips are related to build-time switches, but indeed, some relate to the runtime environment.
>
> One aspect I'm keeping in mind here is that with the Windows Subsystem for Linux, many lines are becoming blurred with respect to which build of Python people are using (I'm already getting bug reports from people who thought they were getting the Windows build but actually the Linux one, or vice versa). And mismatching builds causes some weird problems. Being able to clearly identify "Windows build + WSL environment" or "Linux build + WSL environment" will likely matter more over time.
>
> It would be nice to have all the checks centralized, perhaps a set of decorators in a test.requires module?
>
> @test.requires.linux_platform
> @test.requires.darwin_platform
> @test.requires.macos_runtime(10, 9)
> def test_my_test(self): ...
>
> (It would require some clever decorator design, but we can make those work like "ORs" rather than "ANDs".)
>
> Does it provide any benefit? I think it's clever,
Too clever for me to build. There is a lot about the Python language
(syntax) I do not understand well enough.
> but that doesn't mean it's worthwhile :) Using skipIf and skipUnless with test.support.CONSTANTS is just as readable and just as obvious (once we're using them consistently).

To that end, I modified another 60 lines, or so, of mainly sys.platform
== 'win32' to use MS_WINDOWS, not MS_WINDOWS, skipIf(MS_WINDOWS,...) or
SkipUnless(MS_WINDOWS,...) - or comparable for the other platforms.

I also replaced the use of (support.)is_android and (support.is_java)
with ANDROID and JYTHON.

Curious about comments from code owners. And suggestions re: the
sys.platform lines (roughly 50) that are left.

FYI: when I started there were 321 references to sys.platform with
roughly 315 involved in a string comparison of some kind. Now it is
closer to 50.

>
> <End brainstorming>
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue36624>
> _______________________________________
>
History
Date User Action Args
2019-04-25 07:31:05Michael.Feltsetmessages: + msg340824
2019-04-23 15:53:32steve.dowersetmessages: + msg340736
2019-04-23 11:10:40Michael.Feltsetmessages: + msg340715
2019-04-22 19:14:07steve.dowersetnosy: + steve.dower
messages: + msg340680
2019-04-16 12:05:54Michael.Feltsetmessages: + msg340334
2019-04-16 10:02:42Michael.Feltsetmessages: + msg340328
2019-04-15 15:16:15Michael.Feltsetkeywords: + patch
stage: patch review
pull_requests: + pull_request12768
2019-04-15 14:14:32Michael.Feltsetmessages: + msg340273
2019-04-15 09:50:12vstinnersetmessages: + msg340251
2019-04-15 09:49:04vstinnersetmessages: + msg340250
2019-04-14 20:32:05Michael.Feltsetmessages: + msg340233
2019-04-14 16:04:40Michael.Feltsetmessages: + msg340217
2019-04-13 14:05:57vstinnersetnosy: + vstinner
messages: + msg340160
2019-04-13 12:25:43Michael.Feltcreate