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

Strict version requirement of tokio #409

Closed
EdJoPaTo opened this issue Jun 21, 2022 · 7 comments · Fixed by #412
Closed

Strict version requirement of tokio #409

EdJoPaTo opened this issue Jun 21, 2022 · 7 comments · Fixed by #412

Comments

@EdJoPaTo
Copy link
Contributor

As discussed in #396 the version requirement of tokio was set to a strict version (=1.17.0).
As described by @adamscybot another package with a higher requirement (like 1.19) does break the setup for them.

Personally I do not know how the new dependency system (default with the 2021 edition) changes that behaviour. (Its irrelevant for the crate, its relevant only for the binary using the crate)

Personally I like using 1 as a version requirement as its the simplest requirement. Thinking about this case, it might lead to problems of incompatibility: crate A requires a feature introduced in 1.2 but specifies 1, crate B requires =1.1. Will 1.1 be installed and the build fail because crate A can't be built?

The idea behind the strict version requirement are performance regressions in tokio that happened in the past. Do we have other ways of ensuring a well working setup for the users of this library?

@tekjar
Copy link
Contributor

tekjar commented Jun 21, 2022

As far as I remember, cargo can download multiple versions of dependencies. But I guess this has some semver related restrictions. Let me dig this

@adamscybot
Copy link

adamscybot commented Jun 21, 2022

Thanks for raising this.

I have a lot of experience in the way NPM does this. Rust is a bit diff but there's huge similarities as well.

Personally I like using 1 as a version requirement as its the simplest requirement. Thinking about this case, it might lead to problems of incompatibility: crate A requires a feature introduced in 1.2 but specifies 1, crate B requires =1.1. Will 1.1 be installed and the build fail because crate A can't be built?

This generally never happens because if crate A requires a feature introduced in 1.2, it specifies >=1.2 in the TOML -- and if it doesn't, that's a clear bug in that TOML and a problem for that packages contributors to resolve. Its not something we should think about here. Generally, if this is the case, that package would break for existing users of it who are cargo.lock'ed to 1.1 so they just introduced a problem for themselves, not to mention everyone else consuming it transitively. Such a package would simply be "broken" as it it has mis declared its intentions.

The idea behind the strict version requirement are performance regressions in tokio that happened in the past. Do we have other ways of ensuring a well working setup for the users of this library?

The answer to this is one or both of these options:

  • rumqtt contributors notice the problem and release a new version which specific version range restriction >=1.x <=1.y for that dep. Of course, is kind of "close the stable door after the horse has bolted" situation for users on an existing version. You could choose to yank the version for those people (but cargo doesnt yet allow a yank reason to be displayed, unlike npm).
  • A warning could be issued in logs if its an untested new version

In most respects I think its a question of the first bullet above + documentation. Pinning it is kind of cracking a nut with a sledgehammer when you dont know if the nut exists yet.

As far as I remember, cargo can download multiple versions of dependencies. But I guess this has some semver related restrictions. Let me dig this

You can only alias multiple copies as first class deps. Youre shit out of luck when its transitive without doing patching hence why pinning specific versions of common packages in Rust is almost never a good idea sadly.

As it stands I believe the correct course of action is to restore the 1 version.

