publish-lockfile: Various updates

[ehuss]

This is a collection of changes to the publish-lockfile feature. They are separated by commit (see each for more detailed commit messages), and I can separate them into separate PRs if necessary.

• Move publish-lockfile tests to a dedicated file.
• Regenerate Cargo.lock when creating a package.
• Ignore Cargo.lock in cargo install by default (requires --locked to use).
• Add warnings for yanked dependencies.
• Change default of publish-lockfile to true.

[rust-highfive]

r? @alexcrichton

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

[ehuss]

I'm not confident about some of these changes:

• Is it OK to not validate the .crate file can be extracted during cargo package? (This changes to use fs::copy instead.) Failures should only happen if there is a bug in Cargo, and extracting the files is fairly well exercised in the testsuite.
• Does it make sense to ignore Cargo.lock by default during cargo install for ephemeral workspaces only (that is, cargo install without --path)? It's what we decided during a meeting, but it still seems a little iffy to me. The main concern is most users won't know to use that flag if cargo install fails due to a new, broken dependency. Maybe it should print a suggestion to use --locked if cargo install fails and there is an existing Cargo.lock? Also, should cargo install --git ignore it?
• I re-used --locked during cargo install to mean "yes, use existing Cargo.lock". This may be a little strange, because if the Cargo.lock is missing, it proceeds as normal (normally --locked would error). Should there be a separate flag?
• Yank checks:
  • Is it worthwhile to add the yanked checks?
  • The way is_yanked is added to Source seems a little heavy. However, I couldn't see a more lightweight way to query the yanked status.
  • Checking the yanked status during cargo install causes the index to be updated twice. This is because I can't pass the original source into both resolve_ws_with_method and compile_ws. Any ideas how to avoid that?
  • Any performance concerns about running the resolver separately just to check for yanked dependencies?
  • The recommendation to remove --locked if there is a yanked dependency may not work (if a newer, compatible version is not available). Should it maybe run the resolver again without Cargo.lock to check if that suggestion would actually avoid the yanked dependencies?

[alexcrichton]

Is it OK to not validate the .crate file can be extracted during cargo package?

I would naively say "we should keep doing what Cargo was previously doing" in the sense that it's one extra sanity check to ensure the tarball is indeed what we're expecting. That being said though I suspect it's just easier when generating the lock file to have the package structure on disk?

Would it be possible to keep most of the new lockfile resolution off disk? In theory we've already got the information available and I think we should be able to mostly avoid the disk for the resolution phase here.

Does it make sense to ignore Cargo.lock by default during cargo install for ephemeral workspaces only

I would still personally advocate for "yes" in the sense that picking up updates is often quite important for bugfixes and new features. It's just a question of defaults, and the defaults of "ignore the lock file" still feels correct to me vs "respect what's there". I do think there's probably wordsmithing we can do on error messages, and hinting that --locked may fix things is probably a good idea. We could go super extra far and recognize errors that happen in dependencies and not the crate itself, possibly providing an even stronger signal that such a message should be printed.

I think I'd probably say we should be uniform across sources if we can, ignoring lock files everywhere by default. I suspect we can change this over time as necessary though!

Should there be a separate flag?

Eh I don't feel strongly, but using --locked doesn't seem unreasonable to me

[Alex inserts a nonexistent previous question about printing differences in cargo.lock here]

I think the messaging about warning about differences in Cargo.lock may be a bit heavy-handed for publication. Printing a bold yellow "warning" for normal situations like path dependencies definitely feels a bit extreme to me, although I could see how a warning may work well for [patch]. I'm sort of tempted though to leave out these warnings for now (maybe relegate them to informational -v messages rather than warnings). I'm curious though of the use case you're thinking of when including them?

Is it worthwhile to add the yanked checks?

This I think may be a function of the defaults of --locked or not. If we default to --locked then this definitely feels worthwhile because otherwise it's too easy to silently use yanked packages for one reason or another.

If --locked is not the default, however, I think that we may want to avoid the warnings. You've already got to explicitly opt-in to --locked otherwise and I feel like that's typically motivated by something like:

• You previously got a build failure without it so you're using it. In this case the warning won't really do anything other than be possibly annoying, but you're probably unlikely to take action from it.
• Documentation told you to use --locked, and since you're copy/pasting you have no idea what to do with the warning.
• You're otherwise somewhat paranoid and always use --locked. If you're in this category you're probably the kind of person to go open up an issue, so the warning is probably actionable in this use case.

I think overall though I feel like the warnings may be a bit much, but others of course may feel differently :)

[ehuss]

Would it be possible to keep most of the new lockfile resolution off disk?

I had considered that. The issue is that it needs the new, rewritten Cargo.toml to use for resolution. I was concerned this would require significant changes since a lot of code assumes the files are on disk. I'll take a look at seeing how difficult that will be. Perhaps I can just pass it through ephemeral() and it will all just work.

I think the messaging about warning about differences in Cargo.lock may be a bit heavy-handed for publication.

I agree. I was just thinking it would be good to know that something was added to the lock file. I'll dial it back.

[...yanked warnings...]

I agree with the concern that the warnings can be confusing or unactionable. My thinking is that using --locked would be rare, and having yanked dependencies would also be rare, and together it shouldn't happen often. The primary use case I had in mind was in a CI system, if you had cargo install mdbook --locked --version=0.2.3 because you want reliable builds. However, you would also like to know if what you are installing possibly has a bad dependency and you should probably upgrade (or ask the project to update their dependencies).

In general, I feel like Cargo has little-to-no communication to the user about yanked crates, which I think could be improved, and this would be a minor step in that direction. Do you think there's a better way to communicate this? In #2608 you mentioned warning every time a yanked dependency is built which would happen much more often.

[ehuss]

I have included some updates to address some of the things mentioned above. It now generates the Cargo.lock in memory and retains the old behavior of extracting the tar file to verify (build_lock). It is not super clean, but I think shouldn't be too hard to follow.

[alexcrichton]

It is not super clean, but I think shouldn't be too hard to follow.

Nice! I suspect this can be improved over time as well if necessary, but otherwise it seems pretty reasonable as-is to me.

In general, I feel like Cargo has little-to-no communication to the user about yanked crates, which I think could be improved, and this would be a minor step in that direction.

This is a pretty compelling point to me, as is the rationale to the rarity of the warnings. I think I'm convinced that this seems like a good balance for now, and we can of course change course later!

In #2608 you mentioned warning every time a yanked dependency is built which would happen much more often.

Heh I had forgotten about this! It actually does seem like a good rule of thumb that when a yanked package is "used" (for some definition of the word "used") Cargo emits a warning. For example if you built a yanked package, that's using it. Here if you put it in a lock file going into the registry, that's using it. Similarly we'd warn on using it during cargo install (although we probably wouldn't want double warnings from both cargo install and building the crate itself).

In any case, I think we should go with what you've got implemented here!

---

As one final note apart from the one nit below, these warnings are all printed on cargo publish --dry-run, right?

[ehuss]

Updated with the suggestions. The last two commits seem a little hacky to me, but it seems to work. The last one just removes the source passed in to preload. I think it is no longer necessary, but I can easily revert that last commit if we want to keep it.

these warnings are all printed on cargo publish --dry-run, right?

Correct.

[ehuss]

Also, I had to remove out_of_date_lock_note because the Cargo.lock is now unconditionally updated, so that condition doesn't happen anymore. I feel like there should be some kind of warning if Cargo.lock changes if it is checked in to git (currently it is ignored). I could maybe rearrange things so that build_lock is called before ops::resolve_ws to restore that warning? Since they only show up with -v it's probably not too critical, but it seems a little strange that Cargo.lock can be modified without notice.

[alexcrichton]

Nice! That all looks great to me.

IIRC we already have a "VCS must be clean" check before publication, and I'd imagine that if the Cargo.lock was tracked and we update it then it'd be caught by that check, but maybe the check needs to be sequenced later in the publication process? I do think we'll want to trigger that check when we update Cargo.lock and it's tracked in VCS

[ehuss]

OK, the last commit adds a git check for Cargo.lock if it is tracked.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 7d20230 has been approved by alexcrichton

[bors]

⌛ Testing commit 7d20230 with merge 96e9496...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 96e9496 to master...