◐ Shell
reader mode source ↗
Skip to content

gh-106104: Use public weakref APIs in sqlite3#106105

Closed
erlend-aasland wants to merge 2 commits into
python:mainfrom
erlend-aasland:sqlite-weakref-public-api
Closed

gh-106104: Use public weakref APIs in sqlite3#106105
erlend-aasland wants to merge 2 commits into
python:mainfrom
erlend-aasland:sqlite-weakref-public-api

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jun 26, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

The sqlite3 extension module should be built using the public API, so
we're removing the Py_BUILD_CORE_MODULE define that was introduced as
part of the gh-105927 changes.

@erlend-aasland
pythongh-106104: Use public weakref APIs in sqlite3 …
43cbf83 
The sqlite3 extension module should be built using the public API, so
we're removing the Py_BUILD_CORE_MODULE define that was introduced as
part of the pythongh-105927 changes.
@erlend-aasland erlend-aasland requested a review from vstinner June 26, 2023 13:03
@bedevere-bot bedevere-bot mentioned this pull request Jun 26, 2023
Closed
@erlend-aasland erlend-aasland linked an issue Jun 26, 2023 that may be closed by this pull request
The sqlite3 module should not be built as a core module #106104
Closed
vstinner
vstinner reviewed Jun 27, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Is it an issue to use the internal C API?

@erlend-aasland

erlend-aasland commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Is it an issue to use the internal C API?

Not a big issue, but historically, the sqlite3 extension module has been implemented (mostly) using public API. See also:

@erlend-aasland

erlend-aasland commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Related: I see that _ssl also accidentally got Py_BUILD_CORE_MODULE defined in 46a3190 ;)

@vstinner

vstinner commented Jun 27, 2023

Copy link
Copy Markdown
Member

Related: I see that _ssl also accidentally got Py_BUILD_CORE_MODULE defined in 46a3190 ;)

How is it an issue to use the internal C API? The internal C API is written for best performances thanks to inlining when you control the argument types.

vstinner
vstinner approved these changes Jul 12, 2023
View reviewed changes

@vstinner vstinner left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM.

Well, if you don't want to use the internal C API in sqlite3, yep, the change is good. But warning: I made other sqlite3 changes in the meanwhile to use more internal APIs!?

@erlend-aasland

erlend-aasland commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

Related: I see that _ssl also accidentally got Py_BUILD_CORE_MODULE defined in 46a3190 ;)

How is it an issue to use the internal C API? The internal C API is written for best performances thanks to inlining when you control the argument types.

Well, I'll let you reply yourself (in #88140 (comment)) 😄:

I would prefer to limit the usage of the internal C API in extension modules built as dynamic libraries. See bpo-41111: "[C API] Convert a few stdlib extensions to the limited C API (PEP-384)".

@vstinner

vstinner commented Jul 28, 2023

Copy link
Copy Markdown
Member

I had great hope in being able to convert some stdlib extensions to the limited C API, but then i fall into Argument Clinic and I lost faith. I mean that I found more urgent things to do.

If someone is volunteer to attempt that again, I'm very supportive.

Maybe we might try to define some embargo on abusing private and internal APIs in Python stdlib extensions 😁 It should be the exception, not the default. I'm not sure.

@vstinner

vstinner commented Oct 17, 2023

Copy link
Copy Markdown
Member

Hi, what's the status of this PR?

About converting some stdlib C extensions to the limited C API, you can see: #85283

@erlend-aasland

erlend-aasland commented Oct 22, 2023

Copy link
Copy Markdown
Contributor Author

I don't plan to follow this up right now. If we decide to convert _sqlite3 to use the Limited API in the future, we can revisit this.

@erlend-aasland erlend-aasland deleted the sqlite-weakref-public-api branch October 22, 2023 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The sqlite3 module should not be built as a core module

3 participants

@erlend-aasland @vstinner @bedevere-bot