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

Avoid exporting incorrect PyInit_* symbols (backport #12889) #12893

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Aug 2, 2024

Summary

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

Details and comments

We found this during the deployment of 1.2.0rc1, since that's the most recent PyPI release since abi3audit released version 0.0.11. This commit creates a wheel that passes the audit at its current version 0.0.14.


This is an automatic backport of pull request #12889 done by Mergify.

Footnotes

  1. https://docs.python.org/3/c-api/intro.html

@mergify mergify bot requested a review from alexanderivrii as a code owner August 2, 2024 14:50
@mergify mergify bot added the conflicts used by mergify when there are conflicts in a port label Aug 2, 2024
@mergify mergify bot requested review from ShellyGarion and a team as code owners August 2, 2024 14:50
Copy link
Contributor Author

mergify bot commented Aug 2, 2024

Cherry-pick of 120b73d has failed:

On branch mergify/bp/stable/1.2/pr-12889
Your branch is up to date with 'origin/stable/1.2'.

You are currently cherry-picking commit 120b73d21.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   crates/accelerate/src/convert_2q_block_matrix.rs
	modified:   crates/accelerate/src/dense_layout.rs
	modified:   crates/accelerate/src/error_map.rs
	modified:   crates/accelerate/src/euler_one_qubit_decomposer.rs
	modified:   crates/accelerate/src/isometry.rs
	modified:   crates/accelerate/src/nlayout.rs
	modified:   crates/accelerate/src/optimize_1q_gates.rs
	modified:   crates/accelerate/src/pauli_exp_val.rs
	modified:   crates/accelerate/src/results/mod.rs
	modified:   crates/accelerate/src/sabre/mod.rs
	modified:   crates/accelerate/src/sampled_exp_val.rs
	modified:   crates/accelerate/src/sparse_pauli_op.rs
	modified:   crates/accelerate/src/star_prerouting.rs
	modified:   crates/accelerate/src/stochastic_swap.rs
	modified:   crates/accelerate/src/synthesis/clifford/mod.rs
	modified:   crates/accelerate/src/synthesis/linear/mod.rs
	modified:   crates/accelerate/src/synthesis/mod.rs
	modified:   crates/accelerate/src/synthesis/permutation/mod.rs
	modified:   crates/accelerate/src/two_qubit_decompose.rs
	modified:   crates/accelerate/src/uc_gate.rs
	modified:   crates/accelerate/src/utils.rs
	modified:   crates/accelerate/src/vf2_layout.rs
	modified:   crates/circuit/src/lib.rs
	modified:   crates/qasm2/src/lib.rs
	modified:   crates/qasm3/src/lib.rs

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   crates/accelerate/src/target_transpiler/mod.rs
	both modified:   crates/pyext/src/lib.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @kevinhartman
  • @levbishop
  • @mtreinish

@github-actions github-actions bot added type: qa Issues and PRs that relate to testing and code quality priority: high Changelog: None Do not include in changelog labels Aug 2, 2024
@github-actions github-actions bot added this to the 1.2.0 milestone 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)
@jakelishman jakelishman force-pushed the mergify/bp/stable/1.2/pr-12889 branch from 818c785 to 266ed8f Compare August 2, 2024 16:24
@jakelishman jakelishman removed the conflicts used by mergify when there are conflicts in a port label Aug 2, 2024
@jakelishman jakelishman enabled auto-merge August 2, 2024 16:25
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10218833747

Details

  • 40 of 40 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on mergify/bp/stable/1.2/pr-12889 at 89.972%

Totals Coverage Status
Change from base Build 10218811685: 90.0%
Covered Lines: 66742
Relevant Lines: 74181

💛 - Coveralls

@jakelishman jakelishman added this pull request to the merge queue Aug 2, 2024
Merged via the queue into stable/1.2 with commit 5c19652 Aug 2, 2024
17 checks passed
@mergify mergify bot deleted the mergify/bp/stable/1.2/pr-12889 branch August 2, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog priority: high type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants