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

add `--dry-run` option to cargo update #6371

Merged
merged 2 commits into from Dec 12, 2018

Conversation

Projects
None yet
9 participants
@matklad
Member

matklad commented Dec 3, 2018

Hi!

This is just an idea, but what if we had a --dry-run argument for cargo update, which shows which crates would be updated, without actually writing the lockfile? If this seems useful, I'll write the tests and such. If we can leave without it, feel free to close.

Context: at ferrous-systems, we put together a small tool to review the source code changes between different versions of the same crate. This obviously was inspired by the most recent npm incident. While implementing the tool, I thought that having --dry-run might also be useful for folks who a cautious about updating. IE, you could run cargo update --dry-run once to see what could be updated, and follow up with a series of update -p --precise, to do the update in minimal steps, reviewing the actual source each time.

@rust-highfive

This comment has been minimized.

rust-highfive commented Dec 3, 2018

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 3, 2018

This sounds like a fantastic idea to me, others on @rust-lang/cargo have thoughts?

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Dec 3, 2018

Seems like a great idea.

@matklad

This comment has been minimized.

Member

matklad commented Dec 6, 2018

Added the test.

Should we feature gate this? I'd say no: this is more-or-less UI-only features, so there's no new functionality to stabilize, and feature gate would seem like a procedural overhead. But of course adding a feature gate is not a big deal, will gladly do that if necessary.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Dec 6, 2018

Since we already have dry-run on other commands, I don't think we need to feature gate this. we should probably get a full team check off though

@rfcbot fcp merge

@withoutboats withoutboats added the T-cargo label Dec 6, 2018

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Dec 6, 2018

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Dec 6, 2018

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@matklad

This comment has been minimized.

Member

matklad commented Dec 6, 2018

Since we already have dry-run on other commands,

Pedantic correction: there's a single command with --dry-run, cargo publish :-)

@bors

This comment has been minimized.

Contributor

bors commented Dec 8, 2018

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

@matklad matklad force-pushed the matklad:update-dry-run branch from dd82c8b to 7be09e3 Dec 8, 2018

@matklad

This comment has been minimized.

Member

matklad commented Dec 12, 2018

Fun fact: cargo update --locked is basically cargo update --dry-run.

@nrc

This comment has been minimized.

Member

nrc commented Dec 12, 2018

Discussed in our meeting today. One question was about the exit code, should we return success if the Cargo.lock would have been updated? (Although cargo update --locked gives that). But overall the feeling was positive.

@nrc nrc merged commit 4786249 into rust-lang:master Dec 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Eh2406

This comment has been minimized.

Contributor

Eh2406 commented Dec 12, 2018

@nrc did you mean to merge without bors?

@nrc

This comment has been minimized.

Member

nrc commented Dec 12, 2018

@nrc did you mean to merge without bors?

Whoops, I did not.

@matklad matklad deleted the matklad:update-dry-run branch Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment