-
Notifications
You must be signed in to change notification settings - Fork 9
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 CI workflow #10
Conversation
I'm unsure what's going on with this error for macos runners (some versions):
I found a similar issue here. This definitely needs a squash-merge at this point too (if we merge). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sorry I missed this for a couple days. Overall looks good to me, below are a few quick thoughts / suggestions:
@@ -160,7 +153,7 @@ jobs: | |||
needs: [fmt] | |||
runs-on: ubuntu-latest | |||
steps: | |||
- uses: actions/checkout@v3 | |||
- uses: actions/checkout@v4 | |||
- uses: dtolnay/rust-toolchain@stable | |||
with: | |||
toolchain: nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably update the coverage job to use cargo-llvm-cov
and stable Rust (like PyO3 does). Possibly a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I think it would be best to align with PyO3's practices. I'd like to take another pass at CI once we get the first release of pyo3-async-runtimes
out the door.
.github/workflows/ci.yml
Outdated
- python-version: pypy-3.9 | ||
platform: { os: "windows-latest", python-architecture: "x86" } | ||
include: | ||
# Test minimal supported Rust version | ||
- rust: 1.63.0 | ||
- rust: 1.70.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question on the MSRV bump. In PyO3 we support ranges of optional dependencies and have a helper script to downgrade them for the MSRV job to compatible versions. I'm unsure what the best option is here. Keeping the MSRV down is useful for linux distributions putting these packages in their package managers, e.g. PyO3/pyo3#3113
We last did an MSRV bump with PyO3 0.22 to 1.63, which is the current version supported by Debian stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basing the MSRV on Debian stable and aligning it with PyO3's requirements, rather than targeting the latest version required by tokio
, feel like the right approach (I'll revert back to 1.63). I'm just not sure of the best way to set this up in CI to avoid the error with tokio
. The helper script sounds like what we need, but I'll need some time to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now seems to be the last issue with CI, other than some warnings I'd like to address on another pass (some of the Rust toolchain installation actions are very outdated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I can perhaps try to port that script tomorrow 👍
.github/workflows/ci.yml
Outdated
python-architecture: "x64", | ||
rust-target: "x86_64-apple-darwin", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macos-latest
runners are now arm:
python-architecture: "x64", | |
rust-target: "x86_64-apple-darwin", | |
python-architecture: "arm64", | |
rust-target: "aarch64-apple-darwin", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed up the merge conflict, I assume this is good now. Thanks!
Upgrades the action versions and
black
in theci.yml
workflow, and adds Python 3.13 to the build matrix.As a test, I've also taken the liberty of bumping up the MSRV from 1.63 to 1.70 so that the latesttokio
can be supported, which fixes the MSRV CI job. (I could do this in a separate PR, or we could adjust the approach here.)I've also made the bold move of dropping Python 3.7 and 3.8 in CI. (I can also revert this.)