-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix minimal versions dependencies #346
Conversation
This is the oldest version that works with snapbox
60a18c5
to
cc1fcf9
Compare
Pull Request Test Coverage Report for Build 9714439987Details
💛 - Coveralls |
cc1fcf9
to
194c988
Compare
Catch if we have under-specified any semver dependency
194c988
to
9b4fb26
Compare
Pull Request Test Coverage Report for Build 9721314969Details
💛 - Coveralls |
Looks like a dep has a minimal-versions problem |
Ok. Crap. We'll see when I have time to submit the same to them then. |
Head branch was pushed to by a user without write access
Fixed. There were two more crates in this dependency tree which did not specify their dependencies correctly at their oldest versions allowed by this crate. But all of them had fixed the issue in newer versions. So just bumping the minimum version here in this repository fixes the issue. Arguably those crates should also have a |
cd6d2ff
to
a3a20b9
Compare
Pull Request Test Coverage Report for Build 9756713128Details
💛 - Coveralls |
btw why did this matter and why was this started off with In theory, minimal versions is only relevant for dependents, and not yourself. We could lower the ecosystem burden of adopting this by leaving off the |
How much of a burden do you regard this CI check to be to adopt? I don't think it's very hard at all to keep the dependencies well specified, once this check is in place. And when adopting this CI job it's very easy to fix any existing issues. The only time it's a real problem is if a dependency you depend on have made the same error. But then it's only luck if that dependency happens to be a dev dependency instead of a prod dependency and you ignore checking dev dependencies by leaving out I personally find it does not simplify things a lot to remove |
From my understanding, the primary value add of verifying version requirements is for people who depend on you. I'm not seeing the value for tests and examples. However, I'm much more free with my dev-dependencies than other dependencies and by excluding dev-dependencies, it reduces the number of people I have to bug or workaround to make this work as I adopt it across all of my projects. |
I see your point. If you write a library, keeping everything neat and tidy for your own project's sake vs keeping it correct for your consumers are two different things. However it's still objectively wrong to say On the other hand, checking stricter and actively pushing these CI jobs upstream (like I just did) will spread it faster 🤷 I don't have a very strong opinion about dev-dependencies. But I do think we should push for this being a more standard CI check in general. Would be great if it could be stabilized, so nightly is not required. |
Would it be possible to get a release of |
Released |
The CI job I added in this PR is something I try to run on all my crates as well. It's something that catch if the crate (or a transitive dependency) has under-specified a dependency.
cargo +nightly update -Z minimal-versions
downgrades the entire dependency tree to the lowest allowed versions according to all the dependency semver specs in theirCargo.toml
files.It's very common to just add
foo = "0.3"
manually and then write code and run. Not realizing cargo automatically pulls in the latest version at the time. That might befoo 0.3.8
and you might unknowingly depend on features introduced infoo 0.3.4
. A CI check like this makes sure you don't under-specify version requirements like that.I found this because I depend on
trycmd
and this became an error withfiletime
. See this CI run: https://github.com/mullvad/unicop/actions/runs/9709586296/job/26798567031?pr=2. You depend onfiletime::set_file_mtime
, but that was introduced infiletime 0.2.6
. However, I had to bump the version even further, becausefiletime
did not build with minimal versions until0.2.8
.