Skip to content
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

verbose update of dependencies #5634

Closed
wants to merge 2 commits into from

Conversation

debris
Copy link
Contributor

@debris debris commented Jun 15, 2018

In some cases (#5530) cargo does not print update messages. This pr tries to address this problem.

In a case described in issue #5530, cargo will print additional line:

    Updating registry `https://github.com/rust-lang/crates.io-index`
+   Updating bitflags v0.9.1 -> v1.0.3

If accepted, I will add some tests

@rust-highfive
Copy link

r? @matklad

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


{
let dependents_iter = changes.iter()
.filter(|(_, v)| v.0.len() == 0 && v.1.len() == 0)
Copy link
Contributor Author

@debris debris Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is one edge case where this check is not enough, e.g.

  1. Set up a workspace with the following members and dependencies:
a → bitflags="0.9"
b → bitflags="1.0"
c → bitflags="0.9"
  1. cargo update to build the lock file
  2. New version of bitflags is released on crates.io - 1.x
  3. Edit c's version of bitflags to 1.0
  4. cargo update will print only the message describing an update from 1.0 to 1.x

@debris
Copy link
Contributor Author

debris commented Jun 16, 2018

In the second commit, I completely refactored the way diffs are generated. Yet, the output is consistent with the output cargo produces currently.

diff of cargo update run with cargo 1.26.0 (0e7c5a931 2018-04-06) and this branch on parity/master commit 447950e7c160b8ee2c886d3659007fc7cf50567f

https://www.diffchecker.com/YIRXmxLq

@debris
Copy link
Contributor Author

debris commented Jun 16, 2018

But I am not sure if #5530 and this pr is valid in general.

Before:

  • Adding - means that there is a new dependency
  • Removing - means that the dependency has been removed
  • Adding + Removing - means that dependency has been updated, but cargo printed this weird message instead of the proper one
  • Updating - means that old version of the dependency is no longer used, but the new one is instead

Now

  • Adding - means that there is a new dependency. The meaning remains the same
  • Removing - means that the dependency has been removed. The meaning remains the same
  • Adding + Removing - may still happen in some edge cases. But the meaning remains the same.
  • Updating - means that one of the packages stopped using the old dependency. Does not mean that this dependency is not used any more.

If the new assumptions are valid, than the issue and this pr are valid as well

@alexcrichton
Copy link
Member

Thanks for the PR! I definitely think that #5530 is a good issue to fix in that cargo update with no output implies to me that nothing happened nowadays, so if something happens we should print at least something!

Mind adding a few tests for the fixed cases here as well? Other than that looks good to go to me!

@alexcrichton
Copy link
Member

ping @debris, have you had a chance to take another look at this?

@debris
Copy link
Contributor Author

debris commented Jul 14, 2018

sure, I'll try to finish this in the upcoming week

@alexcrichton
Copy link
Member

Ok I'm gonna close this for now, but @debris if you get a chance to update I can certainly reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants