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

Define missing import APIs #1475

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Conversation

indygreg
Copy link
Contributor

@indygreg indygreg commented Mar 6, 2021

I need some of these APIs for the pyembed crate (used as part of
PyOxidizer). I figured I might as well define them all.

All these APIs are limited.

I don't have a PyPy install with a shared library to test. We may need to disable these when targeting PyPy.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for adding these. Can they please be located in src/ffi/cpython/import.rs?

We try to match the cpython header structure as much as possible to make it easier to diff against upstream changes.

If you're willing to put the definitions as the same order as they currently are in the cpython header, that'd also be much appreciated.

}

#[cfg(not(Py_LIMITED_API))]
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

This extern block contains pub static mut so it's going to need #[cfg_attr(windows, link(name = "pythonXY"))]

@davidhewitt
Copy link
Member

Also can confirm that none of these APIs appear to exist for PyPy.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks. As David says, please consider placing some APIs in cpython mod. We're refactoring the FFI module in that way (#1289).

src/ffi/import.rs Outdated Show resolved Hide resolved
Comment on lines 54 to 57
#[allow(unused)]
pub fn PyImport_ExtendInittab(newtab: *mut _inittab) -> c_int;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I'm getting a compiler warning about PyImport_ExtendInittab() being unused, so I suppressed this. Unsure why. Maybe the presence of the pub static mut above triggers it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was it. I moved this to its own block 🤷‍♂️

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks! Just a final couple of tweaks needed before merging.

Strange, I can't reproduce the unused code warning if I remove those two attributes from the static mut variables...

src/ffi/cpython/import.rs Outdated Show resolved Hide resolved
src/ffi/cpython/import.rs Outdated Show resolved Hide resolved
src/ffi/cpython/import.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nw0 nw0 left a comment

Choose a reason for hiding this comment

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

I think the current comments cover everything in import.h from CPython master.

src/ffi/cpython/import.rs Show resolved Hide resolved
src/ffi/cpython/import.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Base automatically changed from master to main March 16, 2021 22:09
@davidhewitt davidhewitt added this to the 0.14 milestone Apr 13, 2021
@davidhewitt davidhewitt mentioned this pull request Apr 19, 2021
7 tasks
@davidhewitt
Copy link
Member

@indygreg thanks for pushing this PR along to this point; I'm going to take it over and push a commit shortly to finish it off as part of some tidy-ups I'm doing for the 0.14 release.

@davidhewitt davidhewitt merged commit 28ccef2 into PyO3:main Apr 22, 2021
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.

4 participants