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
cleanup the stdlib and tests with regard to sys.platform usage #80805
Comments
Back in 2012 (bpo-12326 and bpo-12795), and just recently (bpo-36588) 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 |
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 :-) |
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? |
On 14/04/2019 18:04, Michael Felt wrote:
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_android = hasattr(sys, 'getandroidapilevel') diff --git a/Lib/test/test_c_locale_coercion.py 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 Comments welcome. |
My previous attempt was: #7800 Serhiy Storchaka and Ned Deily were unable about this change:
*If* a change is done, I would prefer to do it for Linux, macOS and Windows as well. |
support.is_android has two flaws:
In my PR, I used support.ANDROID. |
On 15/04/2019 11:50, STINNER Victor wrote:
I do not like the is_xxx form either, but I am low in the (name) picking I have the habit of using _ (underscore) before a name - but the clear As to being 'backported', even manually - that is something I would work So, I'll finish up for AIX - except I would like to underline again As, relatively speaking, a new-comer to Python, I see this as a vast As to new tests, modifications, etc. it will become part of the PR Anyway, I'll get started with AIX - not that many - and I hope with all
|
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. |
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. |
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. |
On 22/04/2019 21:14, Steve Dower wrote:
I thought I would try In either case - I do believe in a 'canonical' statement - that could be Looking forward - question - should the same canon be applied within the
Agreed. Although there shall always be some platform differences. Some Not directly related perhaps, but is a function absent as a "platform" I'll wait a bit for any other comments - and I am curious about thoughts Maybe the "common wisdom" is that the exceptions for the special
|
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> |
On 23/04/2019 17:53, Steve Dower wrote:
To that end, I modified another 60 lines, or so, of mainly sys.platform I also replaced the use of (support.)is_android and (support.is_java) Curious about comments from code owners. And suggestions re: the FYI: when I started there were 321 references to sys.platform with
|
There a cleanly rebased PR up at #57857. I'd like to get opinions from additional people about this now that the PR is ready. Steve? Victor? Some discussion which happened in PR comments: Andrew Svetlov approved the changes for the asyncio tests. Stefan Krah asked to revert the changes in test_decimal, saying "I don't quite like this change.". Michael Felt argued "imho - this adds clarity to the whole", to which Stefan replied: "I don't think so: Now I have to wonder which of the different idioms hides behind MS_WINDOWS. And in other project's setup.py files I still need sys.platform, so now there's even one additional way to accomplish the same thing." Michael followed up with additional arguments in favor of this change, which are rather verbose, but can be summed up as saying that this adds clarity and uniformity throughout the tests. |
FWIW, my opinion on making this kind of wholesale change has not changed: see the discussion in PR 7800. I think the changes made there were not an improvement for all the reasons stated, primarily because this now requires people reading the code base to learn *two* different ways of doing the same thing since these changes only affect the tests and not the platform-conditional code in the standard library modules themselves (which are not and should not be changed). Also, this means that backports of fixes from 3.8 will be complicated. Note there ware already some "translation" errors detected and fixed in the PR re-spin; how many others remain? |
On 29/05/2019 16:36, Ned Deily wrote:
I had actually read through that before I started on this. Your closing As someone who does not work 100% of the time with python - it is Why, I ask myself, is it sometimes "darwin" (or is it "Darwin" - oh yes, I (think I) understand your concerns. While I would consider going I had hoped to: a) improve consistency and set a good example; as well As we stand now I still have a concern/question - is there any What is presented here does not have to be the solution. I hope everyone All I can offer is my willingness to help. Thank you for your time spent reading!
|
Michael, your willingness to help, and the work on this issue and PR, are greatly appreciated! Reading through the discussion here again, and the one referenced by Ned, I tend to agree with the point that having *yet another* spelling for OS checking is perhaps not a good idea. The point that needing to see exactly which check is done in certain edge-cases is another good point against adding such new constants. Moreover, regardless of my opinion, there isn't a consensus and at this point I don't think there will be. Considering the above, perhaps it would be better to use only a single, "canonical" idiom throughout the tests, or at least from this point forward (to avoid a codebase-wide change)? Steve Dower said he would "prefer sys.platform to be canonical". I do suffer from having all of os.name, sys.platform and platform.system() used in various places, and it not being clear when one should be used instead of another. |
On 05/06/2019 07:07, Tal Einat wrote:
From experience, for AIX, and I think also for windows, not sure about More to the point - we all suffer - and some kind of direction is Further, imho - having it defined as a "constant" is not "yet another I have said before, and I'll say again - I am willing to do the manual Regards, Michael
|
The reason I'd prefer sys.platform in most cases is because it's a compile-time constant, based on the one that includes/excludes APIs near completely, and most of our tests ought to be switching on these. I personally don't see any need to switch on os.name, except perhaps in the tests for that module. But if we start skipping tests based on platform(), then we will miss out on seeing new failures on new platforms. This is a real concern, as I have colleagues currently adding Windows on ARM and ARM64 support, and since the API is the same we expect everything to work. It doesn't, of course, but that's what specific exclusions are for. If I were to propose a consistent scheme for this, I'd suggest:
So if some functionality only exists when MS_WINDOWS was defined, we check sys.platform == win32. But if it doesn't work on ARM, we check platform.platform == (whatever the value is). That way, adding new platforms will get the maximum amount of tests initially. |
Steve's suggestion sounds reasonable. Should we just add this to the devguide, then? |
On 06/06/2019 14:14, Tal Einat wrote:
Well, as I said before - it was never about THIS being the solution. Probably minor, but it is something I could do. Think of it as
|
Changing our policy here (from "no policy" to "here's a recommendation") probably deserves a python-dev discussion first. |
On 06/06/2019 19:08, Steve Dower wrote:
|
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: