cargo package --workspace must support publish = false

[torhovland]

Fixes #14356.

OK, so the issue can be fixed quite easily. But I'm not sure if it is the correct fix. So in this PR we are simply skipping validating the published-to registry if publish = false when inferring what registry to use. But that means we allow any workspace where one of the packages, possibly a crucial dependency, is configured to not get published. So the final, published packages may not work for anybody.

Do we need a smarter validation, that only allows this for packages that are not dependencies of other packages that are scheduled to get published?

[rustbot]

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[jneem]

infer_registry has a bunch of problems that are already addressed in #14340, so I think any further fixes should build on that.

Other than that, I think if you also skip putting the publish = false packages in the local overlay then this fix is reasonable. Because that way, verification will fail if and only it would have failed without publishing those packages. (e.g. maybe it's ok not to publish them because the dependency on them can be satisfied by some other already-published version).

[torhovland]

OK, let's regard this one blocked on #14340. Either the issue gets fixed there, or we fix it here when that one is merged.

[epage]

So in this PR we are simply skipping a validation if publish = false. But that means we allow any workspace where one of the packages,

cargo package --workspace should have the same behavior as cargo package -p foo && cargo package -p no-publish. It sounds like this PR is trying to solve this problem by having the behavior diverge (skip the verify step).

btw how are we skipping the verify step?

[jneem]

The "verify" step @torhovland is talking about isn't "the" verify step where you test-build the packages. It's the step where we're verifying that the inferred registry is valid for all the packages.

[epage]

Thanks for the clarification. I've updated the PR description to try to clarify that.

So if I'm understanding the concern, it is that if you depend on a package with publish = false, we won't catch that mistake during the verify step?

imo this seems less of a problem for cargo package and more of a problem for cargo publish which won't allow these to be published anyways.

[jneem]

if you depend on a package with publish = false, we won't catch that mistake during the verify step?

The goal of my suggestion is to replicate the old behavior. There are a few cases, so imagine we have a workspace with main depending on no-publish, and the current local version of no-publish is 0.1.1

• if main depends on no-publish 0.1.0 and no-publish 0.1.0 is already published on crates-io, the verify step will succeed because it will pull in no-publish from crates-io
• if main depends on no-publish 0.1.0 and it isn't published, the verify step will fail: since no-publish is marked as not publishable, it won't be put in the local registry overlay, and so when verifying main we won't find it
• if main depends on no-publish 0.1.1 but only 0.1.0 is published on crates-io, the verify step will fail for the same reason

[bors]

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

[epage]

I'm not too worried about the exact behavior when a published package depends on an unpublished package that changes state between unpublishable and publishable. That feels like a corner case not worth designing towards. If anything, I think I'd prefer to err on the side of allowing it to be packaged (since the publish state is independent of packaging). We could note it in the issue for consderation when stabilizing.

[torhovland]

Superseded by #14408.