Publish workspace by jneem · Pull Request #14433 · rust-lang/cargo (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Adds support for simultaneously publishing multiple (possibly inter-dependent) packages in a workspace, gated by the -Zpackage-workspace flag.

Questions to be worked out through stabilization:

Are we ok stabilizing this and cargo package --workspace is not very useful #10948 at the same time? Currently, they are behind the same flag
What is the desired behavior for the publish timeout? This PR uploads the crates in batches (depending on the dependency graph), and we only timeout if nothing in the batch is available within the timeout, deferring the rest to the next wait-for-publish. So for example, if you have packages a, b, c then we'll wait up to 60 seconds and if only a and b were ready in that time, we'll then wait another 60 seconds for c.
What is the desired behavior when some packages in a workspace have publish = false? This PR raises an error whenever any of the selected packages has publish = false, so it will error on cargo publish --workspace in a workspace with an unpublishable package. An alternative interface would implicitly exclude unpublishable packages in this case, but still error out if you explicitly select an unpublishable package with -p package-name (see -Zpackage-workspace is not smart about publish = false #14356). This PR's behavior is the most conservative one as it can change from an error to implicit excludes later.

This is part of #1169

r? @ehuss

rustbot has assigned @ehuss.
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

Is it worth adding a new -Zpublish-workspace flag, or is it ok to reuse the old one?

I don't have enough context to know if -Zpackage-workspace was intended as a stepping-stone for publishing, or if there really is an independent use-case for packaging versus publishing.

This breaks the publish_namespaced test, because we now try to parse all packages as package specs, and "foo::bar" fails to parse. This could probably be avoided by a refactor (adding a different package function that takes a list of packages instead of trying to get them from cli arguments), but I also think it's a bug in the package spec parser. Thoughts?

Can the check here be moved to an earlier stage, like in ops::registry::publish::publish by iterating over the just_pkgs list and checking if any of them have the open-namespaces feature?

I realistically won't have time to give this a good review for at least a few days if not a week or more. I was wondering if @weihanglo or @epage would have more bandwidth in the short term? If not, that's ok, I'll try to review this when I can.

epage mentioned this pull request

Aug 28, 2024

This breaks the publish_namespaced test, because we now try to parse all packages as package specs, and "foo::bar" fails to parse. This could probably be avoided by a refactor (adding a different package function that takes a list of packages instead of trying to get them from cli arguments), but I also think it's a bug in the package spec parser. Thoughts?

@jneem I posted #14467

bors added a commit that referenced this pull request

Aug 29, 2024

fix(pkgid): Allow open namespaces in PackageIdSpec's

What does this PR try to resolve?

This is a part of #13576

This unblocks #14433. We have a test to ensure you can't publish a namespaced package and the error for that is getting masked in #14433 because the package name is getting parsed as a PackageIdSpec which wasn't supported until this PR.

How should we test and review this PR?

Additional information

Could you rebase on top of #14467?

Is it worth adding a new -Zpublish-workspace flag, or is it ok to reuse the old one?

I don't have enough context to know if -Zpackage-workspace was intended as a stepping-stone for publishing, or if there really is an independent use-case for packaging versus publishing.

-Zpackage-workspace is a stepping stone. In that spirit, it seems like the feature would be named -Zpublish-workspace but probably not worth changing at this point.

The questions relevant for this is would we want to stabilize these together? Are there design risks for one that by separating the stabilization of them would make it easier to stabilize the other?

But I don't think a straight dependency tree was the concern. Is my update accurate?

Yes, your update is accurate. I thought that a long straight dependency tree might also be a concern, because it effectively extends the timeout.

jneem mentioned this pull request

Sep 5, 2024

bors added a commit that referenced this pull request

Sep 5, 2024

Document -Zpackage-workspace

Adds some unstable documentation on the -Zpackage-workspace feature, as requested in [#10948](#10948 (comment)). This documentation assumes that #14433 gets merged.

Yes, your update is accurate. I thought that a long straight dependency tree might also be a concern, because it effectively extends the timeout.

Not concerned about the straight line because the timeout is for publishing a package, not publishing everything, so its natural they get concatenated when doing them back-to-back.