I miss NPMs way of doing this a bit because there, in the situation im in, it would automatically install 2 versions and link them accordingly automatically. Though that also has its own massive downsides as well (debugging with 2 version of same package that are not compatible is a nightmare and don't even mention singletons). But rust has gone for a "1 version per package" philosophy unless they are first class deps that have unique aliases and those are used explicitly, which you cant control in 3rd party crates.

@EdJoPaTo
Copy link
Contributor Author

This generally never happens because if crate A requires a feature introduced in 1.2, it specifies >=1.2 in the TOML -- and if it doesn't, that's a clear bug in that TOML and a problem for that packages contributors to resolve.

That's exactly what I introduced with #396: 1.61 as in both cases 1.7 is installed. Personally its strange to me to specify .6 when something different is used then which is why I prefer to just omit it. (I know how it works and I think its counter intuitive.)
But I guess you are right, the case that I described could happen and would be a bug then, it would just spark at a different spot currently.
Basically the lowest possible version should be specified in the Cargo.toml? Is there some kind of tooling to check for that?

I miss NPMs way of doing this a bit because there, in the situation im in, it would automatically install 2 versions and link them accordingly automatically.

Cargo definitely works with the same dependency in different versions.
I bodged something some time ago which had a dependency which used clap v2 while I was using clap v3. This works perfectly fine, it is just compiling way more dependencies than needed but it works. (Its a bodge, nothing perfect)
I regularly see this with minor versions too when I take a look into the Cargo.lock diffs.

@EdJoPaTo
Copy link
Contributor Author

Tokio states its own Bug Patching Policy and LTS versions.

Maybe rumqtt should adopt the latest LTS, check for regressions and if it works, go with it.

That would be 1.18 currently.

tokio also suggests using the ~ to use the latest bugfixes of that release. Should be discussed if the bugfixes are worth it or if its a bigger issue to get some regressions there.

@adamscybot
Copy link

adamscybot commented Jul 1, 2022

Tokio states its own Bug Patching Policy and LTS versions.

Maybe rumqtt should adopt the latest LTS, check for regressions and if it works, go with it.

That would be 1.18 currently.

tokio also suggests using the ~ to use the latest bugfixes of that release. Should be discussed if the bugfixes are worth it or if its a bigger issue to get some regressions there.

This makes sense to me as an approach (using tilde semver matching). I think essentially -- its better to leave it up to the consumer to an increased degree when compared how it is now, even if that might mean exposing to more risk as the prevalence/potential for frustration of not being able to use it is greater than the demon of using an untested version. The latter can be countered with stricter version restrictions in the event a major regression was detected; albeit after the fact, but thats about as good as its gonna get.

I hadn't thought of this before but automated perf regression testing against new versions is perhaps the "ultimate" answer but obviously requires a lot of work and it'd still only help with detection and youd still need new version to backlist whatever tokio version has the problem.

Essentially, I think ultimately these are just risks you knowingly take using any library, and that's ok. Its also that no one can really fix tokio's issues, thats up to them -- itd be unusual if all consumers were going as far as perf regression testing against multiple versions of a major third party lib unless it was either mission critical or evidently badly maintained and a super common problem. Even then, as a lib youd often just accept palming off that risk to the users to some degree, even though its a bit uncomfortable.

Cargo definitely works with the same dependency in different versions.
I bodged something some time ago which had a dependency which used clap v2 while I was using clap v3. This works perfectly fine, it is just compiling way more dependencies than needed but it works. (Its a bodge, nothing perfect)
I regularly see this with minor versions too when I take a look into the Cargo.lock diffs.

Oh! I had thought different, apologies if I was incorrect in the first instance. Thank you for pointing out the error as it means I should reset my understanding. That begs the question why the same isn't happening for me automatically....hmmm.

Basically the lowest possible version should be specified in the Cargo.toml? Is there some kind of tooling to check for that?

Feels sensible generally speaking. I think only a human (the authors) can decide what that version is because "it compiles" is a low bar and it could be buggy with certain versions, which kind of loops back to your original point. Though in the absence of that knowledge, going with "1" is probably mostly fine in all probability.

@h3nill
Copy link

h3nill commented Jul 5, 2022

Oh! I had thought different, apologies if I was incorrect in the first instance. Thank you for pointing out the error as it means I should reset my understanding. That begs the question why the same isn't happening for me automatically....hmmm.

I tried having a similar setup as your package (with transitive dependency as you mentioned previously) with clap and it refused to compile that as well. So I am guessing its not something related to tokio itself but about how cargo works with this particular setup. I am still trying to understand why that setup is "not right" and fails to compile.

Nonetheless I think changing the version to "1" is the way to go.

@h3nill
Copy link

h3nill commented Jul 5, 2022

Okay so Cargo is supposed to only compile multiple versions of a crate if its semver incompatible which 1.17 and 1.19 are not. Thanks to @shepmaster for helping to figure this out!

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 a pull request may close this issue.

4 participants