-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: Implement print plan and basic checks for publish command #154
feat: Implement print plan and basic checks for publish command #154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Made a few blocking comments.
cargo-workspaces/src/publish.rs
Outdated
/// | ||
/// This is only a heuristic to prevent simple mistakes. | ||
#[clap(long)] | ||
basic_checks: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does cargo publish
not run these checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It certainly does, but when publishing a workspace, if, say, 7th package has some field missing, you will only know when you're in the middle of process (when you already bumped a version, created a commit, etc). So this is not very useful for publishing individual packages, but IMHO very useful for publishing a workspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of running cargo's --dry-run
option then instead of writing the checks on our own? That way, we can run our own --dry-run
for this publish
subcommand. Here's the related issue #4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, --dry-run
doesn't perform these checks, as it aborts before uploading :(
These checks are performed on the server only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Let's rename the option to dry_run
, keep the checks and also incorporate cargo's --dry-run
. Wouldn't completely fix #4, but is a start towards it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I don't think it would work -- cargo publish --dry-run
does check that dependencies are published, so cargo publish --dry-run
would fail for the 2nd crate in the workspace.
And without this functionality, changing the name would probably be misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And without this functionality, changing the name would probably be misleading.
If we are to incorporate everything else from the cargo's dry run other than checking published dependencies, it wouldn't be misleading, right? I know that cargo-release has a dry run option.
I still think we name this option as such and work toward completing it. But, in the meanwhile, we can document it as in progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pksunkara that makes sense! I've reworked the implementation:
- Renamed
basic-checks
todry-run
. - Changed
dry-run
behavior to ignore versioning and publishing. - Added
cargo build
as a step together withbasic_checks
so that we check if package compiles at least. This is not strictly equivalent to--dry-run
behavior, because AFAIU it will reuse the workspaceCargo.lock
, while with--dry-run
a freshCargo.lock
is generated for the package. Also, it obviously doesn't try to use previously published packages, but I guess it's unavoidable. - With
--dry-run
mode for publish,--basic-checks
do not make much sense inplan
subcommand, so I removed it there. - I also made checking if package is published optional for
plan
subcommand, because the difference in speed is significant and you don't always want to check that.
tl;dr:
cargo ws publish --dry-run
seems to have a good enough implementation.cargo ws plan
doesn't try to be too smart and only provides information on publishing plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
The only other thing is I would need some tests for plan (no publishing check) and dry run (without build check).
If you can test the other parts too, great. If not, no worries since they are hard to test.
@pksunkara I've implemented the requested changes.
Do you have any advice on how to proceed here? |
3b30700
to
ce55c05
Compare
if self.dry_run { | ||
args.push("--dry-run"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it will work? When I checked before, cargo publish --dry-run
would fail on the 2nd package, because we didn't publish the previous one. This is why I didn't supply this argument before (as mentioned here).
Probably this should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked, the command currently doesn't work. See an example output when trying --dry-run
with fixtures/normal
:
error: failed to prepare local package for uploading
Caused by:
no matching package named `dep1` found
location searched: registry `crates-io`
required by package `top v0.1.0 (/home/popzxc/workspace/cargo-workspaces/fixtures/normal/top)`
I will prepare a fix and add tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --no-verify
was added when dry-run
is enabled and that would make sure that dependencies are not checked for being published. This is how the other tools in this space are doing it too.
It was working for me in Cargo 1.75. I had a monorepo with half-published crates and all of it worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, you're absolutely right! I forgot to reinstall the binary when checking 🤦
Will revert corresponding changes in #157
This PR adds two features I found useful recently:
cargo publish
.The first one proved handy when I had to publish a lot of crates written by different people that weren't initially meant to be published. So every so often a field would be forgotten, but I won't know about it until I'm in the middle of the publishing process.
The second one is useful for large workspaces: crates.io has pretty strict rate limits on publishing new packages, so it's actually easier to get the order in which crates should be published and then do it manually.