Skip to content

Pin our toolchain to Rust version 1.67.1 #4673

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 1 commit into from
Mar 9, 2023

Conversation

youknowone
Copy link
Member

@youknowone youknowone commented Mar 9, 2023

it will be better than only showing it on README and getting continuous same issues.

@youknowone youknowone marked this pull request as ready for review March 9, 2023 10:48
@youknowone youknowone changed the title Rust version 1.67.1 Pin our toolchain to Rust version 1.67.1 Mar 9, 2023
@DimitrisJim DimitrisJim merged commit 51020cd into RustPython:main Mar 9, 2023
@xiaozhiyan
Copy link
Contributor

@youknowone
Is the CI error in WASM related to the current PR?
https://github.com/RustPython/RustPython/actions/runs/4373906862/jobs/7652708971

Should we specify Rust version in rust-toolchain instead of Cargo.toml?
The current config may only apply to the main package, while not affecting other packages in the workspace.

@DimitrisJim
Copy link
Member

DimitrisJim commented Mar 9, 2023

@xiaozhiyan the ci error should be a different issue. Something to do with trying to get nodejs when new versions are released though I haven't really looked into it.

@youknowone youknowone deleted the pin-rust-version branch March 9, 2023 12:27
@xiaozhiyan
Copy link
Contributor

xiaozhiyan commented Mar 9, 2023

I see. Then I'll ignore the CI error for WASM.

How about the location of setting Rust version?

Should we specify Rust version in rust-toolchain instead of Cargo.toml?
The current config may only apply to the main package, while not affecting other packages in the workspace.

@DimitrisJim
Copy link
Member

The current config may only apply to the main package, while not affecting other packages in the workspace.

I think you might be right here, we might need to explicitly inherit these like we do for the deps.

@xiaozhiyan
Copy link
Contributor

xiaozhiyan commented Mar 9, 2023

I think https://github.com/RustPython/RustPython/blob/main/rust-toolchain may be the convenient place to specify Rust version. Otherwise we'd better remove the rust-toolchain file to avoid version conflict in the future.

I would usually specify rust-toolchain.toml file in the project root as follows,

[toolchain]
channel = "1.67.1"

@DimitrisJim
Copy link
Member

I don't think that currently fails the build if only specified in rust-toolchain, looking through blame, rust-toolchain was added for actions-rs/toolchain to work better.

We generally do always bump to the most recent version released so having stable in rust-toolchain and bumping rust-version shouldn't be too hard (though it would be nice if rust-version could be used by the action).

@xiaozhiyan
Copy link
Contributor

xiaozhiyan commented Mar 9, 2023

I think rust-toolchain.toml may be a suggested way to specify Rust version of a project (maybe including multiple packages), which is respected by Cargo and is not specific to particular CI tool or script.
https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file

Btw, Rust 1.68.0 was just officially released, but maybe it would not cause compatibility issue with current config.

@DimitrisJim
Copy link
Member

Ah, it does get respected but it doesn't fail. Instead it automatically uses the appropriate version if it already exists or does a download of the more recent version automatically.

I'd personally prefer to let the user make that decision on their own after getting a build error but I could also see how this would work.

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.

3 participants