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

fix: upgrade the toolchain #325

Merged
merged 6 commits into from
Feb 23, 2025
Merged

Conversation

kemingy
Copy link
Contributor

@kemingy kemingy commented Feb 22, 2025

@kemingy
Copy link
Contributor Author

kemingy commented Feb 22, 2025

Not sure about the i686-unknown-linux-gnu CI failure. It also failed in the main branch.

dist/index.js Outdated
@@ -12114,6 +12114,7 @@ async function hostBuild(maturinRelease, args) {
const sccache = core.getBooleanInput('sccache');
const isUniversal2 = args.includes('--universal2') || target === 'universal2-apple-darwin';
core.startGroup('Install Rust target');
await exec.exec('rustup', ['update']);
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite like doing this unconditionally, I think we only need to do this when Rust toolchain is not specified or set to stable/beta/nightly while the currently installed version < MSRV specified in Cargo.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might break many repos upgrading to edition2024 with the default GitHub action settings. We might also need to update the README.

How about moving it to the next if (rustToolchain) block? So it will always update the toolchain if specified. My concern is that MSRV is not a compulsory configuration while it's common to use the latest stable version in dev.

Copy link
Member

Choose a reason for hiding this comment

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

How about moving it to the next if (rustToolchain) block?

Sounds better and it should also pass the rust toolchain argument.

@messense
Copy link
Member

Not sure about the i686-unknown-linux-gnu CI failure. It also failed in the main branch.

rust-lang/cargo#13546

I've disabled the test, please rebase onto main to fix CI.

@kemingy
Copy link
Contributor Author

kemingy commented Feb 23, 2025

Not sure about the i686-unknown-linux-gnu CI failure. It also failed in the main branch.

rust-lang/cargo#13546

I've disabled the test, please rebase onto main to fix CI.

Rebased. Shall we add some notes to the README that upgrading to edition2024 will require a toolchain specified in CI or repo to use the up-to-date version?

Signed-off-by: Keming <[email protected]>
@kemingy
Copy link
Contributor Author

kemingy commented Feb 23, 2025

CI failed due to the docker pull limit. But the previous one succeeded. No code change in the latest commit.

@messense messense enabled auto-merge (squash) February 23, 2025 05:36
@messense messense merged commit 2ea341a into PyO3:main Feb 23, 2025
30 checks passed
@kemingy kemingy deleted the fix_docker_rustc_version branch February 23, 2025 06:25
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.

run rustup update in the container to get the latest stable version
2 participants