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

Update to pyo3-0.23 #11954

Merged
merged 9 commits into from
Nov 15, 2024
Merged

Update to pyo3-0.23 #11954

merged 9 commits into from
Nov 15, 2024

Conversation

ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Nov 15, 2024

No description provided.

@ngoldbaum ngoldbaum mentioned this pull request Nov 15, 2024
@alex
Copy link
Member

alex commented Nov 15, 2024

Is tehre a reason not to push a change to cargo.toml to point at git? Or are there patches on top of github that are still needed?

@alex
Copy link
Member

alex commented Nov 15, 2024

With sadness, you'll need to rebase on main, we moved the pyo3 dep to be a workspace one.

@ngoldbaum
Copy link
Contributor Author

Is tehre a reason not to push a change to cargo.toml to point at git? Or are there patches on top of github that are still needed?

Nope no particular reason, I just had it pointed at a local clone with path. Pointing it at github seems fine for now.

@alex
Copy link
Member

alex commented Nov 15, 2024

this should fix it

diff --git a/src/rust/src/x509/certificate.rs b/src/rust/src/x509/certificate.rs
index 5758acccb..66afa08f1 100644
--- a/src/rust/src/x509/certificate.rs
+++ b/src/rust/src/x509/certificate.rs
@@ -716,7 +716,7 @@ pub(crate) fn parse_access_descriptions(
 
 fn parse_naming_authority<'p>(
     py: pyo3::Python<'p>,
-    authority: NamingAuthority<'p>,
+    authority: NamingAuthority<'_>,
 ) -> CryptographyResult<pyo3::Bound<'p, pyo3::PyAny>> {
     let py_id = match &authority.id {
         Some(data) => oid_to_py_oid(py, data)?,
@@ -736,10 +736,10 @@ fn parse_naming_authority<'p>(
         .call1((py_id, py_url, py_text))?)
 }
 
-fn parse_profession_infos<'a>(
-    py: pyo3::Python<'a>,
+fn parse_profession_infos<'p, 'a>(
+    py: pyo3::Python<'p>,
     profession_infos: &asn1::SequenceOf<'a, ProfessionInfo<'a>>,
-) -> CryptographyResult<pyo3::Bound<'a, pyo3::PyAny>> {
+) -> CryptographyResult<pyo3::Bound<'p, pyo3::PyAny>> {
     let py_infos = pyo3::types::PyList::empty(py);
     for info in profession_infos.clone() {
         let py_naming_authority = match info.naming_authority {
@@ -782,10 +782,10 @@ fn parse_profession_infos<'a>(
     Ok(py_infos.into_any())
 }
 
-fn parse_admissions<'a>(
-    py: pyo3::Python<'a>,
+fn parse_admissions<'p, 'a>(
+    py: pyo3::Python<'p>,
     admissions: &asn1::SequenceOf<'a, Admission<'a>>,
-) -> CryptographyResult<pyo3::Bound<'a, pyo3::PyAny>> {
+) -> CryptographyResult<pyo3::Bound<'p, pyo3::PyAny>> {
     let py_admissions = pyo3::types::PyList::empty(py);
     for admission in admissions.clone() {
         let py_admission_authority = match admission.admission_authority {

@ngoldbaum
Copy link
Contributor Author

woohoo, tests pass!

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

Sorry for the zillion duplicate comments, just wanted to be able to quickly see if they were resolved.

Thanks so much for your work on this!

@@ -34,8 +34,8 @@ fn curve_from_py_curve(
if !py_curve.is_instance(&types::ELLIPTIC_CURVE.get(py)?)? {
if allow_curve_class {
let warning_cls = types::DEPRECATED_IN_42.get(py)?;
let warning_msg = "Curve argument must be an instance of an EllipticCurve class. Did you pass a class by mistake? This will be an exception in a future version of cryptography.";
pyo3::PyErr::warn_bound(py, &warning_cls, warning_msg, 1)?;
let warning_msg = std::ffi::CString::new("Curve argument must be an instance of an EllipticCurve class. Did you pass a class by mistake? This will be an exception in a future version of cryptography.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

We can use CStr::from_bytes_with_nul and save an allocation here

"Properties that return a naïve datetime object have been deprecated. Please switch to not_valid_before_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to not_valid_before_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Same

"Properties that return a naïve datetime object have been deprecated. Please switch to not_valid_after_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to not_valid_after_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Same

"Parsed a negative serial number, which is disallowed by RFC 5280. Loading this certificate will cause an exception in the next release of cryptography.",
1,
)?;
let message = std::ffi::CString::new("Parsed a negative serial number, which is disallowed by RFC 5280. Loading this certificate will cause an exception in the next release of cryptography.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Same

"The parsed certificate contains a NULL parameter value in its signature algorithm parameters. This is invalid and will be rejected in a future version of cryptography. If this certificate was created via Java, please upgrade to JDK21+ or the latest JDK11/17 once a fix is issued. If this certificate was created in some other fashion please report the issue to the cryptography issue tracker. See https://github.com/pyca/cryptography/issues/8996 and https://github.com/pyca/cryptography/issues/9253 for more details.",
2,
)?;
let message = std::ffi::CString::new("The parsed certificate contains a NULL parameter value in its signature algorithm parameters. This is invalid and will be rejected in a future version of cryptography. If this certificate was created via Java, please upgrade to JDK21+ or the latest JDK11/17 once a fix is issued. If this certificate was created in some other fashion please report the issue to the cryptography issue tracker. See https://github.com/pyca/cryptography/issues/8996 and https://github.com/pyca/cryptography/issues/9253 for more details.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Same

"Properties that return a naïve datetime object have been deprecated. Please switch to this_update_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to this_update_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

same

"Properties that return a naïve datetime object have been deprecated. Please switch to next_update_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to next_update_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

same

"Properties that return a naïve datetime object have been deprecated. Please switch to revocation_time_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to revocation_time_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

same

"Properties that return a naïve datetime object have been deprecated. Please switch to this_update_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to revocation_time_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

same

"Properties that return a naïve datetime object have been deprecated. Please switch to next_update_utc.",
1,
)?;
let message = std::ffi::CString::new("Properties that return a naïve datetime object have been deprecated. Please switch to next_update_utc.").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

same

@alex
Copy link
Member

alex commented Nov 15, 2024

Release is up! You should be able to point this at 0.23 on crates.io now

src/rust/src/lib.rs Outdated Show resolved Hide resolved
@ngoldbaum
Copy link
Contributor Author

Sorry for all the noise, I was getting confused by the nox sessions before local passing.

src/rust/src/backend/ec.rs Outdated Show resolved Hide resolved
@ngoldbaum ngoldbaum changed the title WIP: Update to pyo3-0.23 Update to pyo3-0.23 Nov 15, 2024
@ngoldbaum ngoldbaum marked this pull request as ready for review November 15, 2024 20:26
@alex alex linked an issue Nov 15, 2024 that may be closed by this pull request
@alex alex merged commit f137596 into pyca:main Nov 15, 2024
60 checks passed
@reaperhulk
Copy link
Member

That was quick. Awesome work @ngoldbaum

@ngoldbaum
Copy link
Contributor Author

Getting free-threading working will be harder. I want to see whether I can disable the cffi dependency and make some headway while work on free-threading in cffi proceeds.

@alex
Copy link
Member

alex commented Nov 16, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support PyO3 0.23
3 participants