Cargo: Packages for binary crates should include Cargo.lock

[mbrubeck]

Cargo.lock is always excluded from packages generated by cargo package. This means that it can't be used by cargo install when installing from crates.io. Should this be changed for crates that contain binaries, in order to make cargo install more reproducible for these binaries?

[mbrubeck]

Originally reported by @jaxx in https://users.rust-lang.org/t/cargo-install-and-lock-file/4203

[alexcrichton]

Hm yes this seems reasonable to me

[SimonSapin]

for crates that contain binaries

Alternatively include the lock file for all crates, but ignore it when using a dependency.

[alexcrichton]

The relevant line is here and it's there for two reasons:

1. We don't want to consider the Cargo.lock file in terms of "freshness" (e.g. seeing if a package has changed).
2. Historically no one would read it, so we didn't include it. This is of course no longer valid with cargo install being a thing now.

So should just involve fixing it and then ensuring we don't accidentally consider changes to Cargo.lock to rebuild a crate.

[kbknapp]

Just ran into an issue with this in BurntSushi/ripgrep#439

[carols10cents]

Ok so this is currently biting me. I'm trying to publish crates.io on crates.io, to get docs.rs on crates.io's code to help people working on crates.io (bear with me).

Crates.io is an application, really, so its Cargo.lock should be used. In its current state, there's some newer published version of a dependency (not sure which yet) that causes the build to fail. Since cargo package ignores Cargo.lock, cargo build works for me but cargo package fails.

For crates that include both a library and a binary, (ex: bindgen, cargo itself), if we include the Cargo.lock but ignore it sometimes, then there's the possibility that when you build the crate as a binary (and the Cargo.lock is used), the build will succeed, but if you try to use it as a dependency, the build will fail, even though the Cargo.lock is present. I'm concerned that this is surprising behavior but I don't know how to fix it :-/

The comments in BurntSushi/ripgrep#439 are interesting because someone was getting a compiler error and burntsushi wasn't even able to reproduce, because he had a Cargo.lock locally that worked.

My brain is exploding... how does this even work today with just library crates?? When you're working on a library, you have a Cargo.lock locally that gets used for building and tests forever, even though it's gitignored and not published..... I guess this is what CI is for??? Have people run into tests that fail on travis but not locally and the solution to reproduce locally is to delete your local Cargo.lock??

For crates that are both binaries and libraries, should there be a way to request a build as a binary crate using Cargo.lock as well as request a build as a library crate that ignores Cargo.lock?? Without doing cargo package??

It was surprising to me that I could get everything working locally then go to publish and the package verification fails... :(

[steveklabnik]

My brain is exploding... how does this even work today with just library crates?? When you're working on a library, you have a Cargo.lock locally that gets used for building and tests forever, even though it's gitignored and not published..... I guess this is what CI is for??? Have people run into tests that fail on travis but not locally and the solution to reproduce locally is to delete your local Cargo.lock??

FWIW, this is how it works in Ruby as well, and is not generally seen as a major problem.

And yeah, I'd expect CI to catch this kind of thing.

[carols10cents]

And yeah, I'd expect CI to catch this kind of thing.

Except CI can't catch this for crates that are also binaries and check in their Cargo.toml...

[carols10cents]

wait i see that i was talking about libraries there, geez, gotta read my own comments

[alexcrichton]

In general yeah this is somewhat expected and unexpected behavior. It's intended that libraries ignore Cargo.lock on publish because everyone who depends on your library will also ignore your Cargo.lock. In that sense the dependencies listed in Cargo.toml are required to be 100% accurate in terms of accurately reflecting all possible versions of deps you can build a crate with. If that's incorrect then someone depending on your crate may pick the wrong one.

In practice this typically works out as the default dependency is "semver compatible" and the community is in general pretty vigilant about semver updates to prevent this sort of breakage. It's definitely a fine line but it seems to be working out so far?

I know I've personally run into issues locally before where I forget to run cargo update to reproduce issues I'm seeing on CI, although this is relatively rare. I still think, though, that we should implement this issue. Applications/binaries tend to have more restrictive requirements in terms of their deps, either for perf or bugfixes (rather than compiling correctly).

[carols10cents]

So if Cargo.lock is always included in the package, in what cases does verify use it? library crate = never, binary crate = always, library+binary = ???

Or are you saying we should implement the suggestion to only include Cargo.lock with crates that have binaries, rather than unconditionally? In that case, same question, just "in what cases does package include Cargo.lock" instead.

[alexcrichton]

Yeah I'd expect us to always verify with the Cargo.lock provided if it's actually going to get packaged. I wouldn't expect Cargo to have a lib/bin distinction, just a "is this part of the package" check.

[reillysiemens]

Apologies if I'm jumping into the wrong discussion here, but this seems like the most relevant place I've found so far to voice my issue.

4 days ago @Keats published gutenberg v0.0.6. Their repo has both a Cargo.toml and a Cargo.lock. Both specify syntect as a dependency, but the Cargo.toml specifies

syntect = { version = "1", features = ["static-onig"] }

and the Cargo.lock specifies

dependencies = [
 # ...
 "syntect 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
 # ...
]

3 days ago @trishume published syntect v1.4.0 with breaking changes for anyone using custom dumps.

Later, when I went to cargo install gutenberg everything compiled just fine, but I encountered a runtime error when attempting to use gutenberg.

[13:45:46] tucker@khanti:~/Projects/tuckersiemens.com git:(master) $ gutenberg build
Building site...
fatal runtime error: out of memory
[1]    11403 illegal hardware instruction (core dumped)  gutenberg build

This error only occurs when using syntect v1.4.0 and not when using syntect v1.3.0.

Even if @Keats ran cargo update before running tests and publishing their package (which it looks like they did) the end user wouldn't have been protected from this issue since the breaking change was introduced _after_ the package had been published.

I was under the impression that binaries were supposed to check in Cargo.lock to help mitigate problems like this.

[trishume]

Ooops my bad for being a bad person and not following semver. I thought only one person was using custom dumps and I notified them directly, but turns out I was wrong and it broke stuff.

syntect is an interesting case since I expose a ton of things that don't need to be public but are helpful for niche power user cases. My policy so far has been to not bump the major version number if I only change power user features and not the main general use interface. Then I try and keep track of what features all the power users are using, but apparently fail sometimes. Maybe I should reform my weird only-kinda-semver system, but then most casual users won't get updates often, but that may not be a big deal.

[reillysiemens]

To clarify, @trishume, I don't think this makes you even remotely a bad person. 😅 I just think it highlights a case where a binary package author can do their due diligence and things can still go wrong. I think this could happen to any package, not just gutenberg and syntect.

[alexcrichton]

@reillysiemens nah you're definitely right! This issue is just about us actually checking in Cargo.lock to crates.io uploads :)

[boomshroom]

Coming from Nix and NixOS, (I am not affiliated with NixOS, just a user interested in installing crates through the package manager.) this issue seems to be the main reason for not being able to install a crate straight from crates.io with a Nix expression. Due to the desire for reproducible builds, the default procedure for installing a crate depends on the lock file to get the right dependencies and any attempts to use a version without the lock file get rejected.

Given that cargo also emphasizes reproducible builds, this change should improve that as well.

[bluetech]

Here is what npm does, since version 5. I'm not saying if it's a good idea or not.

A package can have one of 2 lock files:

• package-lock.json - similar to Cargo.lock - never published.
• npm-shrinkwrap.json - can be published. If published, will be respected even for nested dependencies/libraries.

The two files have the same format.

With this, a binary package, or even a library package which requires exact versions for some reason, can use npm-shrinkwrap.json and publish it. While all other packages use package-lock.json which is the default.

[aturon]

cc @rust-lang/cargo, we should prioritize this. Came up in the CLI working group.

[reillysiemens]

@aturon: I would love to see this get prioritized. :grin: This is one of the reasons @Keats stopped distributing Gutenberg via crates.io (which was my preferred installation method) and ultimately yanked the crate: https://github.com/Keats/gutenberg/issues/117

I can sympathize as it seems like distributing a Rust CLI as a binary crate with this issue outstanding is too likely to generate support issues for authors to make it worth it.

[Xaeroxe]

@trishume

syntect is an interesting case since I expose a ton of things that don't need to be public but are helpful for niche power user cases. My policy so far has been to not bump the major version number if I only change power user features and not the main general use interface. Then I try and keep track of what features all the power users are using, but apparently fail sometimes. Maybe I should reform my weird only-kinda-semver system, but then most casual users won't get updates often, but that may not be a big deal.

This is a good place for a helper "*core" crate similar to how rayon does it. You might take a look at rayon and rayon-core and decide if such a thing could be useful for you.

[mgeisler]

Have people run into tests that fail on travis but not locally and the solution to reproduce locally is to delete your local Cargo.lock??

@carols10cents, yes, that happens all the time for me! :-) It even happens that my Travis builds stay green due to caching, but builds fail for other users. In my case, the problem is crates changing the version of rustc they need without bumping the version enough to make Cargo care about it.

The most recent example is here: https://github.com/mgeisler/version-sync/issues/39 and I started a thread about how to best handle this situation on the users' forum: https://users.rust-lang.org/t/impact-of-pinning-dependencies/17634

(Just trying to direct people to other relevant cases if they also end up here like me.)