Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-85283: _stat extension uses the limited C API#108573

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
vstinner wants to merge1 commit intopython:mainfromvstinner:limited_stat

Conversation

vstinner
Copy link
Member

@vstinnervstinner commentedAug 28, 2023
edited by bedevere-bot
Loading

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

@vstinner
Copy link
MemberAuthor

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.

erlend-aasland
erlend-aasland previously approved these changesAug 29, 2023
@erlend-aaslanderlend-aasland dismissed theirstale reviewAugust 29, 2023 13:43

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

@vstinner
Copy link
MemberAuthor

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:

@AA-Turner
Copy link
Member

  • Merge this PR: build_stat withPy_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

@vstinner
Copy link
MemberAuthor

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.

Copy link
Member

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

erlend-aasland reacted with rocket emoji
@vstinner
Copy link
MemberAuthor

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 nicePyModule_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 toPyModule_Add(). So I reverted the Py_LIMITED_API version to 3.13.

@vstinner
Copy link
MemberAuthor

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

The _stat C extension is now built with the limited C API.
@vstinner
Copy link
MemberAuthor

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

@vstinner
Copy link
MemberAuthor

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

@vstinner
Copy link
MemberAuthor

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

@erlend-aasland
Copy link
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.

@vstinnervstinner marked this pull request as draftAugust 31, 2023 21:33
@vstinner
Copy link
MemberAuthor

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

@vstinner
Copy link
MemberAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@erlend-aaslanderlend-aaslanderlend-aasland left review comments

@corona10corona10Awaiting requested review from corona10

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@vstinner@AA-Turner@erlend-aasland@serhiy-storchaka@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp