Drop cargo update handling from CI

[dburgener]

This makes sense in lalrpop, where we ship a Cargo.lock, but also a library, but here with only a library and no shipped Cargo.lock, our testing will just end up with the latest semver compatible dependency versions regardless of whether we run cargo update first or not.

[Pat-Lafon]

I recently learned that the guidance for Cargo.lock files for libraries has been updated. Before it was commit your lockfiles for binaries but not libraries, but now the momentum is towards always commit your lockfiles.

The main concern is around MSRV I think? Plus random unintended breakages. rust-lang/cargo#12382 https://doc.rust-lang.org/nightly/cargo/faq.html#why-have-cargolock-in-version-control

[dburgener]

Interesting, thanks for noticing that and bringing it up.

I'm not sure I'm fully following the MSRV angle. Is it your understanding that the issue is just that MSRV updates are technically not semver breaking, and so we could get unintended breakage if a library updates their MSRV?

As far as I can tell, reading the PR and updated guidance, it seems like the purported benefits of checking in Cargo.lock are all for the project maintainers and contributors, rather than for project users. Users still get the latest semver compatible transitive dependencies, with feature resolution ignoring our Cargo.lock. So the decision to check in a Cargo.lock is ultimately about project development having stable CI, better git bisect etc. Does that match your understanding?

Some of the concluding statements in the PR mention:

Since there is no one right answer on how to solve all of these problems, we're documenting these trade offs so people can make the choice that is most appropriate for them.

So the question for defaults is who are we targeting? For a default path in documentation, it would be for new to intermediate users.

So my reading is that the guidance has moved from "don't commit lockfiles for libraries" to "maybe commit lockfiles for libraries. The tradeoffs are complicated", with a default targeted at newer/less experienced rust developers aimed at avoiding PR pain.

I think my overall opinion is unchanged by this discussion. I'd rather test libraries against what the user sees, and heavily error on the side of getting pain in PRs, rather than in user builds.

I think the "commit lockfile" angle might have more weight in a large project with lots of external contributors, so you're trying to avoid the "all PRs are red because of an external issue and you have 6 first time contributors being confused simultaneously" scenario. ascii-canvas is of course very much not that project.

All that said, in this particular case, with lalrpop as the only user of this project, and lalrpop having a committed lockfile, maybe the user breakage benefit is pretty minor, since lalrpop will test both the locked and fully updated versions in its CI regardless. But on the other side, we don't have external contributors to get confused by broken CI, and I doubt we're going to spend a ton of time git bisecting issues in this project. So neither way has a strong case. My vote is leave it as is, but I'm open to other opinions if you disagree.

[Pat-Lafon]

Ah, so to be clear, I'm not advocating for any changes at this time. I remember this crate and Lalrpop relying on cargo's guidance on lockfiles previously and I want to keep this updated guidance in mind going forward.

As far as I can tell, reading the PR and updated guidance, it seems like the purported benefits of checking in Cargo.lock are all for the project maintainers and contributors, rather than for project users. Users still get the latest semver compatible transitive dependencies, with feature resolution ignoring our Cargo.lock. So the decision to check in a Cargo.lock is ultimately about project development having stable CI, better git bisect etc. Does that match your understanding?

This is my understanding as well. It's worth noting atleast 2 things that have changed over time. The rise in MSRV support and usage in the Rust ecosystem which has gotten better tooling from cargo but is still rather inconsistent in terms of peoples usage/upgrading/breakage. Additionally the rise in workflow automation support(like Dependabot) for Rust which automates the tracking and maintenance of lockfiles in repositories.

All that said, in this particular case, with lalrpop as the only user of this project, and lalrpop having a committed lockfile, maybe the user breakage benefit is pretty minor, since lalrpop will test both the locked and fully updated versions in its CI regardless. But on the other side, we don't have external contributors to get confused by broken CI, and I doubt we're going to spend a ton of time git bisecting issues in this project. So neither way has a strong case. My vote is leave it as is, but I'm open to other opinions if you disagree

I think our current usage of lockfiles is judicious and a good place to be in. But what I think the trend is towards having lockfiles and automatically keeping them up to date so that there is no distinction/gap between our locked versions and fully updated versions. The main change/possible benefit for users is us front-running our users in terms of version updates. This way, if there is an unintended breakage, we clearly see it in ci and can see if we need to patch. This alludes to the idea mentioned in one of the links of a distributed crater run in the ecosystem.

Basically, lets continue as we are. In the future, we may set up a lockfile here or a more aggressive lockfile updating workflow in lalrpop if this comes up in practice.