-
Notifications
You must be signed in to change notification settings - Fork 78
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
Downgrade dev-dep tokio to support MSRV 1.65 #157
base: main
Are you sure you want to change the base?
Conversation
I very strongly believe that forcing an exact version of tokio is a grave mistake. It will cause massive incompatibilities with users of this crate when they mix with other libraries. Please never do this |
@msrd0 it is a dev-dependency and so does not effect users of this crate. |
That is not true. I'm not sure about resolver 2 from edition 2021, but with previous versions the resolver always considered dev dependencies, and e.g. enabled features in release mode only necessary for the dev dependencies. I've ended up with binaries linking to openssl when they were compiled with rustls multiple times. One can add a Cargo.lock file with older dependencies and/or use nightly's unstable feature to create one with minimal versions to keep compiling with the same msrv. Using '=' in versions is with very few exceptions a mistake. |
Can you please re--open this PR. |
We should discuss the approach we take. "Spurious" CI failures, i.e. ones not caused by the PR itself, are very annoying, so the current situation is undesirable. I see the following options, feel free to add ones I might've missed
I have personally used version 2 and version 3 in the past. @Empty2k12 what do you think? (Also, I don't think the current msrv is based on anything other than dependencies, so forcing it in the |
Regarding the approach in this PR, you write
How is that relevant to adding a Cargo.lock to this repository, as done by this PR?
Another approach is to patch the toml in the MSRV CI workflows, similar to That avoids the lock file, and means there is a simple to read definition of how to obtain the MSRV dep tree, for users who need that. However that isnt as obvious for users of this project. While a lock file is a little harder to read, anyone who has to deal with old MSRV will see a Cargo.lock in this repo and know that it contains the information they need to reproduce the MSRV in their project.
I dont get where you are going with this. I tested it locally. And CI is testing it works with this MSRV. If there are concerns about compatibility with this MSRV, it should be addressed by adding tests. |
It means we (maintainers/contributors/ci) won't see the breaking change introduced by dependencies, but lib users will. That can make reproducing bugs harder. Happened to the
Setting rust-version=... will make compilers below that version refuse to compile. But if our msrv is based on non minimal dependencies, it might be unnecessarily high. We should test that actually we and not our dependencies need that version before asking compilers to refuse compilation. |
Just for reference, the last increase of MSRV seems to have been a PR I made the same day we got another PR, probably to fix unrelated CI failures from that other PR: #135 |
Description
Cargo.lock
so that it is possible to determine what versions of deps are able to build on the MSRVChecklist
cargo fmt --all
cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features serde,derive,reqwest-client-rustls -- -D warnings
cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features serde,derive,hyper-client -- -D warnings
cargo doc2readme -p influxdb --expand-macros