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 — gh-85283: _stat extension uses the limited C API by vstinner · Pull Request #108573 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Aug 28, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The _stat C extension is now built with the limited C API.

  • Issue: [C API] Convert a few stdlib extensions to the limited C API (PEP 384) #85283

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot mentioned this pull request Aug 28, 2023
[C API] Convert a few stdlib extensions to the limited C API (PEP 384) #85283
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Aug 28, 2023
erlend-aasland
erlend-aasland reviewed Aug 29, 2023
View reviewed changes
Comment thread Modules/_stat.c Resolved

Uh oh!

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

@vstinner
vstinner force-pushed the limited_stat branch from e06a69a to d22aca8 Compare August 29, 2023 13:08
@vstinner

vstinner commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

I moved the Changelog entry to the Build category, and I documented the change in the Build Changes of What's New in Python 3.13. See #85283 (comment) about the choice of the Category.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland previously approved these changes Aug 29, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 29, 2023
@vstinner vstinner mentioned this pull request Aug 29, 2023
Py_TRACE_REFS is not compatible with Py_LIMITED_API #108634
Closed
2 tasks done
@vstinner
vstinner requested a review from corona10 as a code owner August 29, 2023 13:41
@erlend-aasland
erlend-aasland dismissed their stale review August 29, 2023 13:43

Hm, I'm not so sure about not building _stat if Py_TRACE_REFS are defined

@bedevere-bot bedevere-bot added awaiting review and removed awaiting merge labels Aug 29, 2023
@vstinner
vstinner force-pushed the limited_stat branch from 779532b to d22aca8 Compare August 29, 2023 14:47
@vstinner

vstinner commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

Hm, I'm not so sure about not building _stat if Py_TRACE_REFS are defined

It's a workaround for issue #108634: without this change, building Python with ./configure --with-trace-refs fails on this PR.

But. It's more complicated than expect: multiple tests fail when _stat extension is missing. See issue #108638.

I'm going to:

  • Merge this PR: build _stat with Py_LIMITED_API
  • Break TraceRefs buildbot for now
  • maybe: Fix tests with _stat is missing, issue Test suite expects _stat extension to be available #108638
  • maybe: don't build _stat on TraceRefs (to workaround Py_LIMITED_API issue)
  • or: fix TraceRefs for Py_LIMITED_API, issue Py_TRACE_REFS is not compatible with Py_LIMITED_API #108634
All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented Aug 29, 2023

Copy link
Copy Markdown
Member
  • Merge this PR: build _stat with Py_LIMITED_API
  • Break TraceRefs buildbot for now

Just to note a slight hesitation of intentionally breaking a buildbot &c, given that _stat works currently, and building with the limited C API is a "nice to have" rather than critical/urgent. Would it be better to fix test assumptions you referenced first, then revisit this PR?

A

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Aug 30, 2023

Copy link
Copy Markdown
Member Author

or: fix TraceRefs for Py_LIMITED_API, issue #108634

I wrote PR #108663 which makes Py_TRACE_REFS build compatible with the limited C API.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Aug 30, 2023
gh-108634: Py_TRACE_REFS uses a hash table #108663
Merged
serhiy-storchaka
serhiy-storchaka reviewed Aug 30, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka 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.

Caught a bug! PyModule_Add requires 3.13.

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@vstinner

vstinner commented Aug 30, 2023

Copy link
Copy Markdown
Member Author

Caught a bug! PyModule_Add requires 3.13.

Oh! Only on Windows, so I didn't notice locally on Linux.

I don't want to downgrade _stat.c by avoid the new nice PyModule_Add() just to set Py_LIMITED_API to the oldest Python version. There is no benefits here, since _stat is shipped with Python itself. Well, the only benefit would be to test the limited C API itself. But here, I prefer to stick to PyModule_Add(). So I reverted the Py_LIMITED_API version to 3.13.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka
serhiy-storchaka approved these changes Aug 31, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 31, 2023
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Break TraceRefs buildbot for now

This PR will no longer break TraceRefs. I merged PR #108663 which adds limited C API support to Py_TRACE_REFS build.

AA-Turner reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

@vstinner
pythongh-85283: _stat extension uses the limited C API
12b7a8e
The _stat C extension is now built with the limited C API.
@vstinner
vstinner force-pushed the limited_stat branch from 3eaa297 to 12b7a8e Compare August 31, 2023 16:38
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

I rebased my PR to solve a merge conflict (Doc/whatsnew/3.13.rst).

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

@erlend-aasland: Would you mind to review the latest version of the PR?

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Aug 31, 2023
gh-85283: _scproxy uses the limited C API #108572
Closed
gh-85283: _uuid uses the limited C API #108506
Closed
@vstinner vstinner mentioned this pull request Aug 31, 2023
gh-85283: _testmultiphase uses the limited C API #108501
Closed
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

On-going discussion about converting some stdlib C extensions to the limited C API: https://discuss.python.org/t/use-the-limited-c-api-for-some-of-our-stdlib-c-extensions/32465

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

@erlend-aasland: Would you mind to review the latest version of the PR?

Sorry, I prefer to still stay out of the C API discussions. From the linked discussion, it seems to me these changes are far from non-controversial; perhaps considered marking this as draft until consensus have been reached.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner marked this pull request as draft August 31, 2023 21:33
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 31, 2023
@vstinner

vstinner commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Ok, I marked my PR as a draft. I already marked the 3 other similar PRs as draft.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 13, 2023

Copy link
Copy Markdown
Member Author

I close this PR until a decision is taken on using the limited C API for a few Python stdlib extensions. Well, it's not that complicated to rewrite this PR (or reuse the patch of the closed PR).

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner closed this Sep 13, 2023
@vstinner
vstinner deleted the limited_stat branch September 13, 2023 16:20
@vstinner vstinner mentioned this pull request Oct 11, 2023
gh-85283: _stat extension now uses the limited C API #110711
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@erlend-aasland erlend-aasland erlend-aasland left review comments
@corona10 corona10 Awaiting requested review from corona10
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner will be requested when the pull request is marked ready for review AA-Turner is a code owner

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.

5 participants

@vstinner @AA-Turner @erlend-aasland @serhiy-storchaka @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.