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

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

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 28, 2023

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

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

erlend-aasland
erlend-aasland previously approved these changes Aug 29, 2023
@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

@vstinner
Copy link
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:

@AA-Turner
Copy link
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

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

Copy link
Member

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

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

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

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

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

@vstinner
Copy link
Member Author

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

@vstinner
Copy link
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

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

@vstinner vstinner marked this pull request as draft August 31, 2023 21:33
@vstinner
Copy link
Member Author

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

@vstinner
Copy link
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants