Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.06k stars 2.33k forks source link

Create a Rust toolchain file to standardize Rust version #9570

Closed Eric-Arellano closed 1 year ago

Eric-Arellano commented 1 year ago

Several contributors have been confused by this error:

      error: failed to select a version for the requirement `ahash = "^0.8.3"`
      candidate versions found which didn't match: 0.8.0, 0.7.6, 0.7.5, ...

It's because their installed Rust version is too old and cannot build modern ahash. I.e., our minimum supported Rust version (MSRV) was increased.

Rather than our contributors needing to manually upgrade their Rust, we can instead automate this with a Rust toolchain file, which tells Rustup what version our project uses. As long as the user has Rustup on their PATH, they will always be using a consistent Rust version that we/our CI have verified is working.

jakelishman commented 1 year ago

I'm not 100% certain this is going to work right - the channel we want is stable, but that's also the default, so unless this file also triggers rustup to search for updates on every invocation (which also isn't great), it might not answer the question. We don't want to pin to the exact MSRV for most dev work, because we don't lint using MSRV clippy (we use stable) and we have had issues in the past because of discrepancies between the two.

We specify the MSRV in Cargo.toml, so cargo will already give the correct error message, as soon as we get most people onto toolchains 1.59+ (the first version to recognise the option).

jakelishman commented 1 year ago

e.g.:

[~/code/qiskit/terra] (qiskit/3.10) (main)
jake@ninetales$ rustup override set 1.60
info: using existing install for '1.60-x86_64-apple-darwin'
info: override toolchain for '/Users/jake/code/qiskit/terra' set to '1.60-x86_64-apple-darwin'

  1.60-x86_64-apple-darwin unchanged - rustc 1.60.0 (7737e0b5c 2022-04-04)

[~/code/qiskit/terra] (qiskit/3.10) (main)
jake@ninetales$ cargo build
error: package `qiskit-terra v0.24.0 (/Users/jake/code/qiskit/terra)` cannot be built because it requires rustc 1.61 or newer, while the currently active rustc version is 1.60.0

(though if you've got 1.56.1, the MSRV for the 0.23 series, you'll get the ahash error you quoted if you try to build main)

Eric-Arellano commented 1 year ago

It's not safe for the MSRV to be different than the Rust development version because it means developers may be using new features that exceed the MSRV.

For example, our current MSRV is 1.61. But stable is 1.67.1. On main, I added this line:

diff --git a/src/sabre_layout.rs b/src/sabre_layout.rs
index 1fe1b7d53..20b50c046 100644
--- a/src/sabre_layout.rs
+++ b/src/sabre_layout.rs
@@ -43,6 +43,7 @@ pub fn sabre_layout_and_routing(
     seed: Option<u64>,
 ) -> ([NLayout; 2], SwapMap, PyObject) {
     let run_in_parallel = getenv_use_multiple_threads();
+    let _ = 1i32.ilog(5);
     let outer_rng = match seed {
         Some(seed) => Pcg64Mcg::seed_from_u64(seed),
         None => Pcg64Mcg::from_entropy(),

That uses a feature only available to Rust 1.67+. cargo check works just fine for me. But if I run rustup override set 1.62, then cargo check now fails.

❯ cargo check
    Checking qiskit-terra v0.24.0 (/Users/arellano/code/qiskit-terra)
error[E0599]: no method named `ilog` found for type `i32` in the current scope
  --> src/sabre_layout.rs:46:18
   |
46 |     let _ = 1i32.ilog(5);
   |                  ^^^^ help: there is an associated function with a similar name: `log`

More generally, it's also desirable for us to all be using the exact same version of tooling. That gives consistency to our experiences, e.g. the behavior of cargo clippy. It helps to avoid the notorious Works On My Machine™️ issues.

jakelishman commented 1 year ago

Yeah, I suppose the better solution is just to lint on MSRV as well. I think we'd previously tended towards wanting to use the most recent versions of tools for dev (for the improved lints, etc), and Rust usage in Terra is all pretty small-scale, so there was just a couple of us doing it and with the explicit MSRV test in CI it wasn't hard to enforce. But I definitely buy what you're saying - linting on MSRV and promoting using it for dev is a better strategy at a larger scale.

Eric-Arellano commented 1 year ago

I think we'd previously tended towards wanting to use the most recent versions of tools for dev (for the improved lints, etc)

Huge +1 to desiring to use more recent stuff. Rust and Clippy keep getting much better :)

But the right way to do that is by bumping our MSRV. Which of course is a tradeoff of worse compatibility for end-users.