Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: include Cargo.lock in version control #39

Closed
wants to merge 1 commit into from

Conversation

jalil-salame
Copy link
Contributor

Note

This does not affect the test server, it will still use the latest version of the dependecies.

This has various benefits described here in the Cargo Book.

I would specially like to highlight:

  • Running git bisect to find the root cause of a bug
  • Ensuring CI only fails due to new commits and not external factors
  • Reducing confusion when contributors see different behavior as compared to other contributors or CI

The main drawbacks are:

  • More likely to get merge conflicts.
  • False sense of security (cargo add and cargo install do not respect Cargo.lock by default).

In my opinion the benefits of deterministic CI outweight the drawbacks of extra merge conflicts.

> [!Note]
> This does not affect the test server, it will still use the latest
> version of the dependecies.

This has various benefits described
[here](https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control)
in the Cargo Book.

I would specially like to highlight:

- Running `git bisect` to find the root cause of a bug
- Ensuring CI only fails due to new commits and not external factors
- Reducing confusion when contributors see different behavior as
  compared to other contributors or CI

The main drawbacks are:

- More likely to get merge conflicts.
- False sense of security (`cargo add` and `cargo install` do not
  respect `Cargo.lock` by default).

In my opinion the benefits of deterministic CI outweight the drawbacks
of extra merge conflicts.

Signed-off-by: Jalil David Salamé Messina <[email protected]>
@jalil-salame jalil-salame requested a review from a team December 7, 2023 13:36
@jalil-salame jalil-salame self-assigned this Dec 7, 2023
@Tim-Zhang
Copy link
Contributor

Tim-Zhang commented Dec 11, 2023

@jalil-salame According to rust-lang/cargo#315 (comment), we should not add Cargo.lock to the repo because tokio-vsock is a library and Cargo.lock in a library is useless.

I think what you want to do is to add Cargo.lock to repo for test_server which is a binary.

@jalil-salame
Copy link
Contributor Author

@jalil-salame According to rust-lang/cargo#315 (comment), we should not add Cargo.lock to the repo because tokio-vsock is a library and Cargo.lock in a library is useless.

That comment is outdated, but looking at Tokio and other big libraries they don't add a Cargo.lock so I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants