GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [3.12] gh-105020: Share tp_bases and tp_mro Between Interpreters For All Static Builtin Types (gh-105115) by miss-islington · Pull Request #105124 · python/cpython

Conversation

@miss-islington

@miss-islington miss-islington commented May 31, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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 [email protected]

  • Issue: tp_bases of object is NULL: undocumented or unintentional behavior change in 3.12? #105020

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ericsnowcurrently @miss-islington
pythongh-105020: Share tp_bases and tp_mro Between Interpreters For A…
c28c929 
…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 <[email protected]>
@miss-islington
miss-islington requested a review from markshannon as a code owner May 31, 2023 00:15
@bedevere-bot bedevere-bot added the awaiting review label May 31, 2023
This was referenced May 31, 2023
tp_bases of object is NULL: undocumented or unintentional behavior change in 3.12? #105020
Closed
gh-105020: Share tp_bases and tp_mro Between Interpreters For All Static Builtin Types #105115
Merged
@bedevere-bot bedevere-bot added the skip news label May 31, 2023
@ericsnowcurrently
ericsnowcurrently requested review from Yhg1s and removed request for markshannon May 31, 2023 00:15
@ericsnowcurrently

ericsnowcurrently commented May 31, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

Yhg1s commented Jun 1, 2023

Copy link
Copy Markdown
Member

@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)
ericsnowcurrently 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.
@zooba

zooba commented Jun 1, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
Update python3.12.abi
82d76e0 
Changes to _PyRuntime caused the ABI check to fail.  We're okay to move forward since the *public* ABI wasn't touched.
@ericsnowcurrently

ericsnowcurrently commented Jun 1, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
ericsnowcurrently enabled auto-merge (squash) June 1, 2023 22:22
@ericsnowcurrently
ericsnowcurrently merged commit c38ceb0 into python:3.12 Jun 1, 2023
@bedevere-bot bedevere-bot removed the awaiting review label Jun 1, 2023
@miss-islington
miss-islington deleted the backport-7be667d-3.12 branch June 1, 2023 22:25
@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2023
3.12 backport gh 105236 #105358
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yhg1s Yhg1s Awaiting requested review from Yhg1s

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

5 participants

@miss-islington @ericsnowcurrently @Yhg1s @zooba @bedevere-bot
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.