Skip to content

fix: upgrade the toolchain#325

Merged
messense merged 6 commits intoPyO3:mainfrom
kemingy:fix_docker_rustc_version
Feb 23, 2025
Merged

fix: upgrade the toolchain#325
messense merged 6 commits intoPyO3:mainfrom
kemingy:fix_docker_rustc_version

Conversation

@kemingy
Copy link
Copy Markdown
Contributor

@kemingy kemingy commented Feb 22, 2025

Signed-off-by: Keming <kemingy94@gmail.com>
Signed-off-by: Keming <kemingy94@gmail.com>
@kemingy
Copy link
Copy Markdown
Contributor Author

kemingy commented Feb 22, 2025

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

Comment thread dist/index.js
Comment thread dist/index.js Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <kemingy94@gmail.com>
@kemingy
Copy link
Copy Markdown
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
@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