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

Package lock files in published crates #5093

Merged
merged 2 commits into from Feb 28, 2018

Conversation

Projects
None yet
5 participants
@alexcrichton
Member

alexcrichton commented Feb 27, 2018

Previously we had logic to explicitly skip lock files but there's actually a
good case to read these from crates.io (#2263) so let's do so!

Closes #2263

Respect lock files in crates.io crates
Currently Cargo doesn't publish lock files in crates.io crates but we'll
eventually be doing so, so this changes Cargo to recognize `Cargo.lock` when
it's published to crates.io as use it as the basis for resolution during `cargo
install`.

cc #2263
@rust-highfive

This comment has been minimized.

rust-highfive commented Feb 27, 2018

r? @matklad

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

@alexcrichton alexcrichton force-pushed the alexcrichton:include-cargo-lock branch from 9f6bd50 to 3e63f93 Feb 27, 2018

@matklad

This comment has been minimized.

Member

matklad commented Feb 27, 2018

The changes look good to me, however I am not sure that literally pushing the lock file would do what we want here...

Specifically, I believe this fails for workspaces, which have the lockfile only for the root, which may be virtual and not published at all.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 28, 2018

Hm do you think we should always copy in the workspace's lock file unconditionally? That should cover that issue I think?

@matklad

This comment has been minimized.

Member

matklad commented Feb 28, 2018

Hm do you think we should always copy in the workspace's lock file unconditionally? That should cover that issue I think?

I am not sure... Will workspace lockfile work at all for a subpackage? Perhaps we should use the lockfle that we get when we build a package during a validation step of cargo publish?

I am also a little bit unsure about the "unconditionally" fact: perhaps the user should be explicitly aware that the lockfile matters for cargo install? Like, the user might want to run cargo update to make sure that lockfile is maximally fresh.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 28, 2018

Inclusion is currently based on gitignore so if you've got a library crate then Cargo.lock shouldn't be included w/ this PR.

I'm not really sure? If we don't know how to do this I'll just close it...

@matklad

This comment has been minimized.

Member

matklad commented Feb 28, 2018

If we don't know how to do this I'll just close it...

Yeah, I am not really sure, for the following reasons:

  • It's not clear what to do with workspaces.
  • Lockfile is added based on "is it in gitignore" and not on "is it a binary crate". Granted, most commonly these are equivalent, but they are not guaranteed to be equivalent.

On the other hand, this literally just removes ignoring "Cargo.lock", and can't possible do any harm, and will be compatible with any fancier solution we can invent.

Yeah, I guess it's fine to r+ it as is (module a failing on windows), because it does solve the problem in 80% of the cases!

Hm, wait... Is it correct that publishing a lockfile won't break a binary? What if a binary is a workspace root, and it has path dependencies? We rewrite Cargo.toml, but we don't rewrite Cargo.lock. Would installing a binary in this situation fail with some kind of lockfile mismatch, or will we just generate a new lockfile? If the letter, let's r+, if the former, let's think about how to solve the workspace problem in a more principle way :)

@alexcrichton alexcrichton force-pushed the alexcrichton:include-cargo-lock branch 2 times, most recently from 5840a76 to 6c7ddaf Feb 28, 2018

Package lock files in published crates
Previously we had logic to explicitly skip lock files but there's actually a
good case to read these from crates.io (#2263) so let's do so!

Closes #2263

@alexcrichton alexcrichton force-pushed the alexcrichton:include-cargo-lock branch from 6c7ddaf to a4a3302 Feb 28, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 28, 2018

Ok I've updated with a few heuristics:

  • Lock files are only included in packages with binaries/examples (those that can be cargo-install'd)
  • Lock files are always copied in from the workspace root
  • Including a lock file is now a nightly feature

That means that cargo publish should work wherever you are in a workspace and always include the lock file regardless of vcs settings. Upon thinking more about this as well it seems like a bad default as it means if you ever want your users to get updated versions of dependencies you've got to publish a new version. In that sense I felt that we'd want this opt-in which requires a new manifest key, hence starting as a nightly feature.

@matklad

This comment has been minimized.

Member

matklad commented Feb 28, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Feb 28, 2018

📌 Commit a4a3302 has been approved by matklad

@bors

This comment has been minimized.

Contributor

bors commented Feb 28, 2018

⌛️ Testing commit a4a3302 with merge f55b326...

bors added a commit that referenced this pull request Feb 28, 2018

Auto merge of #5093 - alexcrichton:include-cargo-lock, r=matklad
Package lock files in published crates

Previously we had logic to explicitly skip lock files but there's actually a
good case to read these from crates.io (#2263) so let's do so!

Closes #2263

@matklad matklad added the relnotes label Feb 28, 2018

@bors

This comment has been minimized.

Contributor

bors commented Feb 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing f55b326 to master...

@bors bors merged commit a4a3302 into rust-lang:master Feb 28, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:include-cargo-lock branch Mar 1, 2018

@phil-opp

This comment has been minimized.

Contributor

phil-opp commented Apr 14, 2018

How do I use this functionality? With the default settings, there is no Cargo.lock on crates.io. With the nightly publish-lockfile manifest key set to true, I can no longer publish my crate:

error: cannot publish crates which activate nightly-only cargo features to crates.io

What am I missing?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 16, 2018

@phil-opp heh I think you've discovered a bug! The tests here only test packaging, not actual publication, which is why that hasn't been exercised before.

@phil-opp

This comment has been minimized.

Contributor

phil-opp commented Apr 19, 2018

@alexcrichton Should I open an issue for this? What would be the best way to resolve this?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 19, 2018

@phil-opp yes, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment