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

[epage]

What does this PR try to resolve?

Having libraries leave Cargo.lock "on the float" has been serving the ecosystem well, including

• Encouraging people to validate that latest dependencies work
• Encouraging ecosystem-wide health by acting as a "distributed crater"

These benefits are limited though. The policy is inconsistent between workspaces with or without [[bin]]s, reducing the affect of testing the latest. This is also subject to when CI last ran; for passively maintained projects, there is little coverage of new dependencies.

There are also costs associated with this policy

• git bisect is using an unpredictable set of dependencies, affecting the ability to identify root cause
• This is another potential cause for Red CI / broken local development if version is yanked or a bug is introduced
  • Impacting the perceived level of quality for a project
  • Confusing to new contributors who might not recognize why CI failed and assume its their fault
  • Requiring context switching from maintainers to get fixes in

In particular, since this policy was decided, there has been an increased interest in supporting an MSRV (as recently as v1.56.0, cargo gained support for specifying a package's MSRV). This has led to long discussions on what MSRV a package should use (e.g. rust-lang/libs-team#72,. time-rs/time#535). Worst, there has been a growing trend for people to set an non-semver upper bound on dependencies, making it so packages can't work well with other packages (see #12323). Tooling support would help with this (#9930) but the sooner we address this, the less entrenched bad practices will be.

On the positive side, since the policy was decided

• In general, CI became easier to setup and maintain with Github Actions compared to TravisCI
• Dependabot went GA on Github in 2021 (https://github.blog/changelog/2021-03-31-dependabot-version-updates-are-now-generally-available)
  • I believe Dependabot will post security update PRs even when Dependabot is not more generally enabled

So to get some of the benefit from not checking in Cargo.lock, we can recommend either automatically applying updates or having CI check the latest dependencies in a way to get this out of the critical path of PRs and releases.

Since there is no one right answer on how to solve all of these problems, we're documenting these trade offs so people can make the choice that is most appropriate for them. However, we are changing the default to a consistent "always check it in" as the answer for those who don't want to think about it.

Prior art

• Yarn (Javascript)
• Poetry (Python)
• Bundler (Ruby)

Fixes #8728

How should we test and review this PR?

Please review per-commit. I tried to minimize changes I made to the structure of the CI document

In #8728, I brought up having a CI reference page. I keep going back and forth on whether this is guide-level material or reference-level material. Obviously, right now I'm leaning towards it being guide-level.

Additional information

This changes cargo from telling people what to do to giving them a starting point, or default, and giving them the information to make their own choice, if needed.

So the question for defaults is who are we targeting? For a default path in documentation, it would be for new to intermediate users. As for cargo new, we've been prioritizing new users over those that run it frequently (boiler plate comment, bin is default, etc).

See #8728 for the FCP on this policy change

[rustbot]

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

[epage]

I've marked this as a draft until the FCP is closed.

[weihanglo]

Haven't really looked into it yet. Should we take care of source files traversal and cargo package as well?

• cargo/src/cargo/ops/cargo_package.rs

  Lines 268 to 274 in 1b15556

  	if pkg.include_lockfile() {
  	result.push(ArchiveFile {
  	rel_path: PathBuf::from("Cargo.lock"),
  	rel_str: "Cargo.lock".to_string(),
  	contents: FileContents::Generated(GeneratedFile::Lockfile),
  	});
  	}

• cargo/src/cargo/sources/path.rs

  Lines 182 to 186 in 1b15556

  	if rel == "Cargo.lock" {
  	return pkg.include_lockfile();
  	} else if rel == "Cargo.toml" {
  	return true;
  	}

[epage]

I believe both of those cases are more about what your users have access to. In theory, users of a lib should not be affected by a Cargo.lock, so leaving it out should still be fine.

[Turbo87]

I very much support everything in this PR! ❤️

[epage]

Forgot to add, this is not just a draft until the FCP window closes but the cargo team was also wanting to wait on this until after the next beta branch so we get 12 weeks of feedback before this hits stable.

[bors]

☔ The latest upstream changes (presumably #12397) made this pull request unmergeable. Please resolve the merge conflicts.

[Nemo157]

I believe both of those cases are more about what your users have access to. In theory, users of a lib should not be affected by a Cargo.lock, so leaving it out should still be fine.

There are some usecases that use packaged Cargo.lock when building a library, like docs.rs, we attempt to build with the provided lockfile as we assume that gives the best chance of success, then delete it and try again if it fails (for packages that contain both library and binary crates and so distribute their lockfile). I also semi-regularly download and extract packages source code to inspect issues before going to find a repo, having a lockfile there can be useful.

[epage]

Looks like rust-1.73.0 has been branched so I assume we are good to merge.

[bors]

⌛ Testing commit 54ad4a0 with merge 3bb02f2...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 3bb02f2 to master...

[weihanglo]

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).

[Marcono1234]

Sorry for adding a review this late; I hope the comments are useful nonetheless.

[Marcono1234]

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

[Marcono1234]

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

[epage]

Fixed these in #12630