search

assert-rs / snapbox

Snapshot testing for a herd of CLI tests
docs.rs/trycmd
Apache License 2.0
132 stars 17 forks source link

Fix minimal versions dependencies #346

Closed faern closed 2 months ago

faern commented 3 months ago

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 their Cargo.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 be foo 0.3.8 and you might unknowingly depend on features introduced in foo 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 with filetime. See this CI run: https://github.com/mullvad/unicop/actions/runs/9709586296/job/26798567031?pr=2. You depend on filetime::set_file_mtime, but that was introduced in filetime 0.2.6. However, I had to bump the version even further, because filetime did not build with minimal versions until 0.2.8.

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9714439987

Details

Totals Coverage Status
Change from base Build 9501692150: 0.0%
Covered Lines: 1390
Relevant Lines: 2691
💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9721314969

Details

Totals Coverage Status
Change from base Build 9501692150: 0.0%
Covered Lines: 1390
Relevant Lines: 2691
💛 - Coveralls
epage commented 3 months ago

Looks like a dep has a minimal-versions problem

faern commented 3 months ago

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.

faern commented 3 months ago

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 minimal-versions CI job. But they currently have no issue with this, and I don't currently have the time to submit that upstream :)

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 9756713128

Details

Totals Coverage Status
Change from base Build 9501692150: 0.0%
Covered Lines: 1390
Relevant Lines: 2691
💛 - Coveralls
epage commented 2 months ago

btw why did this matter and why was this started off with --all-targets?

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 --all-targets flag and only checking default targets.

faern commented 2 months ago

--all-targets was put there to catch invalidly specified dependencies used in examples and tests etc as well as the main library/binary code.

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 --all-targets.

I personally find it does not simplify things a lot to remove --all-targets but instead it makes the CI checked miss what it was supposed to uphold.

epage commented 2 months ago

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.

faern commented 2 months ago

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 [dev-dependency] foo = "0.2" if your tests use features in foo 0.2.3. However, I can agree that this error is not likely to cause much trouble in practice. And as you say, if/while the ecosystem often commit these crimes, it's going to inflict a bit more pain on you downstream, the more dependencies you strictly check for this correctness property.

On the other hand, checking stricter and actively pushing these CI jobs upstream (like I just did) will spread it faster :shrug:

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.

faern commented 2 months ago

Would it be possible to get a release of trycmd? Since the latest version on crates.io still depend on snapbox ^0.6.0, which has the minimal-versions issue, my minimal-versions CI fail. Thank you :blush:

epage commented 2 months ago

Released

smoelius commented 2 months ago

FWIW, I found this discussion very enlightening. Thank you, @faern! Thank you, @epage!