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

epage · 2023-07-19T16:49:16Z

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

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

When the guide was written, it only included Travis CI. For while, it was *the* CI service people used. However, since git hosting services have been integrating CI support and with Travis CI's new owners, market share has been going down. One estimate I found said that Travis CI's market share is 0.94%. If we had a compelling reason to include it, independent of that, I would. However, including it and making it first in the list is a distraction. There are CI services in more common use (e.g. CircleCI, Jenkins) to include at this point over Travis.

rustbot · 2023-07-19T16:49:21Z

r? @weihanglo

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

epage · 2023-07-19T16:49:44Z

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

Before, we were fairly prescriptive of when to check-in the `Cargo.lock` file. The guidance has been updated to instead explain the trade offs and to mostly leave it in users hands. As a starting point, we do say to check it in for those that want an "easy answer".

Following the default guidance to commit a lockfile, this updates `cargo new` to do it by default.

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 · 2023-07-19T17:07:35Z

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.

src/doc/src/faq.md

Turbo87

I very much support everything in this PR! ❤️

src/doc/src/faq.md

epage · 2023-07-19T19:10:38Z

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 · 2023-07-24T20:19:07Z

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

Nemo157 · 2023-07-24T21:13:56Z

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 · 2023-08-21T20:53:41Z

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

weihanglo · 2023-08-21T21:29:54Z

Ed, thanks for your effort to this! Also thanks everyone for providing lots of useful practices and considerations. Hold on. We're now full speed ahead!

@bors r+

bors · 2023-08-21T21:29:56Z

📌 Commit 54ad4a0 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-08-21T22:19:57Z

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

bors · 2023-08-21T23:13:25Z

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

weihanglo · 2023-08-22T08:57:18Z

src/doc/src/guide/cargo-toml-vs-cargo-lock.md

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


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

This is a follow up to rust-lang#12382

docs(guide): Generalize over VCSs This is a follow up to #12382

Marcono1234

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

Marcono1234 · 2023-09-06T19:56:36Z

src/doc/src/guide/continuous-integration.md

+  - 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:


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

Marcono1234 · 2023-09-06T20:01:09Z

src/doc/src/guide/continuous-integration.md

@@ -1,25 +1,8 @@
 ## Continuous Integration

-### Travis CI
+### Getting Started


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 · 2023-09-06T20:19:54Z

Fixed these in #12630

docs(guide): Apply feedback on CI See #12382

Including Cargo.lock is recommended now to get reproducible builds, see rust-lang/cargo#12382

epage added 3 commits July 19, 2023 10:46

docs(ci): Add section heading to open the way for more headings

b9216c1

docs(ci): Remove unused link

eb1a257

rustbot assigned weihanglo Jul 19, 2023

epage marked this pull request as draft July 19, 2023 16:49

rustbot added A-documenting-cargo-itself Area: Cargo's documentation Command-new S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2023

epage mentioned this pull request Jul 19, 2023

Check Cargo.lock in version control for libraries #8728

Closed

epage and others added 2 commits July 19, 2023 11:55

fix(new): Don't ignore Cargo.lock for all package types

54ad4a0

Following the default guidance to commit a lockfile, this updates `cargo new` to do it by default.

epage force-pushed the lock branch from 8f2ff1e to 9ee6270 Compare July 19, 2023 16:56

weihanglo reviewed Jul 19, 2023

View reviewed changes

joshtriplett reviewed Jul 19, 2023

View reviewed changes

src/doc/src/faq.md Outdated Show resolved Hide resolved

joshtriplett reviewed Jul 19, 2023

View reviewed changes

src/doc/src/faq.md Outdated Show resolved Hide resolved

Turbo87 approved these changes Jul 19, 2023

View reviewed changes

epage force-pushed the lock branch from 9ee6270 to 46b8614 Compare July 19, 2023 17:51

joshtriplett reviewed Jul 19, 2023

View reviewed changes

src/doc/src/faq.md Outdated Show resolved Hide resolved

epage force-pushed the lock branch from 46b8614 to 6f560c5 Compare July 19, 2023 19:09

epage force-pushed the lock branch from 6f560c5 to c3986f3 Compare July 22, 2023 14:52

kpreid mentioned this pull request Jul 23, 2023

Initial release danielhenrymantilla/maybe-dangling.rs#1

Merged

epage force-pushed the lock branch from c3986f3 to 54ad4a0 Compare August 21, 2023 20:51

epage marked this pull request as ready for review August 21, 2023 20:52

epage force-pushed the lock branch from 90be6ba to 54ad4a0 Compare August 21, 2023 21:16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2023

bors merged commit 3bb02f2 into rust-lang:master Aug 21, 2023
35 of 37 checks passed

weihanglo reviewed Aug 22, 2023

View reviewed changes

epage deleted the lock branch August 22, 2023 16:45

epage added a commit to epage/cargo that referenced this pull request Aug 22, 2023

docs(guide): Generalize over VCSs

a3dc5a1

This is a follow up to rust-lang#12382

epage mentioned this pull request Aug 22, 2023

docs(guide): Generalize over VCSs #12541

Merged

bors added a commit that referenced this pull request Aug 22, 2023

Auto merge of #12541 - epage:fix, r=weihanglo

4df5a17

docs(guide): Generalize over VCSs This is a follow up to #12382

weihanglo mentioned this pull request Aug 25, 2023

docs(faq): Recommend always committing Cargo.lock #12275

Closed

epage mentioned this pull request Aug 25, 2023

Commit the Cargo.lock file to build with known working versions of dependencies mozilla/uniffi-rs#1720

Merged

Marcono1234 reviewed Sep 6, 2023

View reviewed changes

epage mentioned this pull request Sep 6, 2023

docs(guide): Apply feedback on CI #12630

Merged

bors added a commit that referenced this pull request Sep 6, 2023

Auto merge of #12630 - epage:ci, r=arlosi

84544d4

docs(guide): Apply feedback on CI See #12382

Marcono1234 added a commit to Marcono1234/struson that referenced this pull request Sep 23, 2023

Update thiserror dependency & add Cargo.lock to Git

009dd5d

Including Cargo.lock is recommended now to get reproducible builds, see rust-lang/cargo#12382

workingjubilee mentioned this pull request Oct 6, 2023

Check in Cargo.lock amatho/mupen64plus-input-gca#17

Merged

ehuss added this to the 1.74.0 milestone Oct 11, 2023

wilyle mentioned this pull request Oct 12, 2023

Add Cargo.lock eclipse-ibeji/freyja#60

Merged

stinodego mentioned this pull request Nov 5, 2023

chore(python): Add py-polars to Cargo workspace pola-rs/polars#12256

Merged

epage mentioned this pull request Nov 15, 2023

MSRV policy for libc crate rust-lang/libs-team#72

Open

epage mentioned this pull request Nov 28, 2023

Expand "CI best practicies" section to the guide #5656

Open

daira mentioned this pull request Mar 5, 2024

Dependabot security alerts for Rust recommend updating a dependency that is only indirect, which is not the right fix dependabot/dependabot-core#9210

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

epage commented Jul 19, 2023 •

edited

rustbot commented Jul 19, 2023

epage commented Jul 19, 2023

weihanglo left a comment

epage commented Jul 19, 2023

Turbo87 left a comment

epage commented Jul 19, 2023

bors commented Jul 24, 2023

Nemo157 commented Jul 24, 2023

epage commented Aug 21, 2023

weihanglo commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

weihanglo Aug 22, 2023

Marcono1234 left a comment

Marcono1234 Sep 6, 2023

Marcono1234 Sep 6, 2023

epage commented Sep 6, 2023

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

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

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

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

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

Conversation

epage commented Jul 19, 2023 • edited

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Jul 19, 2023

epage commented Jul 19, 2023

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Jul 19, 2023

Turbo87 left a comment

Choose a reason for hiding this comment

epage commented Jul 19, 2023

bors commented Jul 24, 2023

Nemo157 commented Jul 24, 2023

epage commented Aug 21, 2023

weihanglo commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

weihanglo Aug 22, 2023

Choose a reason for hiding this comment

Marcono1234 left a comment

Choose a reason for hiding this comment

Marcono1234 Sep 6, 2023

Choose a reason for hiding this comment

Marcono1234 Sep 6, 2023

Choose a reason for hiding this comment

epage commented Sep 6, 2023

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

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

epage commented Jul 19, 2023 •

edited