bpo-33671: Add support.MS_WINDOWS and support.MACOS #7800
Conversation
* MS_WINDOWS: True if Python is running on Microsoft Windows. * MACOS: True if Python is running on Apple macOS.
|
This change is not related to fast file copies, it just that I found the idea when reading commits of https://bugs.python.org/issue33671 :-) |
|
I tried to write the smallest change. There are much more tests testing the platform, but I don't think that it's worth it to update all existing code. |
|
Nice! Why not cover all platforms? $ grep -r "sys.platform" Lib/test/ | grep bsd | wc -l
19
$ grep -r "sys.platform" Lib/test/ | grep solaris | wc -l
2
$ grep -r "sys.platform" Lib/test/ | grep aix | wc -l
18
$ grep -r "os.name" Lib/test/ | grep posix | wc -l
41You can use psutil as an example: Also, (personal taste), I would use "WINDOWS" instead of "MS_WINDOWS". It would be nice to have a test case in Lib\test\test_support.py which makes sure that only 1 of such constants is true and all others are False. |
There was a problem hiding this comment.
I agree with @giampaolo - ditch the MS prefix.
Also these should either be is_windowsand is_macos - to match the existing is_android and is_jython - or the latter should be changed to ANDROID and JYTHON.
| @@ -108,6 +108,18 @@ | |||
| "run_with_tz", "PGO", "missing_compiler_executable", "fd_count", | |||
| ] | |||
|
|
|||
|
|
|||
| # True if Python is running on Microsoft Windows. | |||
| MS_WINDOWS = (sys.platform == 'win32') | |||
There was a problem hiding this comment.
Are we happy this is suitable for replacing all of the following without changing behaviour?
mswindows = (sys.platform == "win32")
MS_WINDOWS = (sys.platform == 'win32')
MS_WINDOWS = (os.name == 'nt')
WIN32 = (sys.platform == "win32")
sys.platform[:3] == 'win'There was a problem hiding this comment.
sys.platform[:3] == 'win'
On "win64", Python uses "win32" as well.
MS_WINDOWS = (os.name == 'nt')
I'm not sure about Cygwin which is a strange beast.
| MS_WINDOWS = (sys.platform == 'win32') | ||
|
|
||
| # True if Python is running on Apple macOS. | ||
| MACOS = (sys.platform == 'darwin') |
There was a problem hiding this comment.
Likewise here:
MACOS = sys.platform.startswith("darwin")
sys.platform == 'darwin'There was a problem hiding this comment.
MACOS = sys.platform.startswith("darwin")
I don't see the purpose of this test. sys.platform is always the "darwin" string on macOS, no?
A test that ensures Windows, macOS, Linux, etc. are exclusive would make sense, but note that Linux and POSIX, for example, can both be true. |
See my previous comment: #7800 (comment) I don't want to write a giant "coding style change". It would make backports painful for everyone, especially for downstream patches. But maybe I will backport this small change to all stable branches. |
Lib/test/support/__init__.py
Outdated
| is_android = hasattr(sys, 'getandroidapilevel') | ||
|
|
||
| if sys.platform != 'win32': | ||
| if not MS_WINDOWS: |
There was a problem hiding this comment.
if MS_WINDOWS:
...
elif is_androif:
...
else:
...There was a problem hiding this comment.
Hum, this change is unrelated, but ok, let's do that :-)
| @@ -108,6 +108,18 @@ | |||
| "run_with_tz", "PGO", "missing_compiler_executable", "fd_count", | |||
| ] | |||
|
|
|||
|
|
|||
| # True if Python is running on Microsoft Windows. | |||
| MS_WINDOWS = (sys.platform == 'win32') | |||
There was a problem hiding this comment.
Wouldn't be better to use is_windows and is_macos for uniformity with is_jython and is_android?
Or rename the latter two?
There was a problem hiding this comment.
I replied here: #7800 (comment)
If we do something, I would prefer to rename is_android and is_jython to ANDROID and JYTHON to be more consistent with PEP 8 and existing code (rather than adding support.is_windows and support.is_macos).
|
What is the benefit from this change? |
MS_WINDOWS or mswindows is already used in the code base. I guess that it avoids confusion with a physical window. Think about GUI tests like IDLE or Tkinter. Moreover, the C code uses "#ifdef MS_WINDOWS": so it's also about consistency. |
I hesitate to rename lowercase is_android to ANDROID, but I have no strong opinion about that one. Maybe it's fine to keep inconstency here? Again, uppercase MS_WINDOWS is also used in C: "#ifdef MS_WINDOWS". |
I always hesitate between (os.name == 'nt') and (sys.platform == 'win32'): sys.platform == 'win32' looks suspicious on 64-bit Windows, and NT is the legacy name of Windows... I prefer an obvious MS_WINDOWS constant: it's no needed to guess what is the correct way to detect Windows, and the name is more explicit and less surprising. Same for macOS: "darwin" is not well known, at least, less known than "macOS". Darwin is the name of the macOS kernel. More generally, there are already MACOS and MS_WINDOWS constants. Another goal is to factorize the code. |
Put all OS-recognizing logic in one place. |
In Python 3, testing for Linux is just sys.platform == 'linux'. IMHO the test is obvious enough, no need for an additional constant. For FreeBSD, in tests usually we want to skip tests on old versions of FreeBSD, and we already have @requires_freebsd_version decorator for that.
Again, IMHO this test is explicit enough, no need for an additonal constant.
I don't recall that any unit test uses/needs that. I prefer to limit my PR to Windows and macOS for these reasons: |
+1 for backporting this. That's probably the best as it would allow to expose all the necessary constants instead of just a few. |
Please don't pretend that there is a shared BSD target. And even if 3 of them do something in the same way, there are other unrelated BSD systems including dead ones. The main ones BSDs diverged in 1995! And there were even decade older forks like 386BSD. It's almost like pretending that MINIX and LINUX should share a common check. |
I see. I guess it makes sense. For the record, this argument is valid also for Solaris which can be detected in 2 ways: There are 8 occurrences for that in Lib/test. |
|
"For the record, this argument is valid also for Solaris which can be detected in 2 ways: (...) There are 8 occurrences for that in Lib/test." Well, it's uncommon and Solaris is dying... :-) |
|
Solaris has set EOL dates and the Illumos community.. isn't growing. |
|
I added two additional changes to make my PR and test.support more consistent:
|
|
If someone wants to add a new constant for a different OS (BSD, Solaris, whatever), please open a different issue. This PR is more about moving existing constants to test.support than adding new constants. |
|
Thank you for the review and advices @serhiy-storchaka, @giampaolo, @pope1ni and others! |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7. |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
Sorry, @vstinner, I could not cleanly backport this to |
|
Sorry, @vstinner, I could not cleanly backport this to |
|
Sorry, @vstinner, I could not cleanly backport this to |
|
Argh! Why would you make such a major change as part of a totally unrelated issue? It's major enough to warrant its own issue and discussion first. I'm really not keen on seeing all this questionable churn backported to the existing branches but without backporting that's going to make ongoing maintenance (i.e. automatic backporting) problematic. And people submitting changes are still going to need to be familiar with spellings like |
|
Also, this change proposes a change in coding style for the tests, at least, that contributors to cpython, both core developers and non-committers, need to be aware of or else, over time, the existing spellings will start to reappear in the tests like they appear in the standard library. |
@ned-deily: I'm not sure if you think about this change? Should it be reverted? I understand that you are are ok to backport it to other branches. Are you suggesting to replace all sys.platform/os.name tests with the new constants? I propose to not modify everything to reduce the risk of conflicts on upstream and downstream patches.
It's ok :-) |
|
@vstinner It seems to me there are a number of issues here. Let me see if I can articulate them.
a change being proposed in a follow-on PR is more readable, or even, were it available: ) So, I honestly don't know what the right thing to do for master is. It's not my call for 3.8 as I'm not wearing the release manager hat for it. But as a fellow core developer I think such a change deserves more exposure and possibly discussion before we follow it to its logical conclusion and people start proposing to make more wholesale changes throughout the code base (as they already are). And as a core developer, I would not be unhappy if it were reverted from master. With my 3.7 and 3.6 release manager hat on, I'm a strong -1 on backporting unless someone can come up with a really compelling case to do so. |
|
I think it is better for us (CPython core developers) to backport this PR. Otherwise the difference between master and maintained branches will make harder backporting other changes. On other side, backporting it will make the life harder for external developers: maintainers of Linux distributives which backport bug fixes to unmaintained versions, core developers of PyPy and other implementations which synchronize tests with CPython, etc. |
|
Oh wow, I didn't expect so much discussions around just twice simple constants... I didn't know that two constants in tests was a major change. I'm not interested to spend more time on this change and so I just reverted it (PR #7919), sorry for the noise. |
|
Thanks, @vstinner! |
|
FYI - We do state that test.support is not a public API and may change without concern for backwards compatibility between releases in https://docs.python.org/3/library/test.html#module-test.support I have no opinion on the constant names other than as this PR points out they are inconsistent and messy today. In distant past workflows we'd sometimes not bother to add nicer cleaned up code because it'd make forward porting more annoying. Now we're not adding nicer more consistent code because it makes backporting more difficult? There is no winning long term when optimizing for merging ease as opposed to making the master branch better and living with the manual merge fixups on the backports. |
|
@gpshead I agree that test.support is not a public API and I agree that, in general, it is better to make the master branch better rather than optimizing for merges. But, as I tried to point out, I don't think this particular change makes the master branch better in that it would have added yet another set of ways to spell various conditions without completely removing the old spellings. Also note that the now reverted PR was just the start: there were many more tests that would have been needed to change but the old spellings would still remain in the standard library outside of the tests. To me, it made things more confusing, not less. But it's a moot point now. |
https://bugs.python.org/issue33671