Discussion: Start committing our lockfile

[madsmtm]

In a recent blog post, the Cargo Team outlined some reasons why they've changed the default for cargo new to not exclude the lockfile from version control.

I think winit should follow that direction, and start committing our Cargo.lock.

This would help greatly with our CI setup, both simplifying it in terms of caching, as well as making it break much less often due to changes in our dependencies.

On the other hand, perhaps it's a good thing that our CI breaks on changes in dependencies, it helps them discover unintended changes - hence why this is a discussion.

[daxpedda]

I think Rust has good guidelines on why to or not to commit Cargo.lock. https://doc.rust-lang.org/nightly/cargo/faq.html#why-have-cargolock-in-version-control.

My opinion on this is that it's only useful for helping debug issues during development, but ultimately we should make sure that we always support the newest dependency versions as that is what users will get when they use winit.

Though I don't see a particular upside for this in Winit, I'm strongly against committing the Cargo.lock file unless we also commit to using Dependabot (or anything similar) and continuously update it. Otherwise we are just not testing what users also experience. In addition I would prefer, like mentioned in the blog post, to also use cargo update in the CI (in addition to testing without cargo update), but that would break the motivation @madsmtm lined out above.

[kchibisov]

We could use a lock file only for CI, but not actually publish it, there's nothing wrong with doing something like that. I also like that they talk about keeping msrv compat, yet we have cargo install ignoring Cargo.lock by default...

Also, I think Cargo.lock is actually ignored from the deps.

[notgull]

Yes, I'm fine with committing Cargo.lock.

[daxpedda]

@kchibisov from https://github.com/rust-windowing/winit/pull/3486#issuecomment-1937922210.

It's a default behavior for libraries now even.

The default that has changed is that its not ignored in .gitignore, the default is not to just commit it. See the Rust blog post about this change:

Since there isn't a universal answer to these situations, we felt it was best to leave the choice to developers and give them information they need in making a decision.

It only solves issues, like having working bisect.

I'm not against committing Cargo.lock, I'm saying we should also check latest versions.

It's also worth saying that I'd rather test min version than latest deps, since if semver breaks it's not our issue, and if winit can't work correctly without version 0.8.N+1 we should force it in Cargo.toml, otherwise you have broken build which you don't notice because you pull latest and things magically work.

Its still an issue if latest dependencies don't work, even if its not our fault. There might also be behavioral changes, again not our fault, that don't necessarily cause compile errors.

---

I think its important that we advertise the right thing. As a library we can't publish the Cargo.lock file, so unless we pin every dependency in Cargo.toml, we do say that we work with the latest dependency version. It doesn't matter if its our fault or not if libraries make breaking changes, we should account for that, to a reasonable degree of course.

Updating Cargo.lock is also an issue, doing that manually is additional maintenance work that I'm not particularly happy with. But I can live with it if we use something like Dependabot.

[kchibisov]

Its still an issue if latest dependencies don't work, even if its not our fault. There might also be behavioral changes, again not our fault, that don't necessarily cause compile errors.

This will report regardless upstream, because users run winit on CI as well, and I get more reports from winit not compiling with minimal version of dependencies than that new version broke something, in fact i never seen anyone breaking semver, but winit breaking min version happens quite often.

Updating Cargo.lock is also an issue, doing that manually is additional maintenance work that I'm not particularly happy with. But I can live with it if we use something like Dependabot.

there's no point in updating unless you want to pull a new feature, that's the thing. And for breaking updates you already have to do that manually.

[madsmtm]

We made a decision at today's meeting:

• Lockfile for MSRV (maybe called Cargo-MSRV.lock to not conflict with user's local dev?) which is generated using Cargo's -Zminimal-versions.
• Run cargo update for not MSRV because we want to test against latest versions (i.e. don't use a lockfile here).

(I'm not gonna be doing this work myself, am feeling a little burnt out by this discussion, and ultimately I don't think it matters that much).