image-rs / jpeg-decoder

JPEG decoder written in Rust
Apache License 2.0
149 stars 87 forks source link

Toolchain pinning necessary? #192

Open vsaase opened 3 years ago

vsaase commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

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 commented 3 years ago

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