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

[3.12] gh-105020: Share tp_bases and tp_mro Between Interpreters For All Static Builtin Types (gh-105115) #105124

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented May 31, 2023

In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process.
(cherry picked from commit 7be667d)

Co-authored-by: Eric Snow ericsnowcurrently@gmail.com

…ll Static Builtin Types (pythongh-105115)

In pythongh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry.  However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses).  We address that here by reverting back to shared objects, making them immortal in the process.
(cherry picked from commit 7be667d)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@ericsnowcurrently
Copy link
Member

@Yhg1s, what do I need to do to fix the ABI check? It's a false positive.

@Yhg1s
Copy link
Member

Yhg1s commented Jun 1, 2023

@Yhg1s, what do I need to do to fix the ABI check? It's a false positive.

As the failed check says:

The up to date ABI file should be attached to this build as an artifact.

To learn more about this check: https://devguide.python.org/setup/#regenerate-the-abi-dump

@zooba recently merged support for the CI check to produce the necessary artifact (#105088) but I don't think the devguide has been updated yet. (You may need to update the branch to get the updated check?)

For the record, the failure is not a false positive, it's just a change in the ABI that is okay (because all direct use of the changed struct is internal-only)

@zooba
Copy link
Member

zooba commented Jun 1, 2023

The devguide has a mention now, but not step-by-step instructions (which would probably require screenshots of the GitHub UI that would likely go out of date and are probably too big for the doc page).

When the PR check fails, the associated run will have the updated ABI file attached as an artifact. After release manager approval, you can download and add this file into your PR to pass the check.

(Worth pointing out that "the PR check" has already been mentioned in the section, so it's not being newly introduced in the new text.)

Only trick seems to be that the artifacts are listed on the Summary page, not the specific check that published it. Perhaps we can figure out how to infer the URL to the summary page and add a link in that message? In this case, it'd be https://github.com/python/cpython/actions/runs/5127462177?pr=105124#artifacts.

Changes to _PyRuntime caused the ABI check to fail.  We're okay to move forward since the *public* ABI wasn't touched.
@ericsnowcurrently
Copy link
Member

@zooba I was able to use that artifact, which saved me a bunch of work. Thanks!

@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) June 1, 2023 22:22
@ericsnowcurrently ericsnowcurrently merged commit c38ceb0 into python:3.12 Jun 1, 2023
@miss-islington miss-islington deleted the backport-7be667d-3.12 branch June 1, 2023 22:25
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.

5 participants