GeezerVison

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

GeezerVison is showing github.com — gh-125783: Add tests to prevent regressions with the combination of `ctypes` and metaclasses. by junkmd · Pull Request #125881 · python/cpython

Conversation

junkmd commented Oct 23, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

I would like to backport this to 3.12 and 3.13 as well.

Issue: Add tests to prevent regressions with the combination of ctypes and metaclasses. #125783

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot added the tests Tests in the Lib/test dir label

Oct 23, 2024

bedevere-app Bot mentioned this pull request

Oct 23, 2024

Add tests to prevent regressions with the combination of ctypes and metaclasses. #125783

Closed

bedevere-app Bot commented Oct 23, 2024

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot added the awaiting review label

Oct 23, 2024

ZeroIntensity commented Oct 24, 2024

Copy link

Copy Markdown

Member

This is an internal-only change, I don't think it needs a NEWS entry.

junkmd reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

ZeroIntensity added skip news topic-ctypes labels

Oct 24, 2024

junkmd added 2 commits

October 24, 2024 12:49


          Add a test.

80deddb

Such an implementation is used in `IUnknown` of `comtypes`.


          Add a test.

cf81f03

Such an implementation is used in `CoClass` of `comtypes`.

junkmd force-pushed the test_metacls_PyCSimpleType branch from 1f4eecd to cf81f03 Compare

October 24, 2024 12:50

ZeroIntensity reviewed

Oct 24, 2024

View reviewed changes

ZeroIntensity left a comment

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.

I would really rather not use _ in local names here, it makes it unclear that we're testing something public.

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji

All reactions

👍 1 reaction


          Remove the _ prefix from local names.

junkmd mentioned this pull request

Oct 24, 2024

Breaking backward compatibility between ctypes and metaclasses in Python 3.13. #124520

Closed

ZeroIntensity approved these changes

Oct 24, 2024

View reviewed changes

ZeroIntensity left a comment

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.

Thank you! Take my review as provisional--I am concerned about the use of _pointer_type_cache, as that's private, but it looks like it's not terribly abused in this case (I think?)

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with rocket emoji

All reactions

🚀 1 reaction

bedevere-app Bot added awaiting core review and removed awaiting review labels

Oct 24, 2024

ZeroIntensity requested a review from encukou

October 24, 2024 13:28

junkmd commented Oct 24, 2024

Copy link

Copy Markdown

Contributor Author

comtypes has been using _pointer_type_cache since the first commit (an impressive 18 years ago!).
https://github.com/enthought/comtypes/blob/938fbd50d2077c6bd524790a9ed798c3990cdfc0/comtypes/__init__.py#L230-L231

This was also discussed in #125783, and it seems that these implementations were made with careful consideration, as the originator of both comtypes and ctypes is the same person.

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou reviewed

Oct 25, 2024

View reviewed changes

encukou left a comment

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.

Thank you for the test! I'd only like to change the names and add some comments.

The main thing we're testing here instantiation of a class in its own __new__, not the early return, that's there to avoid recursion.

Sorry, something went wrong.

Uh oh!

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

junkmd reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Lib/test/test_ctypes/test_c_simple_type_meta.py Resolved

Uh oh!

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

Comment thread

Lib/test/test_ctypes/test_c_simple_type_meta.py Resolved

Uh oh!

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

encukou commented Oct 25, 2024

Copy link

Copy Markdown

Member

I plan to commit the suggestions and merge next week, if there are no objections.

junkmd reacted with rocket emoji

All reactions

🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

junkmd and others added 2 commits

October 26, 2024 00:23


          Apply suggestions from code review

47ec482

Co-authored-by: Petr Viktorin <[email protected]>


          Update comments.

4f1b74c

junkmd commented Oct 25, 2024

Copy link

Copy Markdown

Contributor Author

Can we apply needs backport to 3.12 and needs backport to 3.13 labels to this PR?

All reactions

Sorry, something went wrong.

