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

correct ffi definition of PyIter_Check #2914

Merged
merged 2 commits into from Feb 3, 2023
Merged

Conversation

davidhewitt
Copy link
Member

Closes #2913

It looks like what is happening is that PyO3 was relying on an outdated macro form of PyIter_Check which is now a CPython implementation detail, which would explain why it was behaving inconsistently on different platforms (likely due to differences in linkers / implementations).

The test I've pushed succeeds, but fails to compile due to a hygiene bug! I'm done for tonight so I'll take a look at that soon and then rebase this after.

bors bot added a commit that referenced this pull request Jan 27, 2023
2873: A new example that shows how to integrate Python plugins that use pyclasses into a Rust app r=davidhewitt a=alexpyattaev

Example showing integration of a Python plugin into a Rust app while having option to test pyclass based API without the main app.  This also illustrates some aspects related to import of Python modules into a Rust app while also having an API module available for the Python code to be able to produce Rust objects. 

CI seems to fail on my local machine for reasons unrelated to the example just added:
```
error: unused macro definition: `check_struct`
  --> pyo3-ffi-check/src/main.rs:13:18
   |
13 |     macro_rules! check_struct {
   |                  ^^^^^^^^^^^^
   |
```

2889: Added support for PyErr_WriteUnraisable r=davidhewitt a=mitsuhiko

Fixes #2884

2923: hygiene: fix `#[pymethods(crate = "...")]` r=davidhewitt a=davidhewitt

Got to the bottom of the hygiene issue in test of #2914 

Turns out that `#[pymethods] #[pyo3(crate = "...")]` works, but `#[pymethods(crate = "...")]` was ignoring the argument.

Added a tweak to fix this and a snippet in the hygiene test (which fails on `main`). 

2924: remove unneeded into_iter calls r=davidhewitt a=davidhewitt

Clippy complaining about these to me this morning locally.

Co-authored-by: Alex Pyattaev <alex.pyattaev@gmail.com>
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Co-authored-by: Armin Ronacher <armin.ronacher@active-4.com>
bors bot added a commit that referenced this pull request Jan 27, 2023
2923: hygiene: fix `#[pymethods(crate = "...")]` r=davidhewitt a=davidhewitt

Got to the bottom of the hygiene issue in test of #2914 

Turns out that `#[pymethods] #[pyo3(crate = "...")]` works, but `#[pymethods(crate = "...")]` was ignoring the argument.

Added a tweak to fix this and a snippet in the hygiene test (which fails on `main`). 

2924: remove unneeded into_iter calls r=davidhewitt a=davidhewitt

Clippy complaining about these to me this morning locally.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@davidhewitt davidhewitt force-pushed the fix-iter-check branch 2 times, most recently from 936bfec to b010c9d Compare January 27, 2023 21:10
@davidhewitt
Copy link
Member Author

Rebased & tests adjusted.

@adamreichold
Copy link
Member

@davidhewitt This (like all other PR) will fail the MSRV build due to trybuild bump MSRV (via basic-toml) in a point release. I think we need to pin it to 1.0.76...

@adamreichold
Copy link
Member

@davidhewitt This (like all other PR) will fail the MSRV build due to trybuild bump MSRV (via basic-toml) in a point release. I think we need to pin it to 1.0.76...

I took the liberty of rebasing this onto the current main branch which contains that version pin.

@davidhewitt
Copy link
Member Author

Thanks! It would have been ok to just leave that to bors too :)

bors r+

bors bot added a commit that referenced this pull request Jan 28, 2023
2914: correct ffi definition of PyIter_Check r=davidhewitt a=davidhewitt

Closes #2913 

It looks like what is happening is that PyO3 was relying on an outdated macro form of `PyIter_Check` which is now a CPython implementation detail, which would explain why it was behaving inconsistently on different platforms (likely due to differences in linkers / implementations).

The test I've pushed succeeds, but fails to compile due to a hygiene bug! I'm done for tonight so I'll take a look at that soon and then rebase this after.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 28, 2023

Build failed:

@davidhewitt
Copy link
Member Author

Looks like I fluffed the #[cfg] on the test. Think it's fixed now.

@davidhewitt
Copy link
Member Author

bors retry

bors bot added a commit that referenced this pull request Jan 29, 2023
2914: correct ffi definition of PyIter_Check r=davidhewitt a=davidhewitt

Closes #2913 

It looks like what is happening is that PyO3 was relying on an outdated macro form of `PyIter_Check` which is now a CPython implementation detail, which would explain why it was behaving inconsistently on different platforms (likely due to differences in linkers / implementations).

The test I've pushed succeeds, but fails to compile due to a hygiene bug! I'm done for tonight so I'll take a look at that soon and then rebase this after.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 29, 2023

Build failed:

@davidhewitt
Copy link
Member Author

Hmm. It looks like this night have always been broken on Windows.

We might just have to remove this functionality on Python 3.7 (which only has this broken macro form, the function form of PyIter_Check was added in 3.8).

@samuelcolvin samuelcolvin mentioned this pull request Jan 31, 2023
@davidhewitt
Copy link
Member Author

I've pushed a commit which removes PyIter_Check from 3.7, thus removing ability to .downcast() to PyIterator on 3.7 or accept it as a function argument.

This is a bit unfortunate, but it's broken and I can't see a way to fix. I think this is better than the alternative of leaving this defined but broken on CPython 3.7 on Windows.

I'd welcome feedback here, otherwise I'll merge this sometime ahead of the 0.18.1 release early next week.

@samuelcolvin
Copy link
Contributor

Is much prefer PyIterator to work in 3.7, at least in Linux and macos.

Otherwise we'd have to drop 3.7 support from pydantic v2, and I've promised to support all python versions that are still alive.

"This thing is broken on windows on 3.7" is fine, but "we don't support 3.7" will annoy a lot of people.

@birkenfeld
Copy link
Member

Why not do a getattr(obj.__class__, "__next__") under the conditions where PyIter_Check is broken? It's slow, but should work?

@samuelcolvin
Copy link
Contributor

According to https://pypistats.org/packages/pydantic 3.7 is still the second most common version with 25% of downloads.

Screenshot_20230203-072020

@davidhewitt
Copy link
Member Author

Why not do a getattr(obj.__class__, "__next__") under the conditions where PyIter_Check is broken? It's slow, but should work?

Good idea - I'll change to that (and I'll use it in ffi::PyIter_Check too).

@davidhewitt
Copy link
Member Author

Ok, that's pushed - let's see if it works on CI.

@davidhewitt
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Feb 3, 2023
@bors
Copy link
Contributor

bors bot commented Feb 3, 2023

try

Build succeeded:

@davidhewitt
Copy link
Member Author

That's a success! Thanks all for reviews & design assistance here.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 3, 2023

Build succeeded:

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.

py_any.downcast::<PyIterator>() behaviour is different on windows vs macos and linux
4 participants