HMIProject / open62541-sys

2 stars 2 forks source link

Delete Cargo.lock #6

Closed uklotzde closed 4 months ago

sgoll commented 4 months ago

What is the motivation here? The latest recommendations seem to indicate that Cargo.lock should be included in library crates.[^1]

[^1]: Why have Cargo.lock in version control? and Verifying Latest Dependencies

uklotzde commented 4 months ago

What is the motivation here? The latest recommendations seem to indicate that Cargo.lock should be included in library crates.1

Footnotes

1. [Why have Cargo.lock in version control?](https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control) and [Verifying Latest Dependencies](https://doc.rust-lang.org/cargo/guide/continuous-integration.html#verifying-latest-dependencies) [↩](#user-content-fnref-1-118f1ffbc303dcbda1266df9335b8025)

However, this determinism can give a false sense of security because Cargo.lock does not affect the consumers of your package, only Cargo.toml does that.

Users of a lib crate will use any combination of versions that are valid according to the Cargo.toml file. Proving that the build succeeds for some frozen combination never provided any value in the past. Generating the Cargo.lock file at the time of the build is sufficient.

For bin crates the situation is different and the Cargo.lock file is mandatory.

uklotzde commented 4 months ago

None of the major infrastructure crates like tokio and futures maintain Cargo.lock files in their repos.

sgoll commented 4 months ago

However, this determinism can give a false sense of security because Cargo.lock does not affect the consumers of your package, only Cargo.toml does that.

Yes, that's obviously the case. The benefit of including Cargo.lock is that we as developers can keep track of a consistent set of dependency versions. And in addition, we can make sure that older versions of our dependencies continue working – we don't accidentally and without need bump the version numbers that users of our library will need to use.

To mitigate that false sense of security, we check that we stay compatible with the latest versions (both Rust versions and dependency versions) with the CI action latest-dependencies.

This is a combination of recommended approaches no. 2 and no. 3 from Verifying Latest Dependencies:

  1. Have a CI job verify the latest dependencies but mark it to “continue on failure”
    • Depending on the CI service, failures might not be obvious
    • Depending on PR velocity, may use more resources than necessary
  2. Have a scheduled CI job to verify latest dependencies
    • A hosted CI service may disable scheduled jobs for repositories that haven’t been touched in a while, affecting passively maintained packages
    • Depending on the CI service, notifications might not be routed to people who can act on the failure
    • If not balanced with dependency publish rate, may not test enough versions or may do redundant testing

We can mitigate the caveats of these approaches by making sure that failures reach us (we receive GitHub notifications when CI runs fail) and keep the check running at a sensible interval (on pull requests and merges, plus once a week).

In fact, the side effects of approach no. 1 are what keeps me from omitting Cargo.lock from the repository:

  1. Not checking in the Cargo.lock
    • Depending on PR velocity, many versions may go untested
    • This comes at the cost of determinism

I like having determinism to rule out unrelated causes of errors. It's much easier to check whether a build fails with a reproducible environment (with Cargo.lock) or due to incompatibility with latest versions (latest-dependencies).

Users of a lib crate will use any combination of versions that are valid according to the Cargo.toml file. Proving that the build succeeds for some frozen combination never provided any value in the past. Generating the Cargo.lock file at the time of the build is sufficient.

I am curious: what good is generating the Cargo.lock at the time of the build? If we leave it out, why do we need it at all?

For bin crates the situation is different and the Cargo.lock file is mandatory.

We are not creating a bin crate but the recommendations no longer differentiate between bin and lib crates anyway.

None of the major infrastructure crates like tokio and futures maintain Cargo.lock files in their repos.

I suspect this may be much more a matter of not needlessly wanting to adjust to the changed recommended practice than a conscious decision. Until a year or so ago, the recommendation was to omit Cargo.lock from lib crates. This has changed, as long as we can ensure that we test compatibility with latest dependency versions in some other way – which we do.

That said, I'd like to keep Cargo.lock in the repository (here and in https://github.com/HMIProject/open62541).

uklotzde commented 4 months ago

I would prefer to follow common practice until there is an actual use case or incident that proofs why we should keep and maintain the Cargo.lock file.

Disagree and commit ;)