Downgrade dev-dep tokio to support MSRV 1.65

[jayvdb]

Description

• Downgrade dev-dep tokio so that builds on Rust 1.65 still work
• Set MSRV in Cargo.toml
• Add Cargo.lock so that it is possible to determine what versions of deps are able to build on the MSRV

Checklist

• [ ] Formatted code using cargo fmt --all
• [ ] Linted code using clippy
  • [ ] with reqwest feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features serde,derive,reqwest-client-rustls -- -D warnings
  • [ ] with surf feature: cargo clippy --manifest-path influxdb/Cargo.toml --all-targets --no-default-features --features serde,derive,hyper-client -- -D warnings
• [ ] Updated README.md using cargo doc2readme -p influxdb --expand-macros
• [ ] Reviewed the diff. Did you leave any print statements or unnecessary comments?
• [ ] Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

[msrd0]

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

[jayvdb]

@msrd0 it is a dev-dependency and so does not effect users of this crate.

[msrd0]

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.

[jayvdb]

Can you please re--open this PR. I can remove the = because there is now a lock file which will keep the dep at an appropriate version to support MSRV 1.65

[msrd0]

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

1. We go the approach of this PR, i.e. add a Cargo.lock file to the root of the repository. This file would then be used by this repository, CI, maintainers and contributors alike, but not by users of the library. The Cargo.lock file of a library becomes irrelevant when the library is used, so users of the library would combine it with other, newer versions of the dependencies. That shouldn't™ be a problem but breaking semver accidentially isn't that hard and has happened before.

2. We add a Cargo.lock file but not in the root of the repository, and only use that with CI. That means maintainers and contributors, who usually don't compile using the minimum version of Rust anyways, get the chance to use latest versions, while CI can compile with older versions to keep msrv. This has the drawback that one might inadvertently require updates to the Cargo.lock file with a change and miss updating the hidden file.

3. We tell CI to use a nightly version of rust to generate a minimal Cargo.lock file before running the msrv compiler. This should™ not be a problem but I've had fun with that in the past as not all libraries manage to set correct minimal versions (and that can easily be missed so I can't even blame maintainers).

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 Cargo.toml without previous testing is probably not ideal)

[jayvdb]

Regarding the approach in this PR, you write

breaking semver accidentially isn't that hard and has happened before.

How is that relevant to adding a Cargo.lock to this repository, as done by this PR?

cargo publish does not even include the lock file in the artifacts uploaded to crates.io

Another approach is to patch the toml in the MSRV CI workflows, similar to

https://github.com/open-telemetry/opentelemetry-rust/blob/main/scripts/patch_dependencies.sh

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.

(Also, I don't think the current msrv is based on anything other than dependencies, so forcing it in the Cargo.toml without previous testing is probably not ideal)

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.

[msrd0]

How is that relevant to adding a Cargo.lock to this repository, as done by this PR?

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 lsd command line utility recently.

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.

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.

[msrd0]

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: https://github.com/influxdb-rs/influxdb-rust/pull/135

[jayvdb]

@Empty2k12 , I would appreciate your input on this.

[Empty2k12]

Hey @jayvdb I tried fixing the MSRV CI step yesterday but I did not manage to fix it. I am not sure committing the Cargo.lock is the best solution. The Check MSRV step I committed yesterday should work without it?

What du you think is the best solution?

[msrd0]

The latest rust release gave us a new toy. I played with it: https://github.com/influxdb-rs/influxdb-rust/pull/160

I believe this PR is currently less of a PR and more of a discussion anyways so I'll go ahead and close it. Feel free to protest if you believe otherwise.