Skip to content
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

gh-115882: Add missing #include <Unknwn.h> on Windows #115350

Merged
merged 2 commits into from Feb 26, 2024

Conversation

georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented Feb 12, 2024

Copy link

cpython-cla-bot bot commented Feb 12, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Feb 12, 2024

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.

@georgthegreat georgthegreat changed the title Add missing #include <Unknwn.h> on Windows gh-115882: Add missing #include <Unknwn.h> on Windows Feb 24, 2024
@georgthegreat
Copy link
Contributor Author

georgthegreat commented Feb 24, 2024

Could anyone mark this with skip news label to get the CI green?

@hugovk
Copy link
Member

hugovk commented Feb 24, 2024

This should have a NEWS entry, please can you add one?

@georgthegreat
Copy link
Contributor Author

georgthegreat commented Feb 24, 2024

@hugovk, it looks like generating files in Misc/News.d/next is automated somehow.
It there any guide on how do I add an entry atop of the existing PR?

UPD: managed to do this by myself, sorry for disturbing.

@georgthegreat
Copy link
Contributor Author

NB: this is not the only patch needed to build CPython with this define.

I will have to dig through our patch set to get the remaining ones.

@georgthegreat
Copy link
Contributor Author

@hugovk, the CI is green, so two questions arise.

  1. Can we proceed with merging this PR?
  2. Is there a chance for this PR to be backported to 3.12 branch? Updating to 3.13 is a huge amount of work in our case and we would like to remove as many patches as possible prior to doing that.

@hugovk
Copy link
Member

hugovk commented Feb 26, 2024

  1. Can we proceed with merging this PR?

I'll leave it to a Windows expert to review.

  1. Is there a chance for this PR to be backported to 3.12 branch? Updating to 3.13 is a huge amount of work in our case and we would like to remove as many patches as possible prior to doing that.

Generally, bug fixes can be backported for all bugfix branches -- see https://devguide.python.org/versions/ -- currently 3.11 and 3.12. Again, I'll leave it to a Windows expert to decide.

@zooba
Copy link
Member

zooba commented Feb 26, 2024

This one is fine. But no promises that the rest will be fine - I've definitely fixed issues recently by removing LEAN_AND_MEAN specifications, and I'd rather keep them in than start referencing non-top-level header files directly.

I see no reason to backport, as this doesn't fix a bug (neither us nor Microsoft make any promises about things being buildable with WIN32_LEAN_AND_MEAN). Keep the patches up and you may be able to drop all your own patches for 3.13 onwards, but you'll want to hang onto them for 3.12 and prior.

@zooba zooba merged commit 96c10c6 into python:main Feb 26, 2024
34 checks passed
@georgthegreat georgthegreat deleted the patch-1 branch February 26, 2024 17:54
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…15350)

This allows the module to be compiled with WIN32_LEAN_AND_MEAN enabled
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…15350)

This allows the module to be compiled with WIN32_LEAN_AND_MEAN enabled
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…15350)

This allows the module to be compiled with WIN32_LEAN_AND_MEAN enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants