Add --dry-run to cargo publish #2849

Merged
merged 1 commit into from Jul 18, 2016

Conversation

Projects
None yet
6 participants
@wezm
Contributor

wezm commented Jul 11, 2016

This PR picks up where @JustAPerson left off in #1699. I've updated their changes to apply to current master and added tests, including (I think) a test that checks that the upload doesn't actually occur.

The output is as it was before:

output

Closes #1332

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jul 11, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/cargo/ops/registry.rs
+ if !opts.verify && opts.dry_run {
+ bail!("cannot specify both no verify and dry run \
+ simultaneously");
+ }

This comment has been minimized.

@alexcrichton

alexcrichton Jul 11, 2016

Member

Could you clarify on what we're preventing with this check? I could imagine --dry-run also being done to check the *.crate manually without actually performing a build being a use case

@alexcrichton

alexcrichton Jul 11, 2016

Member

Could you clarify on what we're preventing with this check? I could imagine --dry-run also being done to check the *.crate manually without actually performing a build being a use case

This comment has been minimized.

@wezm

wezm Jul 11, 2016

Contributor

It was in the original PR. My guess is that it's not a full dry run if the verification step isn't run. Combining --no-verify and --dry-run works fine with the guard removed so I can remove it if you like.

@wezm

wezm Jul 11, 2016

Contributor

It was in the original PR. My guess is that it's not a full dry run if the verification step isn't run. Combining --no-verify and --dry-run works fine with the guard removed so I can remove it if you like.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 12, 2016

Member

Yeah thinking back I think this should be ok to remove

@alexcrichton

alexcrichton Jul 12, 2016

Member

Yeah thinking back I think this should be ok to remove

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 11, 2016

Member

Thanks @wezm! Looks great to me!

cc @rust-lang/tools, other thoughts?

Member

alexcrichton commented Jul 11, 2016

Thanks @wezm! Looks great to me!

cc @rust-lang/tools, other thoughts?

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 11, 2016

Contributor

Just from looking at the screenshot, "warning: Aborting" should not be capitalized.

Contributor

brson commented Jul 11, 2016

Just from looking at the screenshot, "warning: Aborting" should not be capitalized.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Jul 11, 2016

Member

Just from looking at the screenshot, "warning: Aborting" should not be capitalized.

Should it be a warning at all? Seems like expected behaviour

Member

nrc commented Jul 11, 2016

Just from looking at the screenshot, "warning: Aborting" should not be capitalized.

Should it be a warning at all? Seems like expected behaviour

src/bin/publish.rs
@@ -28,6 +29,7 @@ Options:
--no-verify Don't verify package tarball before publish
--allow-dirty Allow publishing with a dirty source directory
--manifest-path PATH Path to the manifest of the package to publish
+ --dry-run Performs all checks without uploading

This comment has been minimized.

@nrc

nrc Jul 11, 2016

Member

nit: Perform, not Performs

@nrc

nrc Jul 11, 2016

Member

nit: Perform, not Performs

@wezm

This comment has been minimized.

Show comment
Hide comment
@wezm

wezm Jul 11, 2016

Contributor

Just from looking at the screenshot, "warning: Aborting" should not be capitalized.

Should it be a warning at all? Seems like expected behaviour

I wondered about that. Perhaps the Uploading step could looks follows when it's a dry run:

Uploading tmp v0.1.0 (file:///Users/wmoore/Source/cargo/tmp) -- dry run

Although that would be fairly easy to miss.

Contributor

wezm commented Jul 11, 2016

Just from looking at the screenshot, "warning: Aborting" should not be capitalized.

Should it be a warning at all? Seems like expected behaviour

I wondered about that. Perhaps the Uploading step could looks follows when it's a dry run:

Uploading tmp v0.1.0 (file:///Users/wmoore/Source/cargo/tmp) -- dry run

Although that would be fairly easy to miss.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 12, 2016

Member

I vaguely remember that s3cmd'd --dry-run option prints out warning, so it doesn't seem all that too out of the ordinary for me

Member

alexcrichton commented Jul 12, 2016

I vaguely remember that s3cmd'd --dry-run option prints out warning, so it doesn't seem all that too out of the ordinary for me

@wezm

This comment has been minimized.

Show comment
Hide comment
@wezm

wezm Jul 12, 2016

Contributor

Ok folks what do you need from me to get this over the line?

Contributor

wezm commented Jul 12, 2016

Ok folks what do you need from me to get this over the line?

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 13, 2016

Contributor

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

Contributor

bors commented Jul 13, 2016

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 14, 2016

Member

Let's take out the extra check and leave the UI as-is for now, we can always tweak later and it's unfortunately not easy right now to have something vastly different than this.

Member

alexcrichton commented Jul 14, 2016

Let's take out the extra check and leave the UI as-is for now, we can always tweak later and it's unfortunately not easy right now to have something vastly different than this.

@wezm

This comment has been minimized.

Show comment
Hide comment
@wezm

wezm Jul 15, 2016

Contributor

Ok

  • updated to apply cleanly to current master
  • remove mutual exclusion of --no-verify and --dry-run

Should be good to go.

Contributor

wezm commented Jul 15, 2016

Ok

  • updated to apply cleanly to current master
  • remove mutual exclusion of --no-verify and --dry-run

Should be good to go.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 15, 2016

Member

Thanks! Could you also squash the commits as well? Other than that looks good to go to me.

Member

alexcrichton commented Jul 15, 2016

Thanks! Could you also squash the commits as well? Other than that looks good to go to me.

Add --dry-run option to publish sub-command
Squashed commit of the following:

commit deed1d7
Author: Wesley Moore <wes@wezm.net>
Date:   Fri Jul 15 13:35:01 2016 +1000

    Remove --dry-run and --no-verify mutual exclusion

commit 8a91fcf
Merge: 0c0d057 970535d
Author: Wesley Moore <wes@wezm.net>
Date:   Fri Jul 15 13:30:38 2016 +1000

    Merge remote-tracking branch 'upstream/master' into publish_dry_run

commit 0c0d057
Author: Wesley Moore <wes@wezm.net>
Date:   Tue Jul 12 08:03:15 2016 +1000

    Improve grammar in --dry-run option

commit a17c1bf
Author: Wesley Moore <wes@wezm.net>
Date:   Mon Jul 11 14:17:41 2016 +1000

    Add test for passing no-verify and dry-run to publish

commit 284810c
Author: Wesley Moore <wes@wezm.net>
Date:   Mon Jul 11 14:51:38 2016 +1000

    Add test for publish --dry-run

commit 8514e47
Merge: 2b061c5 ef07b81
Author: Wesley Moore <wes@wezm.net>
Date:   Mon Jul 11 08:27:10 2016 +1000

    Merge branch 'publish_dry_run' of github.com:JustAPerson/cargo into publish_dry_run

commit ef07b81
Author: Jason Priest <jpriest128@gmail.com>
Date:   Tue Jun 9 23:11:51 2015 -0500

    Improve publish `--dry-run`

    Catch a few more errors by aborting midway through transmit().

commit 0686fb0
Author: Jason Priest <jpriest128@gmail.com>
Date:   Tue Jun 9 14:38:58 2015 -0500

    Teach publish the `--dry-run` flag

    Closes #1332
@wezm

This comment has been minimized.

Show comment
Hide comment
@wezm

wezm Jul 17, 2016

Contributor

All squashed now

Contributor

wezm commented Jul 17, 2016

All squashed now

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 18, 2016

Member

@bors: r+ c05a5b4

Thanks!

Member

alexcrichton commented Jul 18, 2016

@bors: r+ c05a5b4

Thanks!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 18, 2016

Contributor

⌛️ Testing commit c05a5b4 with merge f2cf284...

Contributor

bors commented Jul 18, 2016

⌛️ Testing commit c05a5b4 with merge f2cf284...

bors added a commit that referenced this pull request Jul 18, 2016

Auto merge of #2849 - wezm:publish_dry_run, r=alexcrichton
Add --dry-run to cargo publish

This PR picks up where @JustAPerson left off in #1699. I've updated their changes to apply to current master and added tests, including (I think) a test that checks that the upload doesn't actually occur.

The output is as it was before:

![output](https://cloud.githubusercontent.com/assets/21787/16720362/f7ea710e-4778-11e6-8163-65f3978bb7ae.png)

Closes #1332
@bors

This comment has been minimized.

Show comment
Hide comment
Contributor

bors commented Jul 18, 2016

@bors bors merged commit c05a5b4 into rust-lang:master Jul 18, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment