All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

The issue quoted existing code for cmd in ('ver', 'command /c ver', 'cmd /c ver'):. Are all 3 options still needed for the Windows versions we currently support? Are any of these specific to Win95, ..., WinXp, etc?

The issue quoted existing code for cmd in ('ver', 'command /c ver', 'cmd /c ver'):. Are all 3 options still needed for the Windows versions we currently support? Are any of these specific to Win95, ..., WinXp, etc?

'command /c ver' - I think its relevant till Windows ME actually.
ver - straight from cmd.exe
cmd /c ver - will work on cmd.exe, although as i understand it intended for powershell.

I think we should remove this code completely, but if not it will be better to stay just with ver

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 phrase I 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.

That said, the actual executable that is run could be improved. Explicitly launching cmd /C ver seems best to me - the others make more assumptions that we can avoid.

We should avoid using ctypes here, and use a native function. sys.getwindowsversion already exists and is already used in this code, and WMI gets the most accurate result and is tried first.

There is no need for this change, but at the very least, it should not use ctypes and should not use kernel mode APIs when user mode ones exist.

That said, the actual executable that is run could be improved. Explicitly launching cmd /C ver seems best to me - the others make more assumptions that we can avoid.

This is already fallback code for if WMI doesn't work, and GetVersionEx is already provided by sys.getwindowsversion. I don't think there's anything to change here.

Hmm, lemme address.

1. Invoking process just to query what OS you run on is bad, very bad practice. Its very bad performance-wise and much worse, security-wise. if you do it 3 times (!) it even worse. That line of code was committed 23 years (!) ago. I think that either way it should be deleted completely and no process invokes should be done.

2. GetVersionEx - There're 2 major problems with it:

   • "may be altered or unavailable for releases after Windows 8.1" (according to MSDN).
   • The compitability issue - which means this function isn't accurate enough to our purpose -

     cpython/Python/sysmodule.c

     Line 1628 in e119526

     sys_getwindowsversion_impl(PyObject *module)

3. sys.getwindowsversion - because GetVersionEx cannot be trusted, it does some very long walk around, it calls GetFileVersionInfoW (on kernel32.dll) to validate the OS version. Although it probably works fine.

4. ctypes - I'm not sure why we should avoid it. I'll be happy to hear. Anyway, ctypes is already used inside this code (android_ver).

5. rtlGetVersion - That's not a kernel mode API. I meant the user mode rtlGetVersion of course. https://learn.microsoft.com/en-us/windows/win32/devnotes/rtlgetversion

After all I think that:

1. We must remove the process invoking anyway.
2. rtlGetVersion is accurate and efficient fallback.
3. sys.getwindowsversion - is a possible fallback also, we can use both.

Thank you !

You should propose a PR that removes the subprocess functionality then. It's already fallback code, so it doesn't need a replacement - we can simply say that if WMI fails, you won't get the version number (or you may get one that reflects the configured compatibility mode, rather than reality).

As a policy, we don't use ctypes in the standard library - some distributions remove ctypes (to avoid all the security issues), and we want them to be able to do it. But I don't review every line of code, and not every core developer is aware of this policy, so sometimes it slips in. If it's being used elsewhere in this file, we should remove it.