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

Backport PyPy 3.9 support for pyo3 0.15 #2262

Merged
merged 27 commits into from Apr 14, 2022

Conversation

messense
Copy link
Member

Fixes #2260

@messense
Copy link
Member Author

messense commented Mar 31, 2022

Looks like we need to raise MSRV to 1.48?

Downgrading clap to 2.33 works.

@adamreichold
Copy link
Member

Looks like we need to raise MSRV to 1.48?

I think that would not be possible in the context of #2260 as this is about supporting old platforms like RHEL 6 using rustc 1.41?

@messense messense added the CI-no-fail-fast If one job fails, allow the rest to keep testing label Mar 31, 2022
@birkenfeld
Copy link
Member

Yes, MSRV should not change in patch releases.

src/callback.rs Outdated Show resolved Hide resolved
@messense
Copy link
Member Author

I tried to bless the ui tests on macOS, but it still failed on CI.

src/ffi/datetime.rs Outdated Show resolved Hide resolved
@messense messense linked an issue Mar 31, 2022 that may be closed by this pull request
@mejrs
Copy link
Member

mejrs commented Apr 12, 2022

All green 🎉

Thanks @messense for your work :) I think this is done now?

@alex
Copy link
Contributor

alex commented Apr 12, 2022

🎉 🎉 🎉

Would it be helpful for us to do a CI run on pyca/cryptography with this branch to verify everything looks good?

@mejrs
Copy link
Member

mejrs commented Apr 12, 2022

I was just about to ask 😄

Yes, that's a good idea 👍

@alex
Copy link
Contributor

alex commented Apr 12, 2022

Ok testing PR up at pyca/cryptography#7062

@alex
Copy link
Contributor

alex commented Apr 12, 2022

Looks like there's an issue with pypy3.7 7.3.9, several tests are failing with SystemError: Function returned a NULL result without setting an exception

@mejrs
Copy link
Member

mejrs commented Apr 12, 2022

It looks like these tests go through the datetime api 😬 Your CI uses PyPy 7.3.9, try pinning to pypy-3.7-v7.3.7

This may be a case of #2217

@alex
Copy link
Contributor

alex commented Apr 12, 2022

I don't think we use the pyo3/C-API for handling datetimes directly, AFAIK everywhere we process datetimes we do it like this: https://github.com/pyca/cryptography/blob/main/src/rust/src/x509/common.rs#L663

@mejrs
Copy link
Member

mejrs commented Apr 12, 2022

Uh, strange. I don't think this PR introduces any difference between PyPy 3.7 and PyPy 3.8 ..... 🤔

@davidhewitt, any ideas? 🤷

@alex
Copy link
Contributor

alex commented Apr 12, 2022

Confirmed that the tests pass on pypy3.7 7.3.7, but I'd like to understand what the regression on 7.3.9 is :-/

src/callback.rs Outdated
Comment on lines 247 to 263
#[cfg(all(PyPy, not(Py_3_8)))]
{
const PYPY_GOOD_VERSION: [u8; 3] = [7, 3, 8];
let version = py
.import("sys")?
.getattr("implementation")?
.getattr("version")?;
if version.compare(crate::types::PyTuple::new(py, &PYPY_GOOD_VERSION))?
== std::cmp::Ordering::Less
{
let warn = py.import("warnings")?.getattr("warn")?;
warn.call1((
"PyPy 3.7 versions older than 7.3.8 are known to have binary \
compatibility issues which may cause segfaults. Please upgrade.",
))?;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the one place where we're changing the behavior of the code on pypy3.7. I don't have an obvious reason this would cause a crash, but it does jump out to me that we're doing this import+operations on every single function invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sound expensive

Copy link
Member

Choose a reason for hiding this comment

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

...huh?

It looks like this piece was committed to main with #2217 but then picked into this branch with c18e60f but put in a different location...

Copy link
Member

@mejrs mejrs Apr 13, 2022

Choose a reason for hiding this comment

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

I don't have an obvious reason this would cause a crash,

I think it's most likely that this set an exception/warning when it shouldn't have. For example, if you raise a warning during __traverse__, you can segfault.

@alex
Copy link
Contributor

alex commented Apr 12, 2022

Alternatively... computers cursed. CI's now passing just fine on 7.3.9

@h-vetinari
Copy link

Confirmed that the tests pass on pypy3.7 7.3.7, but I'd like to understand what the regression on 7.3.9 is :-/

CC @mattip

@messense
Copy link
Member Author

Thanks! @mejrs

@davidhewitt
Copy link
Member

I've pushed a commit to support PyPy 3.7 v7.3.8+ instead of pinning to v7.3.7, because this was agreed as the correct solution to the binary compatibility issues in https://foss.heptapod.net/pypy/pypy/-/issues/3688#note_181074

@alex
Copy link
Contributor

alex commented Apr 13, 2022

I'll continue to bump the pyca/cryptography side for any changes here -- it's still green (I'm not going to leave more status updates unless that changes :-))

@davidhewitt
Copy link
Member

I think this is now good to go! Unless I hear otherwise, I'll put this live some time later today.

@davidhewitt davidhewitt merged commit a9f7259 into PyO3:release-0.15 Apr 14, 2022
mtremer pushed a commit to ipfire/ipfire-2.x that referenced this pull request Nov 11, 2022
- Updated from version 0.15.1 to 0.15.2
- Update of rootfile
- Changelog
   ## [0.15.2] - 2022-04-14
     ### Packaging
        - Backport of PyPy 3.9 support from PyO3 0.16. [#2262](PyO3/pyo3#2262)

Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast If one job fails, allow the rest to keep testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 0.15.2 with support for Pypy 3.9 and Python 3.6
8 participants