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

Toolchain pinning necessary? #192

Open
vsaase opened this issue Jul 25, 2021 · 4 comments
Open

Toolchain pinning necessary? #192

vsaase opened this issue Jul 25, 2021 · 4 comments

Comments

@vsaase
Copy link
Contributor

vsaase commented Jul 25, 2021

Hi,

just ran cargo test and noticed that the currently pinned toolchain (1.34.2) is too low, we need at least 1.40.0

Is there a reason this needs to be pinned? Could just delete the rust-toolchain file, tests run fine with the stable toolchain, in my case:

stable-x86_64-pc-windows-msvc
rustc 1.53.0 (53cb7b09b 2021-06-17)

thanks,
Victor

@HeroicKatora
Copy link
Member

I'm not sure I understand, rust-toolchain should only affect the choice that cargo makes for contributors of the project. It doesn't 'pin' it for a whole dependency tree. In that sense it ensures backwards compatibility for the code of the crate and its dependence. We know fully well that it compiles on stable due to Rust's forward compatibility promise and CI executing that case all the time.

@vsaase
Copy link
Contributor Author

vsaase commented Jul 25, 2021

I see, sorry for being not as experienced in Rust, maybe you can help me understand.

What I did is clone the repo, using the master branch, and run rustup show in the directory. It tells me that the active toolchain is 1.34.2-x86_64-pc-windows-msvc (overridden by 'C:\Users\vsaas\src\jpeg-decoder\rust-toolchain'). Now when I run cargo test I get these errors:

error[E0658]: naming constants with _ is unstable (see issue #54912) due to memchr

error[E0658]: non exhaustive is an experimental feature (see issue #44109) due to memchr

error[E0658]: use of unstable library feature 'alloc': this library is unlikely to be stabilized in its current form or name (see issue #27783) due to itertools

error[E0161]: cannot move a value of type dyn std::ops::FnOnce() + std::marker::Send: the size of dyn std::ops::FnOnce() + std::marker::Send cannot be statically determined due to crossbeam-utils

so it seems that some of the dependencies have lost compatibility with 1.34.2 and cargo does not succeed in getting compatible older versions

@HeroicKatora
Copy link
Member

Ah, now I see. That's of course quite unfortunate because there are two goals at conflict here. Let me explain by given a rough overview over the build tool. cargo has two separate sets of dependencies:

  • [dependencies], used when building the crate as used in a dependency of another crate, and in cargo build.
  • [dev-dependencies], used for the development setup of the crate itself. So for cargo test etc.

Now, we want the first set to be compatible to 1.34.2 because that's what downstream crates will end up needing. All of this is in order not to break any downstream users that may depend on an older version of the toolchain (Debian stable for example). The second set, however, doesn't need to be compatible because it's not relevant for backwards compatibility. I think the source of the error is the criterion crate, which we use for benchmarking.

Problematically there's a conflict of interests here: If we don't setup any toolchain version then it's easy to accidentally add code that isn't compatible with 1.34.2. However, this setup also appears to imply you can build but then can't run the tests easily.

On your local machine you can always overwrite the default choice: cargo +stable test. I wonder how we could explain all of the above succinctly in contribution instructions?

@vsaase
Copy link
Contributor Author

vsaase commented Jul 25, 2021

Thanks for explaining!
But even the build dependencies are not ensured to be compatible to 1.34.2 with the current setup. Try running cargo check and compare to cargo +stable check.

This is the error: error[E0161]: cannot move a value of type dyn std::ops::FnOnce() + std::marker::Send: the size of dyn std::ops::FnOnce() + std::marker::Send cannot be statically determined due to crossbeam-utils-0.8.5

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

No branches or pull requests

2 participants