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

Require minimum rust version in Cargo.toml instead of rust-toolchain file #2891

Closed
wants to merge 1 commit into from

Conversation

silwol
Copy link
Contributor

@silwol silwol commented May 17, 2022

Description

Review

  • Add a short description of the change to the CHANGELOG.md file

@silwol silwol force-pushed the dev/msrv branch 2 times, most recently from bd322a7 to c1095bd Compare May 17, 2022 08:44
Having a `rust-toolchain` file forces rustup to download the defined
toolchain version, effectively preventing developers from using the
version they already have installed in case it doesn't match.

Using the `rust-version` field in `Cargo.toml` is the proper way for
declaring a minimum supported rust version in my opinion, and it works
with rust toolchains not installed by rustup.
@silwol silwol changed the title WIP: Require minimum rust version in Cargo.toml instead rust-toolchain file Require minimum rust version in Cargo.toml instead of rust-toolchain file May 17, 2022
@silwol silwol marked this pull request as ready for review May 17, 2022 08:49
@silwol silwol requested a review from syrusakbary as a code owner May 17, 2022 08:49
@syrusakbary
Copy link
Member

I'm not sure if we want this. We need to double check that people using wasmer (as a lib) will not be enforced to use a specific Rust version

@silwol
Copy link
Contributor Author

silwol commented May 20, 2022

The question is, what is the reason for having a rust-toolchain in the first place? This is what interfered with my rust setup which is actually pretty mainline, so I expect this to cause issues for others as well if they want to start developing. My thought was that adding a MSRV is close enough to what the rust-toolchain file does right now. If there is no hard requirement on a MSRV, we can of course leave that out or think about other options.

@syrusakbary
Copy link
Member

The question is, what is the reason for having a rust-toolchain in the first place?

Basically, when people clone the project and contribute to it they must know what is the minimum version expected to work to compile Wasmer and run it's tests with.

When people use the library, they should not enforced to use a specific version of Rust.

@epilys
Copy link
Contributor

epilys commented May 20, 2022

From Cargo docs:

Setting the rust-version key in [package] will affect all targets/crates in the package, including test suites, benchmarks, binaries, examples, etc.

As for the libraries that use wasmer as a dependency, apparently Cargo does not use rust-version for dependencies.

It does not provide any value to wasmer to require a specific version because we're not (I think) using features that don't exist on earlier versions. To have this documented for ourselves we can build it for all versions after let's say 1.56 and see if there's something unexpected.

@jcaesar
Copy link
Contributor

jcaesar commented May 31, 2022

Basically, when people clone the project and contribute to it they must know what is the minimum version expected to work to compile Wasmer and run it's tests with.

You do want this.

  • The rust-toolchain file will be ignored if you're not using rustup
  • The rust-toolchain file doesn't set a minimum, but the default version which will be auto-installed even if you have a newer rust
  • Cargo-toml's rust-version sets a minimum and gives nice error message:
error: package `bar v0.1.0 (/src/bar)` cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.56.1

Caveat here is that rust before 1.56 will ignore rust-version:

warning: `rust-version` is not supported on this version of Cargo and will be ignored

This could be fixed by also bumping the edition to 2021.

Anyway: wasmer-vm depends on corosensei, which already sets rust-version = "1.59" and edition = "2021". So this PR doesn't really change the situation much, but makes it a bit clearer. ¯\_(つ)_/¯

As for the libraries that use wasmer as a dependency, apparently Cargo does not use rust-version for dependencies

You're misinterpreting this. The rust-version isn't used as a constraint for dependency resolution. The rust-version on dependencies is definitely checked.

@epilys epilys closed this Jul 21, 2022
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.

4 participants