feat(client,cli): add multi-packaging support for wkg publish#215
feat(client,cli): add multi-packaging support for wkg publish#215mkatychev wants to merge 64 commits into
wkg publish#215Conversation
Co-authored-by: Bailey Hayes <ricochet@users.noreply.github.com>
Co-authored-by: Bailey Hayes <ricochet@users.noreply.github.com>
vados-cosmonic
left a comment
There was a problem hiding this comment.
Hey I took a pass through, a lot of NITs but it looks mostly good to me!
One thing I'd love to have is a few more tests... This is a pretty substantial feature, so figured there would be more edges/fixtures. That said, we can certainly add tests down the line if we run into anything.
| // 2. Publish our packages in "waves" to the actual registries ensuring all | ||
| // possible dependency free pacakges are published in the same group | ||
| while !plan.is_empty() { | ||
| let ready_for_publish = plan.take_ready(); |
There was a problem hiding this comment.
Are we sure this will always terminate? Should we add a check just in case? We're not allowed to have cycles AFAIK so maybe I'm being a bit too defensive here.
There was a problem hiding this comment.
You're totally right, cargo's take_ready bakes in a timeout.
EDIT: forgot to post link:
https://github.com/rust-lang/cargo/blob/a595d0da21f228b7fdae64d3d5c0e527ea66bb59/src/cargo/ops/registry/publish.rs#L289-L294
There was a problem hiding this comment.
Oh Ok great! but it's not like... async? I think I actually have more questions now -- I was wondering if it's possible for ready_for_publish to not have anything in it and the plan to not be empty, essentially.
Not looking for a proof or anything of course, but basically... could the graph be misconfigured in a way that nodes are kinda hanging out but don't end up looking ready? The answer could easily be "absolutely not" and then this isn't an issue, but a small invariant check (i.e. "if plan is empty, take_ready will always produce at least one thing to do on each iteration") would be nice
There was a problem hiding this comment.
Here's what I've reasoned myself into:
ready_for_publishis guaranteed to be nonempty IF!plan.is_empty()and- a
DependencyGraph(petgraph::Acyclic) will produceAcyclicEdgeErrorerrors duringtry_update_edgefor any "danging graphs"1
- a
- a node is determined to be ready when it has zero
Incomingedges2:
wasm-pkg-tools/crates/wasm-pkg-core/src/resolver.rs
Lines 899 to 902 in c26b6bf
The one area we don't have guarantees is if PackagePublisher::publish will ever terminate.
Footnotes
There was a problem hiding this comment.
IMO @mkatychev this would make for a great comment close in the code!
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Victor Adossi <123968127+vados-cosmonic@users.noreply.github.com>
Co-authored-by: Victor Adossi <123968127+vados-cosmonic@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
@vados-cosmonic for wasm-pkg-tools/crates/wkg/src/main.rs Line 279 in 778b1cc |
Added the ability to publish multiple WIT packages at once.
Introduced the
PublishPlanstruct to resolve local references into a sequence of batches for publication:wasm-pkg-tools/crates/wasm-pkg-core/src/resolver.rs
Lines 846 to 853 in 40d2dd3
Reference implementation (cargo multi-package publishing):