GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — [WIP] bpo-32592: Drop support for Windows Vista by vstinner · Pull Request #5231 · python/cpython

Conversation

vstinner commented Jan 18, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Windows Vista extended support ended in April 2017. As stated in PEP
11, Python 3.7 drops support for Windows Vista and now requires
Windows 7 or newer.

https://bugs.python.org/issue32592

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner requested review from zooba and zware

January 18, 2018 09:38

vstinner requested a review from a team as a code owner

January 18, 2018 09:38

the-knights-who-say-ni added the CLA signed label

Jan 18, 2018

bedevere-bot added the awaiting merge label

Jan 18, 2018


          bpo-32592: Drop support for Windows Vista

1a83ced

Windows Vista extended support ended in April 2017. As stated in PEP
11, Python 3.7 drops support for Windows Vista and now requires
Windows 7 or newer.

vstinner commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

Hum, the change should be mentionned in Doc/whatsnew/3.7.rst.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

AppVeyor compilation failed with: "C:\Program Files (x86)\Windows Kits\10\Include\10.0.16299.0\shared\sdkddkver.h(289): fatal error C1189: #error: NTDDI_VERSION setting conflicts with _WIN32_WINNT setting [C:\projects\cpython\PCbuild\pythoncore.vcxproj]"

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matrixise reviewed

Jan 18, 2018

View reviewed changes

Comment thread

Include/internal/condvar.h

		#define WIN32_LEAN_AND_MEAN
		#include <windows.h>

		/* options */

matrixise Jan 18, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could change the comment before #define Py_HAVE_CONDVAR ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

matrixise Jan 18, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact, should we remove all the references to Vista when we drop the support of Vista ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Jan 18, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matrixise reacted with thumbs up emoji

All reactions

👍 1 reaction

matrixise commented Jan 18, 2018

Copy link

Copy Markdown

Member

@vstinner There is some references to Vista in the Tools/msi/ directory.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Remove condvar emulation (_PY_EMULATED_WIN_CV)

7dc2b5d

Rephrase also a comment in condvar.h.

vstinner commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

@vstinner There is some references to Vista in the Tools/msi/ directory.

I don't know this directory, so I prefer to leave it unchanged.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matrixise commented Jan 18, 2018

Copy link

Copy Markdown

Member

@vstinner ok then in this case, the PR seems to be right.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba reviewed

Jan 18, 2018

View reviewed changes

Comment thread

Python/condvar.h

		* example native support on VISTA and onwards.
		*/

		#if _PY_EMULATED_WIN_CV

zooba Jan 18, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://bugs.python.org/issue29871 before pulling this code out - our alternate code isn't 100% correct yet.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Jan 18, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I can revert this change and propose it in https://bugs.python.org/issue29871.

My change should have no effect on Windows 7 and newer. Why do you prefer to keep the unused emulation code (until bpo-29871 is fixed)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

AppVeyor compilation failed with: "C:\Program Files (x86)\Windows Kits\10\Include\10.0.16299.0\shared\sdkddkver.h(289): fatal error C1189: #error: NTDDI_VERSION setting conflicts with _WIN32_WINNT setting [C:\projects\cpython\PCbuild\pythoncore.vcxproj]"

@zooba: Do you understand why my change breaks the compilation?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun reviewed

Jan 18, 2018

View reviewed changes

Comment thread

Include/internal/condvar.h

		* to target Windows Vista. Modify this macro to enable them.
		*/
		#ifndef _PY_EMULATED_WIN_CV
		#define _PY_EMULATED_WIN_CV 1 /* use emulated condition variables */

eryksun Jan 18, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't removing this a mistake? I think the SRWLOCK version was tried in another issue that was abandoned.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Jan 18, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, this code seems to sensitive and my change may conflict with https://bugs.python.org/issue29871 so I reverted this change and will open a new dedicated PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner added 3 commits

January 18, 2018 15:19


          Revert: keep _PY_EMULATED_WIN_CV

764f4a8


          Revert: Keep _WIN32_WINNT_VISTA

747ee70


          Merge branch 'master' into drop_vista

1ed070d

vstinner commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

"posixmodule.obj : error LNK2001: unresolved external symbol _GetMaximumProcessorCount [C:\projects\cpython\PCbuild\pythoncore.vcxproj]"

Oh, my posixmodule.c change doesn't work :-(

Extract of pythoninfo:

sys.windowsversion: sys.getwindowsversion(major=10, minor=0, build=14393, platform=2, service_pack='')
sys.winver: 3.7-32
sys.version: 3.7.0a4+ (heads/master-2-g1d7f51e:1d7f51e, Jan 18 2018, 05:42:21) [MSC v.1912 32 bit (Intel)]

I understand that AppVeyor runs Windows 10 and Python was built by Visual Studio 2017

https://msdn.microsoft.com/fr-fr/library/windows/desktop/dd405489(v=vs.85).aspx

"Remarks: To compile an application that uses this function, set _WIN32_WINNT >= 0x0601. For more information, see Using the Windows Headers."

Aha. I need to try to understand why changing _WIN32_WINNT failed. But my Windows VM is down :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner changed the title ~~bpo-32592: Drop support for Windows Vista~~ [WIP] bpo-32592: Drop support for Windows Vista

Jan 18, 2018

vstinner commented Jan 18, 2018

Copy link

Copy Markdown

Member Author

Hum, I'm not sure that my PR is correct anymore. I change its status to WIP, I will rework on it, once I get again access to a Windows VM to fix my PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matrixise commented Jan 26, 2018 via email

Copy link

Copy Markdown

Member

Some news about this PR ?

…

-- Stéphane Wirtel - http://wirtel.be - @matrixise

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner closed this

Feb 1, 2018

vstinner deleted the drop_vista branch

May 29, 2018 21:48

vstinner mentioned this pull request

May 10, 2022

Drop support of Windows Vista and Windows 7 #76773

Closed

zware removed the awaiting merge label

Nov 7, 2022

Reviewers

eryksun

eryksun left review comments

zooba

zooba left review comments

zware

Awaiting requested review from zware

+1 more reviewer

matrixise

matrixise left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

vstinner commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 18, 2018

Uh oh!

vstinner commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matrixise commented Jan 18, 2018

Uh oh!

vstinner commented Jan 18, 2018

Uh oh!

matrixise commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 18, 2018

Uh oh!

vstinner commented Jan 18, 2018

Uh oh!

matrixise commented Jan 26, 2018 via email

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vstinner commented Jan 18, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.