Avoid exporting incorrect PyInit_* symbols
#12889
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using the `#[pymodule]` derive macro in PyO3 0.21 always causes a `PyInit_*` symbol with a matching name to be exported in the output `cdylib`. This is required for the top-level module, in order for Python to import it---it needs to know which symbol in a shared library file it should call---but submodules must be manually initialised, so do not need it. Including it is typically harmless (and something we've been doing for a long time), but it is technically against the coding rules for CPython extensions[^1]. Recent versions of `abi3audit` (0.0.11+) have tightened their symbol checkers to better match the CPython guidelines, which causes our wheels to be rejected by their audits. This is, in theory, not a break of abi3 because CPython could never introduce an API-elvel `PyInit_*` function themselves without causing problems, so there ought to be no problems for our users, even with future Python versions. That said, we still want to pass the audit, because the coding guidelines are useful. This commit is not the cleanest way of doing things. PyO3 0.22 includes a `#[pymodule(submodule)]` option on the attribute macro, which lets us use all the standard code generation while suppressing the unnecessary `PyInit_*` symbol. When we are ready to move to PyO3 0.22, we probably want to revert this commit to switch to that form. [^1]: https://docs.python.org/3/c-api/intro.html
jakelishman
added
type: qa
jakelishman
requested review from
alexanderivrii,
ShellyGarion and
a team
as code owners
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10214478341Details
💛 - Coveralls |
|
We'll probably need to do this on |
|
Yeah, I can open another PR to do that there. It should be a smaller job. |
mtreinish
approved these changes
Aug 2, 2024
|
Fwiw, what I did to test was to pull this commit into a Linux VM (needs to be Linux, since |
mergify bot
pushed a commit
that referenced
this pull request
Aug 2, 2024
Using the `#[pymodule]` derive macro in PyO3 0.21 always causes a `PyInit_*` symbol with a matching name to be exported in the output `cdylib`. This is required for the top-level module, in order for Python to import it---it needs to know which symbol in a shared library file it should call---but submodules must be manually initialised, so do not need it. Including it is typically harmless (and something we've been doing for a long time), but it is technically against the coding rules for CPython extensions[^1]. Recent versions of `abi3audit` (0.0.11+) have tightened their symbol checkers to better match the CPython guidelines, which causes our wheels to be rejected by their audits. This is, in theory, not a break of abi3 because CPython could never introduce an API-elvel `PyInit_*` function themselves without causing problems, so there ought to be no problems for our users, even with future Python versions. That said, we still want to pass the audit, because the coding guidelines are useful. This commit is not the cleanest way of doing things. PyO3 0.22 includes a `#[pymodule(submodule)]` option on the attribute macro, which lets us use all the standard code generation while suppressing the unnecessary `PyInit_*` symbol. When we are ready to move to PyO3 0.22, we probably want to revert this commit to switch to that form. [^1]: https://docs.python.org/3/c-api/intro.html (cherry picked from commit 120b73d) # Conflicts: # crates/accelerate/src/target_transpiler/mod.rs # crates/pyext/src/lib.rs
jakelishman
added a commit
that referenced
this pull request
Aug 2, 2024
Using the `#[pymodule]` derive macro in PyO3 0.21 always causes a `PyInit_*` symbol with a matching name to be exported in the output `cdylib`. This is required for the top-level module, in order for Python to import it---it needs to know which symbol in a shared library file it should call---but submodules must be manually initialised, so do not need it. Including it is typically harmless (and something we've been doing for a long time), but it is technically against the coding rules for CPython extensions[^1]. Recent versions of `abi3audit` (0.0.11+) have tightened their symbol checkers to better match the CPython guidelines, which causes our wheels to be rejected by their audits. This is, in theory, not a break of abi3 because CPython could never introduce an API-elvel `PyInit_*` function themselves without causing problems, so there ought to be no problems for our users, even with future Python versions. That said, we still want to pass the audit, because the coding guidelines are useful. This commit is not the cleanest way of doing things. PyO3 0.22 includes a `#[pymodule(submodule)]` option on the attribute macro, which lets us use all the standard code generation while suppressing the unnecessary `PyInit_*` symbol. When we are ready to move to PyO3 0.22, we probably want to revert this commit to switch to that form. [^1]: https://docs.python.org/3/c-api/intro.html (cherry picked from commit 120b73d)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 2, 2024
Using the `#[pymodule]` derive macro in PyO3 0.21 always causes a `PyInit_*` symbol with a matching name to be exported in the output `cdylib`. This is required for the top-level module, in order for Python to import it---it needs to know which symbol in a shared library file it should call---but submodules must be manually initialised, so do not need it. Including it is typically harmless (and something we've been doing for a long time), but it is technically against the coding rules for CPython extensions[^1]. Recent versions of `abi3audit` (0.0.11+) have tightened their symbol checkers to better match the CPython guidelines, which causes our wheels to be rejected by their audits. This is, in theory, not a break of abi3 because CPython could never introduce an API-elvel `PyInit_*` function themselves without causing problems, so there ought to be no problems for our users, even with future Python versions. That said, we still want to pass the audit, because the coding guidelines are useful. This commit is not the cleanest way of doing things. PyO3 0.22 includes a `#[pymodule(submodule)]` option on the attribute macro, which lets us use all the standard code generation while suppressing the unnecessary `PyInit_*` symbol. When we are ready to move to PyO3 0.22, we probably want to revert this commit to switch to that form. [^1]: https://docs.python.org/3/c-api/intro.html (cherry picked from commit 120b73d) Co-authored-by: Jake Lishman <[email protected]>
eliarbel
added a commit
to eliarbel/qiskit
that referenced
this pull request
Sep 18, 2024
This is a manual backport of the change introduced in PR Qiskit#12889. It is done manually since in 0.46 we are using PyO3 version 0.19.2 which does not yet have the `Bound` concept PR Qiskit#12889 relies on in the `add_submodule` helper function. In this branch we also need to handle just a subset of submodule functions compared to the ones in Qiskit#12899.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 20, 2024
This is a manual backport of the change introduced in PR #12889. It is done manually since in 0.46 we are using PyO3 version 0.19.2 which does not yet have the `Bound` concept PR #12889 relies on in the `add_submodule` helper function. In this branch we also need to handle just a subset of submodule functions compared to the ones in #12899.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Using the
#[pymodule]derive macro in PyO3 0.21 always causes aPyInit_*symbol with a matching name to be exported in the outputcdylib. This is required for the top-level module, in order for Python to import it---it needs to know which symbol in a shared library file it should call---but submodules must be manually initialised, so do not need it. Including it is typically harmless (and something we've been doing for a long time), but it is technically against the coding rules for CPython extensions1.Recent versions of
abi3audit(0.0.11+) have tightened their symbol checkers to better match the CPython guidelines, which causes our wheels to be rejected by their audits. This is, in theory, not a break of abi3 because CPython could never introduce an API-elvelPyInit_*function themselves without causing problems, so there ought to be no problems for our users, even with future Python versions. That said, we still want to pass the audit, because the coding guidelines are useful.This commit is not the cleanest way of doing things. PyO3 0.22 includes a
#[pymodule(submodule)]option on the attribute macro, which lets us use all the standard code generation while suppressing the unnecessaryPyInit_*symbol. When we are ready to move to PyO3 0.22, we probably want to revert this commit to switch to that form.Details and comments
We found this during the deployment of 1.2.0rc1, since that's the most recent PyPI release since
abi3auditreleased version 0.0.11. This commit creates a wheel that passes the audit at its current version 0.0.14.Footnotes
https://docs.python.org/3/c-api/intro.html ↩