Check in Cargo.lock

[hugwijst]

With the stabilization of Cargo.lock file publishing in rust-lang/cargo#7026, lets restart the discussion from #148 about adding Cargo.lock to the repo.

My two cents: not having Cargo.lock checked in is a huge road block to using svd2rust in production.

Backgroud: we are running svd2rust on every CI build. By not locking svd2rust, there is a chance that the final artifact is not reproducible if a package dependent on by svd2rust fails to adhere to semver. More realistically, such an update breaks the build, which is completely unacceptable.

[hugwijst]

The first stable version of Cargo that supports publishing lock-files is 1.37.

[therealprof]

I disagree with your reasoning: Semver has proven to provide pretty good stability that API breakage does pretty much never happen.

On the other hand people are continuously improving their crates in a compatible manner, ranging from dependency cutting over optimisation all the way to fixing bugs detected by newer Rust compiler versions or forward compatibility lints that it actually is very desirable to automatically receive those updates without having to intervene manually, which is a cumbersome and very annoying maintenance task.

Indeed as you mentioned the purpose of Cargo.lock is to provide reproducible builds however I don't see much value in rebuilding svd2rust continuously. If you want to have a defined and stable version of svd2rust, why not compile a specific version, execute your tests and then re-use that pre-built artefact as long as you like. Alternatively you could also use the new cargo ability to download all dependencies and do an offline build, then you'll also get the exact same dependencies every time.

IMHO the far bigger hazard here is that svd2rust changes in an incompatible way...

[hugwijst]

I agree that semver generally works quite well, and that picking up the latest version of compatible crates is generally desirable for end users. Note that this won't change when including Cargo.lock files in published releases: Cargo will normally ignore the lock file, unless install --locked is used.

Regardless of if we would rebuild svd2rust on every build or if we would distribute an artifact, we would want to be able to 1. rebuild the artifact to prove it wasn't tempered with and 2. be able to apply fixes without having to trace back versions of dependencies that allowed the project to build.

Furthermore, it turns out that correctly applying semver is quite tricky. We've had breakages specifically due to semver when the edition field got added to Cargo.toml. Several crates released minor or patch version updates that added edition = 2015 to their toml file, which our older version of Cargo didn't understand. To reproduce, try building svd2rust v0.14.0 using Rust version 1.29.1.

I guess my question is: why not check in Cargo.lock?

[therealprof]

I guess my question is: why not check in Cargo.lock?

Because it requires a ton of manual work to maintain. Work that I'm absolutely not willing to commit to for in my eyes negative gain.

I don't really see why we are supposed to do the pinning upstream when you can easily do it yourself. If you're so keen on building exact versions anyway you're most likely doing your own vetting/verification anyway so why not simply maintain your own fork with a Cargo.lock.

[Emilgardis]

I don't think this is the way to go. If something breaks semver, we deal with it as fast as we can. But enforcing Cargo.lock requires too much work to keep up to date and provides negligible improvements.

If you want to make your CI work reproducibly, I'd say you should check in a Cargo.lock besides Cargo.toml in your CI source/builder.

Edit:: too clarify, my position on this has changed after having to deal with checked in Cargo.lock in repositories. One way to solve this for me is to expand our CI and implement automatic Cargo.lock bumps, but that introduces repo noise and can make bugs slip through more easily.

[newAM]

This is a problem for packaging maintainers. svd2rust is not reproducible without Cargo.lock. Reproducible builds are required to package svd2rust on some distros. For example, NixOS maintains an out-of-tree Cargo.lock to ensure builds are reproducible:

• https://github.com/NixOS/nixpkgs/blob/3b6c3bee9174dfe56fd0e586449457467abe7116/pkgs/development/tools/rust/svd2rust/cargo-lock.patch
• https://github.com/NixOS/nixpkgs/blob/3b6c3bee9174dfe56fd0e586449457467abe7116/pkgs/development/tools/rust/svd2rust/default.nix

My opinion is that reproducible builds add value, and requiring each user who desires reproducible builds to maintain their own Cargo.lock is of negative value.

This has all been discussed before, but there has been new developments since this issue was opened:

• Dependabot has become a mature tool for automatically updating rust dependencies.
• The svd2rust MSRV (1.46) supports publishing lock files.

Do either of these recent developments change the discussion / decision?

[therealprof]

The svd2rust MSRV (1.46) supports publishing lock files.

That makes it even worse. Now we not only have to maintain the Cargo.lock but also make frequent releases for it to actually take effect.

Do either of these recent developments change the discussion / decision?

I'm not terribly opposed to having a Cargo.lock iff there was a reliable volunteer keeping the whole thing going. 😉 I'm not going to hold my breath for someone showing up though...

[Emilgardis]

Lock files only matter for published binary crates when using cargo install --locked

I think adding a lockfile is fine if we make at least one ci check to use --locked

It doesnt need to be updated frequently imo.

[therealprof]

Lock files only matter for published binary crates when using cargo install --locked

Sure, but that's exactly the point: We still need to have more frequent releases to cater for the people who requested reproducible builds in the first place. A big part of the idea behing reproducible builds is that you can a) reproduce them and make sure no one tampered with the binaries and b) know exactly which version goes in so it's known whether you're affected by security issues or such... The latter implies that lockfiles are frequently updated and new versions released; not that it should matter at all for an offline "text processor"...

I think adding a lockfile is fine if we make at least one ci check to use --locked

No, if Cargo.lock is addded added, the whole CI needs to use it.

[Emilgardis]

No, if Cargo.lock is addded added, the whole CI needs to use it.

Ah yes, I was thinking that new features in respect to semver patch are additive, and a single check would catch when features are missing, but that does not work if the change is not in this particular check, eg we now parse x properly in the xml library, here is the test for it.

[Emilgardis]

A lockfile has been added in #741