-
Notifications
You must be signed in to change notification settings - Fork 887
fixes #5202 -- treat the PYO3_BUILD_EXTENSION_MODULE
env var the same as the extension-module feature
#5343
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
Conversation
bfef190
to
702e5ca
Compare
I couldn't see a great way to write a test case for this, so my plan is to send PRs to maturin and setuptools-rust to respect this env var, at which point once they're in a release it'll be easy to test. |
maturin: PyO3/maturin#2723 |
setuptools-rust: PyO3/setuptools-rust#540 |
PYO3_BUILD_EXTENSION_MODULE
env var the same as teh extension-module featurePYO3_BUILD_EXTENSION_MODULE
env var the same as the extension-module feature
@davidhewitt any chance we can sneak this in to 0.26? |
I think we can probably test the basics within this repo by checking the shared objects produced by the I will do my best to give this a full review tonight, agreed it would be nice to put this in 0.26 so that maturin / setuptools-rust can add support. |
Hmm, so the problem is that until this is in a maturin release (the PR was merged there), setting the env var doesn't do anything :-) Once the maturin release is out I can definitely update the test. Do we have any existing extension module tests that don't go through either maturin or rust-openssl? |
@alex Feel free to raise a PR in maturin to release a new patch version to unblock this. (or I can do it after work today.) Thanks! |
Almost bed time here, if you're able to get to it that'd be great, thank you! |
702e5ca
to
bfbb4fe
Compare
I'm very confused:
|
Looks like only 3.14 is segfaulting, is this a known issue? I don't see how this PR could possibly cause this (which isn't to say it doesn't...) |
Try merging main, for reasons I haven't quite followed the CI plan is coming from the merge commit even though the fix for 3.14 is not on your branch. |
…e same as teh extension-module feature
bfbb4fe
to
66e044a
Compare
Easy enough! Thanks |
Phew, green now -- with test! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ship this 👍
No description provided.