Uh oh!

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

ZeroIntensity commented Oct 25, 2024

Copy link

Copy Markdown

Member

I'm not sure if we backport increased test coverage.

All reactions

Sorry, something went wrong.

Uh oh!

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

junkmd requested a review from encukou

October 25, 2024 16:12

encukou added needs backport to 3.12

only security fixes

needs backport to 3.13 only security fixes labels

Oct 25, 2024

encukou commented Oct 25, 2024

Copy link

Copy Markdown

Member

We generally do, mostly to make it easier to backport later changes that need tests.

ZeroIntensity and junkmd reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

encukou merged commit 1384409 into python:main

Oct 25, 2024

miss-islington-app Bot commented Oct 25, 2024

Copy link

Copy Markdown

Thanks @junkmd for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the awaiting core review label

Oct 25, 2024

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Oct 25, 2024


          pythongh-125783: Add tests to prevent regressions with the combinatio…

51ace8a

…n of `ctypes` and metaclasses. (pythonGH-125881)

(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Oct 25, 2024


          pythongh-125783: Add tests to prevent regressions with the combinatio…

ad5349e

…n of `ctypes` and metaclasses. (pythonGH-125881)

(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>

bedevere-app Bot commented Oct 25, 2024

Copy link

Copy Markdown

GH-125987 is a backport of this pull request to the 3.13 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.13 only security fixes label

Oct 25, 2024

bedevere-app Bot commented Oct 25, 2024

Copy link

Copy Markdown

GH-125988 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Oct 25, 2024

junkmd deleted the test_metacls_PyCSimpleType branch

October 25, 2024 23:22

This was referenced Oct 26, 2024

Supporting Python 3.13 enthought/comtypes#618

Closed

Future of this package enthought/comtypes#216

Open

encukou pushed a commit that referenced this pull request

Oct 28, 2024


          [3.13] gh-125783: Add tests to prevent regressions with the combinati…

86ec8aa

…on of `ctypes` and metaclasses. (GH-125881) (#125987)

gh-125783: Add tests to prevent regressions with the combination of `ctypes` and metaclasses. (GH-125881)
(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>

encukou pushed a commit that referenced this pull request

Oct 29, 2024


          [3.12] gh-125783: Add tests to prevent regressions with the combinati…

bce9df9

…on of `ctypes` and metaclasses. (GH-125881) (GH-125988)

cherry picked from commit 1384409
by Jun Komoda

Also: Add test_ctypes/_support.py from 3.13+
This partially backports be89ee5
(#113727)
by AN Long


Co-authored-by: Jun Komoda <[email protected]>
Co-authored-by: AN Long <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>

junkmd mentioned this pull request

Oct 29, 2024

gh-125783: Add more tests to prevent regressions with the combination of ctypes and metaclasses. #126126

Merged

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request

Jan 12, 2025


          pythongh-125783: Add tests to prevent regressions with the combinatio…

a761341

…n of `ctypes` and metaclasses. (pythonGH-125881)

Reviewers

ZeroIntensity

ZeroIntensity approved these changes

encukou

Awaiting requested review from encukou

Uh oh!

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

Assignees

No one assigned

Labels

skip news tests

Tests in the Lib/test dir

topic-ctypes

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.

3 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.

GeezerVison

Conversation

junkmd commented Oct 23, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app Bot commented Oct 23, 2024

Uh oh!

ZeroIntensity commented Oct 24, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

junkmd commented Oct 24, 2024

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

encukou commented Oct 25, 2024

Uh oh!

junkmd commented Oct 25, 2024

Uh oh!

ZeroIntensity commented Oct 25, 2024

Uh oh!

encukou commented Oct 25, 2024

Uh oh!

miss-islington-app Bot commented Oct 25, 2024

Uh oh!

bedevere-app Bot commented Oct 25, 2024

Uh oh!

bedevere-app Bot commented Oct 25, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

junkmd commented Oct 23, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

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