Skip to content

Conversation

alex
Copy link
Contributor

@alex alex commented Sep 7, 2020

This depends on PyO3/pyo3#1125

@alex alex marked this pull request as ready for review September 20, 2020 13:38
@alex
Copy link
Contributor Author

alex commented Sep 20, 2020

Now that that the abi3 PR is almost ready for pyo3, I think this is ready for review.

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.

Overall looks good to me. Is there some way we can test this in CI? I guess we may need to add a project which builds a limited api crate etc. (Or change one of the existing test crates to build to the limited api.)

Also I think we can't merge this until we've decided on the feature naming for pyo3.

Comment on lines +50 to +53
py_limited_api : bool
Same as `py_limited_api` on `setuptools.Extension`. Note that if you
set this to True, your extension must pass the appropriate feature
flags to pyo3 (ensuring that `abi3` feature is enabled).
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to wait to merge because there was talk of renaming the abi3 feature in pyo3. (Usually features should add to the lib, rather than abi3 which currently takes things away.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's reasonable.

@alex
Copy link
Contributor Author

alex commented Sep 23, 2020

It should be possible to test this by building a module following the instructions in PyO3/pyo3#1203 with one python version, and then loading it against another python version.

@davidhewitt
Copy link
Member

👍 sounds like we can achieve that by adding a new CI job which installs maybe Python 3.6 and 3.8 and does exactly as you say. Probably doesn't need any new example code in that case.

@alex
Copy link
Contributor Author

alex commented Sep 24, 2020

Agreed. Want me to add that as a part of this PR?

@davidhewitt
Copy link
Member

davidhewitt commented Sep 24, 2020

If you're willing to build that, would be great! (I was thinking I can take a stab eventually, though I'm still pretty worn out / chasing chores from having moved house last week...)

@alex
Copy link
Contributor Author

alex commented Sep 24, 2020 via email

@alex alex force-pushed the abi3 branch 6 times, most recently from 6acdc97 to 04b36d5 Compare September 26, 2020 15:05
@alex
Copy link
Contributor Author

alex commented Sep 26, 2020

Test failures are because the abi3 branch needs to be rebased on master to pick up some of the fixes.

@alex alex mentioned this pull request Sep 28, 2020
6 tasks
@alex alex force-pushed the abi3 branch 3 times, most recently from c1ede8d to ef635af Compare October 10, 2020 16:26
@alex
Copy link
Contributor Author

alex commented Oct 10, 2020

Ok with master merged into abi3 on pyo3 this is green. In terms of ordering, when do you want to review/merge this:

  • Now
  • after abi3 is merged into master
  • after a pyo3 release?

@davidhewitt
Copy link
Member

I'm pretty much happy with how this is as-is, thanks very much for all your effort on this! It's really cool to see the CI job build against one Python and then run it with another!

With regards to merging - because it changes the rust_with_cffi job to point at the abi3 branch I think I'd at least like to wait until the abi3 branch is merged and we can point it at master.

I think at that point it's mergeable and I'll leave an issue to update the examples to use pyo3 0.13 when it releases.

@alex
Copy link
Contributor Author

alex commented Oct 10, 2020 via email

@alex
Copy link
Contributor Author

alex commented Oct 27, 2020

Ok, now pointed at pyo3 master!

@davidhewitt
Copy link
Member

👍 thanks, will release shortly!

@davidhewitt davidhewitt merged commit cc3931b into PyO3:master Oct 30, 2020
@alex alex deleted the abi3 branch October 30, 2020 12:46
@alex
Copy link
Contributor Author

alex commented Oct 30, 2020

Thanks!

@davidhewitt
Copy link
Member

@alex 0.11.4 now released; sorry for the delay.

@alex
Copy link
Contributor Author

alex commented Nov 3, 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