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

fix: Change the defaults to always check-in Cargo.lock #12382

Merged
merged 5 commits into from
Aug 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/cargo/ops/cargo_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ struct MkOptions<'a> {
path: &'a Path,
name: &'a str,
source_files: Vec<SourceFileInformation>,
bin: bool,
edition: Option<&'a str>,
registry: Option<&'a str>,
}
Expand Down Expand Up @@ -448,7 +447,6 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
path,
name,
source_files: vec![plan_new_source_file(opts.kind.is_bin(), name.to_string())],
bin: is_bin,
edition: opts.edition.as_deref(),
registry: opts.registry.as_deref(),
};
Expand Down Expand Up @@ -553,7 +551,6 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<NewProjectKind> {
version_control,
path,
name,
bin: has_bin,
source_files: src_paths_types,
edition: opts.edition.as_deref(),
registry: opts.registry.as_deref(),
Expand Down Expand Up @@ -745,9 +742,6 @@ fn mk(config: &Config, opts: &MkOptions<'_>) -> CargoResult<()> {
// for all mutually-incompatible VCS in terms of syntax are in sync.
let mut ignore = IgnoreList::new();
ignore.push("/target", "^target$", "target");
if !opts.bin {
ignore.push("/Cargo.lock", "^Cargo.lock$", "Cargo.lock");
}

let vcs = opts.version_control.unwrap_or_else(|| {
let in_existing_vcs = existing_vcs_repo(path.parent().unwrap_or(path), config.cwd());
Expand Down
64 changes: 39 additions & 25 deletions src/doc/src/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,33 +102,47 @@ issue][cargo-issues].

[cargo-issues]: https://github.com/rust-lang/cargo/issues

### Why do binaries have `Cargo.lock` in version control, but not libraries?
### Why have `Cargo.lock` in version control?

While [`cargo new`] defaults to tracking `Cargo.lock` in version control,
whether you do is dependent on the needs of your package.

The purpose of a `Cargo.lock` lockfile is to describe the state of the world at
the time of a successful build. Cargo uses the lockfile to provide
deterministic builds on different times and different systems, by ensuring that
the exact same dependencies and versions are used as when the `Cargo.lock` file
was originally generated.

This property is most desirable from applications and packages which are at the
very end of the dependency chain (binaries). As a result, it is recommended that
all binaries check in their `Cargo.lock`.

For libraries the situation is somewhat different. A library is not only used by
the library developers, but also any downstream consumers of the library. Users
dependent on the library will not inspect the library’s `Cargo.lock` (even if it
exists). This is precisely because a library should **not** be deterministically
recompiled for all users of the library.

If a library ends up being used transitively by several dependencies, it’s
likely that just a single copy of the library is desired (based on semver
compatibility). If Cargo used all of the dependencies' `Cargo.lock` files,
then multiple copies of the library could be used, and perhaps even a version
conflict.

In other words, libraries specify SemVer requirements for their dependencies but
cannot see the full picture. Only end products like binaries have a full
picture to decide what versions of dependencies should be used.
the time of a successful build.
Cargo uses the lockfile to provide deterministic builds at different times and
on different systems,
by ensuring that the exact same dependencies and versions are used as when the
`Cargo.lock` file was originally generated.

Deterministic builds help with
- 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

Having this snapshot of dependencies can also help when projects need to be
verified against consistent versions of dependencies, like when
- Verifying a minimum-supported Rust version (MSRV) that is less than the latest
version of a dependency supports
- Verifying human readable output which won't have compatibility guarantees
(e.g. snapshot testing error messages to ensure they are "understandable", a
metric too fuzzy to automate)

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.
For example:
- [`cargo install`] will select the latest dependencies unless `--locked` is
passed in.
- New dependencies, like those added with [`cargo add`], will be locked to the latest version

The lockfile can also be a source of merge conflicts.

For strategies to verify newer versions of dependencies via CI,
see [Verifying Latest Dependencies](guide/continuous-integration.md#verifying-latest-dependencies).

[`cargo new`]: commands/cargo-new.md
[`cargo add`]: commands/cargo-add.md
[`cargo install`]: commands/cargo-install.md

### Can libraries use `*` as a version for their dependencies?

Expand Down
13 changes: 5 additions & 8 deletions src/doc/src/guide/cargo-toml-vs-cargo-lock.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,11 @@ about them, here’s a summary:
* `Cargo.lock` contains exact information about your dependencies. It is
maintained by Cargo and should not be manually edited.

If you’re building a non-end product, such as a rust library that other rust
[packages][def-package] will depend on, put `Cargo.lock` in your
`.gitignore`. If you’re building an end product, which are executable like
command-line tool or an application, or a system library with crate-type of
`staticlib` or `cdylib`, check `Cargo.lock` into `git`. If you're curious
about why that is, see
["Why do binaries have `Cargo.lock` in version control, but not libraries?" in the
FAQ](../faq.md#why-do-binaries-have-cargolock-in-version-control-but-not-libraries).
When in doubt, check `Cargo.lock` into git.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found this. We may use version control system instead.

Suggested change
When in doubt, check `Cargo.lock` into git.
When in doubt, check `Cargo.lock` into the version control system (e.g. Git).

For a better understanding of why and what the alternatives might be, see
[“Why have Cargo.lock in version control?” in the FAQ](../faq.md#why-have-cargolock-in-version-control).
We recommend pairing this with
[Verifying Latest Dependencies](continuous-integration.md#verifying-latest-dependencies)

Let’s dig in a little bit more.

Expand Down
77 changes: 57 additions & 20 deletions src/doc/src/guide/continuous-integration.md
Original file line number Diff line number Diff line change
@@ -1,25 +1,8 @@
## Continuous Integration

### Travis CI
### Getting Started
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this header here really needed? And also, in general maybe the header levels have to be adjusted to be semantically more useful?

Currently the header hierarchy looks like this:

  • Continuous Integration
    • Getting Started
    • GitHub Actions
    • GitLab CI
    • builds.sr.ht
    • Verifying Latest Dependencies

Maybe something like this would be better?

  • Continuous Integration
    • <no sure how to name this>
      • GitHub Actions
      • GitLab CI
      • builds.sr.ht
    • Verifying Latest Dependencies


To test your [package][def-package] on Travis CI, here is a sample
`.travis.yml` file:

```yaml
language: rust
rust:
- stable
- beta
- nightly
matrix:
allow_failures:
- rust: nightly
```

This will test all three release channels, but any breakage in nightly
will not fail your overall build. Please see the [Travis CI Rust
documentation](https://docs.travis-ci.com/user/languages/rust/) for more
information.
A basic CI will build and test your projects:

### GitHub Actions

Expand Down Expand Up @@ -122,4 +105,58 @@ channel, but any breakage in nightly will not fail your overall build. Please
see the [builds.sr.ht documentation](https://man.sr.ht/builds.sr.ht/) for more
information.

[def-package]: ../appendix/glossary.md#package '"package" (glossary entry)'
### Verifying Latest Dependencies

When [specifying dependencies](../reference/specifying-dependencies.md) in
`Cargo.toml`, they generally match a range of versions.
Exhaustively testing all version combination would be unwieldy.
Verifying the latest versions would at least test for users who run [`cargo
add`] or [`cargo install`].

When testing the latest versions some considerations are:
- Minimizing external factors affecting local development or CI
- Rate of new dependencies being published
- Level of risk a project is willing to accept
- CI costs, including indirect costs like if a CI service has a maximum for
parallel runners, causing new jobs to be serialized when at the maxium.

Some potential solutions include:
- [Not checking in the `Cargo.lock`](../faq.md#why-have-cargolock-in-version-control)
- Depending on PR velocity, many versions may go untested
- This comes at the cost of determinism
- 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
- 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
- Regularly update dependencies through PRs, like with [Dependabot] or [RenovateBot]
- Can isolate dependencies to their own PR or roll them up into a single PR
- Only uses the resources necessary
- Can configure the frequency to balance CI resources and coverage of dependency versions

An example CI job to verify latest dependencies, using Github Actions:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really minor but this should probably be "GitHub" (with capital "H"). That would also be consistent with the other references in this file.

```yaml
jobs:
latest_deps:
name: Latest Dependencies
runs-on: ubuntu-latest
continue-on-error: true
steps:
- uses: actions/checkout@v3
- run: rustup update stable && rustup default stable
- run: cargo update --verbose
- run: cargo build --verbose
- run: cargo test --verbose
```
For projects with higher risks of per-platform or per-Rust version failures,
more combinations may want to be tested.

[`cargo add`]: ../commands/cargo-add.md
[`cargo install`]: ../commands/cargo-install.md
[Dependabot]: https://docs.github.com/en/code-security/dependabot/working-with-dependabot
[RenovateBot]: https://renovatebot.com/
1 change: 0 additions & 1 deletion tests/testsuite/cargo_init/auto_git/out/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
/target
/Cargo.lock
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
target
Cargo.lock
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
target
Cargo.lock
1 change: 0 additions & 1 deletion tests/testsuite/cargo_init/git_autodetect/out/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
/target
/Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,3 @@
# Added by cargo

/target
/Cargo.lock
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
/target
/Cargo.lock
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
^target$
^Cargo.lock$
1 change: 0 additions & 1 deletion tests/testsuite/cargo_init/pijul_autodetect/out/.ignore
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
/target
/Cargo.lock
1 change: 0 additions & 1 deletion tests/testsuite/cargo_init/simple_git/out/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
/target
/Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,3 @@
# already existing elements were commented out

#/target
/Cargo.lock
1 change: 0 additions & 1 deletion tests/testsuite/cargo_init/simple_hg/out/.hgignore
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
^target$
^Cargo.lock$
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,3 @@
# Added by cargo

^target$
^Cargo.lock$
4 changes: 2 additions & 2 deletions tests/testsuite/new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ fn simple_git() {

let fp = paths::root().join("foo/.gitignore");
let contents = fs::read_to_string(&fp).unwrap();
assert_eq!(contents, "/target\n/Cargo.lock\n",);
assert_eq!(contents, "/target\n",);

cargo_process("build").cwd(&paths::root().join("foo")).run();
}
Expand All @@ -112,7 +112,7 @@ fn simple_hg() {

let fp = paths::root().join("foo/.hgignore");
let contents = fs::read_to_string(&fp).unwrap();
assert_eq!(contents, "^target$\n^Cargo.lock$\n",);
assert_eq!(contents, "^target$\n",);

cargo_process("build").cwd(&paths::root().join("foo")).run();
}
Expand Down