Skip to content

Fix Rust 1.82 compilation errors by upgrading PyO3 from 0.18 to 0.20 #4146

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

Merged
merged 6 commits into from
Jun 3, 2025

Conversation

drganjoo
Copy link
Contributor

@drganjoo drganjoo commented May 22, 2025

PyO3 Upgrade to Fix Compatibility with Rust 1.82

This PR addresses a compatibility issue between PyO3 0.18 and Rust 1.82, which causes compilation errors due to unexpected cfg condition names.

When compiled with Rust 1.82, the current PyO3 0.18 dependency produces errors like:

error: unexpected `cfg` condition name: `addr_of`

This error occurs because of changes in how Rust handles conditional configuration checks in newer compiler versions.

Changes in this PR:

  1. Upgrades PyO3 from version 0.18 to 0.20 (the latest version compatible with pyo3-asyncio)
  2. Updates generated functions to follow the required API pattern where required fields precede optional ones
  3. API changes in pyo3, whereby in the new version dictionary.get_key returns a Result<Option> instead of Result in the older version.

This upgrade ensures compatibility with Rust 1.82 while maintaining all existing functionality.

Testing

  1. Current protocol tests pass
  2. An additional test has been included for required versus optional parameters.

@drganjoo drganjoo requested review from a team as code owners May 22, 2025 12:52
@drganjoo drganjoo marked this pull request as draft May 22, 2025 12:52
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@drganjoo drganjoo force-pushed the fahadzub/pyo3-upgrade branch from 26d68fb to 2ab329d Compare May 22, 2025 13:30
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@drganjoo drganjoo marked this pull request as ready for review May 23, 2025 11:16
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

With the code changes, did executing below from smithy-rs/rust-runtime work fine (or maybe reduced error messages)?

RUSTDOCFLAGS='--cfg docsrs -Dwarnings' cargo +nightly-2025-05-04 doc --no-deps --document-private-items --all-features

which was this incantation that led to the original issue

@arielb1
Copy link
Contributor

arielb1 commented May 25, 2025

pyo3-asyncio is now https://crates.io/crates/pyo3-async-runtimes (the new crate is by the author of pyo3, the previous crate had a different author), you can update to that

@drganjoo
Copy link
Contributor Author

drganjoo commented Jun 2, 2025

pyo3-asyncio is now https://crates.io/crates/pyo3-async-runtimes (the new crate is by the author of pyo3, the previous crate had a different author), you can update to that

We'll handle the migration to pyo3-async-runtimes in a future PR. Since that change will involve several breaking API modifications, this current PR focuses specifically on removing the CI warnings to keep our build pipeline clean.

I've created a separate issue to track this upgrade: #4152

@drganjoo
Copy link
Contributor Author

drganjoo commented Jun 2, 2025

With the code changes, did executing below from smithy-rs/rust-runtime work fine (or maybe reduced error messages)?

RUSTDOCFLAGS='--cfg docsrs -Dwarnings' cargo +nightly-2025-05-04 doc --no-deps --document-private-items --all-features

which was this incantation that led to the original issue

Yes - this PR addresses all the Python-related errors. I intentionally kept these fixes separate from the other work in PR #4125 to maintain clearer change boundaries. Once this PR is merged, PR #4125 should be able to resolve the remaining warnings.

Copy link

github-actions bot commented Jun 2, 2025

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

landonxjames added a commit that referenced this pull request Jun 3, 2025
Note we expect some failures here, at least until the following are merged:

#4125

#4146
Copy link

github-actions bot commented Jun 3, 2025

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@landonxjames landonxjames merged commit 5c7b50d into main Jun 3, 2025
83 of 85 checks passed
@landonxjames landonxjames deleted the fahadzub/pyo3-upgrade branch June 3, 2025 17:47
@landonxjames
Copy link
Contributor

Went ahead and merged this since I am working on the next MSRV bump in #4154 and wanted to clear out these errors

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.

5 participants