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

Consider using a rust-toolchain.toml file #25603

Closed
ruuda opened this issue May 27, 2022 · 5 comments
Closed

Consider using a rust-toolchain.toml file #25603

ruuda opened this issue May 27, 2022 · 5 comments
Assignees
Labels
stale [bot only] Added to stale content; results in auto-close after a week.

Comments

@ruuda
Copy link
Contributor

ruuda commented May 27, 2022

Problem

Solana depends intimately on the particular rustc version used. One example of this was solana-labs/rbpf@5394026, where before that change in rbpf was merged, Solana would segfault when compiled with the wrong rustc version.

However, for the default approach of building a Rust project (cargo build), rustup uses the system-wide default toolchain, not the one suitable for the particular Solana release. Fortunately, the right Rust version is recorded in the repository:

stable_version=1.60.0
However, it’s in a nonstandard location, and it’s not used by rustup’s cargo proxy binary.

Proposed Solution

Add a rust-toolchain or rust-toolchain.toml file to pin the stable toolchain using rustup’s standard mechanism. The existing ci/rust-version.sh can still manage the nightly toolchain. As far as I am aware, there is no standardized way to manage multiple toolchains.

@mvines, you seem to most often update the Rust version, what do you think?

@mvines
Copy link
Member

mvines commented May 27, 2022

sounds like a good idea!

@KirillLykov
Copy link
Contributor

KirillLykov commented Dec 18, 2022

The plan is to split one huge PR into several smaller:

  1. Add ./cargo-nightly, use it instead of ./cargo introduce cargo-nightly #29319
  2. Add toolchain file, take version from it take rust version from toolchain file #29320
  3. Get rid of ./cargo in (depends on 1 and 2)
  1. Do the same replacement for github workflows Simplfiy scripts using ./cargo after git workflow changes #29105

@yihau
Copy link
Member

yihau commented Dec 22, 2022

I'm thinking should we use the pattern below to replace ./cargo instead of only cargo 🤔

source ci/rust-version.sh stable

... (omit)

cargo +"$rust_stable"

the concern is that if someone set rustup override, I think the priority will be higher than cargo-toolchain.toml.

@KirillLykov
Copy link
Contributor

I think the main idea of using cargo build is that it is simple and expected procedure. If a user has set up rust version using overrides, I would assume that he/she knows what he/she is doing and it is up to developer.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 28, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2024
@steviez
Copy link
Contributor

steviez commented Jan 5, 2024

Switching this from "not planned" to "closed"; we still depend on the custom cargo script for some aspects (ie running fmt and clippy with nightly), but, we do have a rust-toolchain.toml file as of #29320 so the basic case of just building the repo is supported.

Any further work to try to reduce usage of this scripts can probably take place in new PR's / issue(s)

@steviez steviez closed this as completed Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

No branches or pull requests

5 participants