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

Make setuptools-rust play nice with cffi #68

Merged
merged 2 commits into from
Aug 2, 2020

Conversation

alex
Copy link
Contributor

@alex alex commented Jul 27, 2020

The primary benefit of this is that it'll work when you have a single project using both setuptools-rust as well as cffi.

@alex alex changed the title Migrate this to use slightly more constrained monkeypatching [WIP] Migrate this to use slightly more constrained monkeypatching Jul 27, 2020
@alex alex force-pushed the monkey-patch-different branch from e777992 to ba9bd4c Compare July 27, 2020 17:24
@alex
Copy link
Contributor Author

alex commented Jul 27, 2020

(Apologies that this is a WIP mess! I'm having some trouble testing locally, so I'm relying on travis!)

@alex alex force-pushed the monkey-patch-different branch 4 times, most recently from 9f1ba74 to a7d5761 Compare July 27, 2020 20:32
@alex alex changed the title [WIP] Migrate this to use slightly more constrained monkeypatching Migrate this to use slightly more constrained monkeypatching Jul 27, 2020
@alex alex marked this pull request as ready for review July 27, 2020 20:43
@alex
Copy link
Contributor Author

alex commented Jul 27, 2020

👋

So the reason for this is that currently, if you use both cffi and setuptools-rust in the same setup() invocation, you have a sad time. See pyca/cryptography#5357 as an example that hits this.

This switches from the monkeypatching approach to using the same approach that cffi uses, which should play nice with other things that use the same strategy. It's a bit gross, but seems to work!

❤️ to @dstufft for explaining this to me.

We can't use maturin for the same reason: we have a large existing setup.py and we want to do a gradual migration.

@alex alex changed the title Migrate this to use slightly more constrained monkeypatching Make setuptools-rust play nice with cffi Jul 27, 2020
@alex
Copy link
Contributor Author

alex commented Jul 30, 2020

If there's anything I can be doing to help get this reviewed and landed, please don't hesitate to ask me to do more work :-)

@davidhewitt
Copy link
Member

As a pyo3 maintainer I'm not super familiar with the internals of setuptools-rust, though am happy to help you get this through.

It looks like all functionality is preserved with this PR, though what is not clear to me is whether this is a breaking change to public API of setuptools-rust. Can you elaborate on this?

Also, as this PR is intended to add compatibility with cffi, it would be great to add an example / test which uses both together in the same project and proves it works in CI.

@alex
Copy link
Contributor Author

alex commented Aug 1, 2020

a) I don't think this changes anything, at least as used by normal setup.py. If people were relying on importing these classes for some reason, I guess it potentially breaks them?
b) Yes, good call, I'll write an example/test.

The primary benefit of this is that it'll work when you have a single project using both setuptools-rust as well as cffi.
@alex
Copy link
Contributor Author

alex commented Aug 1, 2020

Question: Would it be ok to add some tiny CFFI usage to the html-py-ever module, or woiuld you prefer it be an entirely separate example?

@alex alex force-pushed the monkey-patch-different branch 4 times, most recently from b2bf400 to 739ce95 Compare August 1, 2020 17:47
@alex alex force-pushed the monkey-patch-different branch from 739ce95 to 215ce76 Compare August 1, 2020 18:01
@alex
Copy link
Contributor Author

alex commented Aug 1, 2020

Took me a few try to get .travis.yml happy, but we now have tests. Huzzah!

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.

This is brilliant; thanks very much!

I'm writing a PR this afternoon to update the release process for setuptools-rust to use github actions, once I've got that sorted I'll put out a new setuptools_rust version.

@davidhewitt davidhewitt merged commit 5fa909d into PyO3:master Aug 2, 2020
@alex alex deleted the monkey-patch-different branch August 2, 2020 14:47
@alex
Copy link
Contributor Author

alex commented Aug 2, 2020

🎉 thanks!

@davidhewitt
Copy link
Member

(Just an apology to note that I had too many things I was making progress with this afternoon and didn't get around to the release process PR. It's on my list of things to make time for in the week ahead. Sorry for the delay.)

@alex
Copy link
Contributor Author

alex commented Aug 2, 2020 via email

@davidhewitt
Copy link
Member

0.11.0 is now released and should be good for you to use!

@alex
Copy link
Contributor Author

alex commented Aug 6, 2020 via email

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.

2 participants