Print information about `update`d packages #1931

thirtythreeforty · Aug 24, 2015

Compare #1621. Like that pull request, this one solves #984.

I have rewritten most of @pyfisch's logic to account for @alexcrichton's comments. Specifically, this code will correctly handle adding/removing multiple versions of one package, as well as several packages with different sources, but the same name.

The output looks like this:

[georgev@desertvoice cargo]$ cargo update
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating libc v0.1.8 -> v0.1.10
    Updating memchr v0.1.3 -> v0.1.5
    Updating num v0.1.26 -> v0.1.27
    Updating rand v0.3.9 -> v0.3.10
    Updating rustc-serialize v0.3.15 -> v0.3.16

Comments welcome.

r? @alexcrichton

thirtythreeforty · Aug 24, 2015

Rats, CI fails. I'll have to patch all the tests for the new blurbs.

alexcrichton · Aug 25, 2015

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton · Aug 25, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+        if removed.len() == 1 && added.len() == 1 {
+            try!(print_change("Updating", format!("{} v{} -> v{}",
+                                                  removed[0].name(),
+                                                  removed[0].version(),


Another part of package ids is the source it comes from (e.g. foo from crates.io is different than foo from github). In addition to indicating the source, updates of git repositories probably want to print out the SHA that was updated to instead of the revision as that's the "precise" notion there. You should be able to access this through the precise field of the SourceId

Alright, that shouldn't be hard. How would you like the SourceId displayed in the Git and the non-Git cases?

Falling back to the normal Display implementation should be good enough

alexcrichton · Aug 25, 2015

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton · Aug 25, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+            for package in removed.iter() {
+                try!(print_change("Removing", format!("{} v{}",
+                                                      package.name(),
+                                                      package.version())));


For these two it should be safe to just pass package as the thing-to-print as it'll cover the name/version automatically as well as including the source id.

alexcrichton · Aug 25, 2015

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton · Aug 25, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+            (dep.name(), dep.source_id())
+        }
+
+        fn vec_subtract<'a, T>(a: &'a Vec<T>, b: &'a Vec<T>) -> Vec<T>


&'a Vec<T> is typically taken as &[T], and in this case I don't think you need to connect the lifetimes together so it's probably ok to remove that annotation.

alexcrichton · Aug 25, 2015

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton · Aug 25, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+        }
+
+        // Sort the packages by their names.
+        let mut packages: Vec<(&'a str, &'a SourceId)> =


You can probably omit the (&'a str, ...) here with a placeholder _ type variable

alexcrichton · Aug 25, 2015

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton · Aug 25, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+        // Sort the packages by their names.
+        let mut packages: Vec<(&'a str, &'a SourceId)> =
+            changes.keys().map(|x| *x).collect();
+        packages.sort_by(|a, b| a.0.cmp(b.0));


Could this just call sort? That should naturally handle the orderings of names while also sorting by the sources next.

alexcrichton · Aug 25, 2015

Thanks for picking this up @thirtythreeforty, looking good!

We do also modify the lockfile in the case that the manifest was modified and then cargo build was re-run, and that's another possible case where we could print what's being updated/removed, but it may also be safe to avoid doing so in that situation because it was manually required anyway.

Also, to ensure that this scales well, could you try running this over Servo? If you check out servo and run cargo update in components/servo then something should happen (I'm not quite sure what), but it'd also be good to make sure that it doesn't keep your cpus too too warm.

thirtythreeforty · Aug 25, 2015

Here's the output from Servo:

[georgev@desertvoice servo]$ ~/Code/cargo/target/debug/cargo update
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/servo/rust-png`
    Updating git repository `https://github.com/pcwalton/ipc-channel`
    Updating git repository `https://github.com/TyOverby/bincode`
    Updating git repository `https://github.com/servo/rust-tenacious`
    Updating git repository `https://github.com/servo/rust-azure`
    Updating git repository `https://github.com/ecoal95/rust-offscreen-rendering-context`
    Updating git repository `https://github.com/servo/rust-layers`
    Updating git repository `https://github.com/servo/io-surface-rs`
    Updating git repository `https://github.com/servo/skia`
    Updating git repository `https://github.com/servo/core-text-rs`
    Updating git repository `https://github.com/servo/rust-egl`
    Updating git repository `https://github.com/servo/rust-freetype`
    Updating git repository `https://github.com/servo/libfreetype2`
    Updating git repository `https://github.com/servo/libexpat`
    Updating git repository `https://github.com/servo/rust-glx`
    Updating git repository `https://github.com/servo/rust-mozjs`
    Updating git repository `https://github.com/servo/rust-selectors`
    Updating git repository `https://github.com/servo/mozjs`
    Updating git repository `https://github.com/servo/rust-stb-image`
    Updating git repository `https://github.com/servo/glutin`
    Updating git repository `https://github.com/aweinstock314/rust-clipboard`
    Updating git repository `https://github.com/servo/rust-fontconfig`
    Updating git repository `https://github.com/servo/libfontconfig`
    Updating git repository `https://github.com/libheartbeats/heartbeats-simple-rust.git`
    Updating git repository `https://github.com/libheartbeats/heartbeats-simple-sys.git`
    Updating git repository `https://github.com/tomaka/clock_ticks`
    Updating git repository `https://github.com/jgraham/webdriver-rust.git`
    Updating aster v0.4.1 -> v0.4.3
    Updating azure v0.1.0 -> v0.1.0
    Updating bincode v0.4.0 -> v0.4.0
      Adding bitflags v0.1.1
      Adding block v0.1.0
    Updating clipboard v0.0.2 -> v0.0.3
      Adding clipboard-win v1.5.1
    Removing clipboard-win v1.4.0
    Updating gcc v0.3.12 -> v0.3.13
    Updating getopts v0.2.11 -> v0.2.13
      Adding gl_generator v0.0.26
      Adding heapsize_plugin v0.0.1
    Updating html5ever v0.2.1 -> v0.2.3
    Updating html5ever_macros v0.2.0 -> v0.2.1
    Updating httparse v0.1.5 -> v0.1.6
    Updating hyper v0.6.8 -> v0.6.10
    Updating kernel32-sys v0.1.3 -> v0.1.4
      Adding khronos_api v0.0.8
    Updating layers v0.1.0 -> v0.1.0
    Updating libc v0.1.8 -> v0.1.10
    Updating memchr v0.1.3 -> v0.1.5
    Updating num v0.1.25 -> v0.1.27
    Updating objc v0.1.5 -> v0.1.6
      Adding objc-foundation v0.0.1
      Adding objc_id v0.0.1
    Updating offscreen_gl_context v0.1.0 -> v0.1.0
    Updating rand v0.3.9 -> v0.3.10
    Updating rustc-serialize v0.3.15 -> v0.3.16
    Updating serde v0.5.1 -> v0.5.2
    Updating serde_codegen v0.5.0 -> v0.5.1
    Updating solicit v0.4.1 -> v0.4.2
    Updating tendril v0.1.4 -> v0.1.5
      Adding unicase v1.0.0
    Updating url v0.2.36 -> v0.2.37
    Updating user32-sys v0.1.1 -> v0.1.2
    Updating webdriver v0.2.2 -> v0.2.3
    Updating winapi v0.2.1 -> v0.2.2

thirtythreeforty · Aug 25, 2015

The Removing clipboard-win and Adding clipboard-win lines are confusing without sources. With sources added (by simply using the Display impl for SourceId), they become:

    Removing clipboard-win v1.4.0 (https://github.com/DoumanAsh/clipboard-win#e9f01b5b)
      Adding clipboard-win v1.5.1

The lack of source on the second line is still a bit confusing, but I suppose it's easy enough to figure out that it's from the crate registry.

alexcrichton · Aug 25, 2015

Yeah we've got a pretty strong convention of not printing the registry URL for registry packages, so I think it'll quickly become common knowledge that "no source" means "crates.io".

Also, thanks for generating that! Did it take an overly long time to generate or was it nice and speedy? (make sure you're using a --release build to test)

thirtythreeforty · Aug 25, 2015

Initially it took a while to download and it did a ton of work (looked like it was compiling something?) when it was updating git repos, but subsequent runs are pretty quick and don't heat up the processor at all. Generating the list of changes takes no time at all, as far as I can tell.

alexcrichton · Aug 25, 2015

Perfect, sounds good to me!

thirtythreeforty · Aug 26, 2015

OK, that commit should address your feedback. (I knew &Vec<T> was wrong, I just wanted to see if you'd catch it 😇.)

The tests should all still fail because the test cases need to be rewritten to account for the extra info. I'll do that and squash everything once you and I are happy with the main implementation.

Here's the new result for Servo:

[georgev@desertvoice servo]$ ~/Code/cargo/target/debug/cargo update
    Updating git repository `https://github.com/servo/rust-png`
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `https://github.com/pcwalton/ipc-channel`
    Updating git repository `https://github.com/servo/rust-mozjs`
    Updating git repository `https://github.com/servo/rust-selectors`
    Updating git repository `https://github.com/servo/rust-layers`
    Updating git repository `https://github.com/servo/rust-azure`
    Updating git repository `https://github.com/TyOverby/bincode`
    Updating git repository `https://github.com/servo/mozjs`
    Updating git repository `https://github.com/servo/skia`
    Updating git repository `https://github.com/servo/rust-glx`
    Updating git repository `https://github.com/servo/rust-egl`
    Updating git repository `https://github.com/servo/io-surface-rs`
    Updating git repository `https://github.com/servo/libexpat`
    Updating git repository `https://github.com/servo/libfreetype2`
    Updating git repository `https://github.com/servo/rust-freetype`
    Updating git repository `https://github.com/servo/core-text-rs`
    Updating git repository `https://github.com/servo/rust-tenacious`
    Updating git repository `https://github.com/ecoal95/rust-offscreen-rendering-context`
    Updating git repository `https://github.com/servo/rust-stb-image`
    Updating git repository `https://github.com/tomaka/clock_ticks`
    Updating git repository `https://github.com/servo/rust-fontconfig`
    Updating git repository `https://github.com/servo/libfontconfig`
    Updating git repository `https://github.com/libheartbeats/heartbeats-simple-rust.git`
    Updating git repository `https://github.com/libheartbeats/heartbeats-simple-sys.git`
    Updating git repository `https://github.com/servo/glutin`
    Updating git repository `https://github.com/aweinstock314/rust-clipboard`
    Updating git repository `https://github.com/jgraham/webdriver-rust.git`
    Updating aster v0.4.1 -> v0.4.3
    Updating azure #53e7b7d0 -> #e4831b3d
    Updating bincode #5bc43b85 -> #c1d94fe8
      Adding bitflags v0.1.1
      Adding block v0.1.0
    Updating clipboard #701fa10b -> #b0092c86
    Removing clipboard-win v1.4.0 (https://github.com/DoumanAsh/clipboard-win#e9f01b5b)
      Adding clipboard-win v1.5.1
    Updating gcc v0.3.12 -> v0.3.13
    Updating getopts v0.2.11 -> v0.2.13
      Adding gl_generator v0.0.26
      Adding heapsize_plugin v0.0.1
    Updating html5ever v0.2.1 -> v0.2.3
    Updating html5ever_macros v0.2.0 -> v0.2.1
    Updating httparse v0.1.5 -> v0.1.6
    Updating hyper v0.6.8 -> v0.6.10
    Updating kernel32-sys v0.1.3 -> v0.1.4
      Adding khronos_api v0.0.8
    Updating layers #c4fa5652 -> #2fcdaf62
    Updating libc v0.1.8 -> v0.1.10
    Updating memchr v0.1.3 -> v0.1.5
    Updating num v0.1.25 -> v0.1.27
    Updating objc v0.1.5 -> v0.1.6
      Adding objc-foundation v0.0.1
      Adding objc_id v0.0.1
    Updating offscreen_gl_context #9efc32eb -> #9704865b
    Updating rand v0.3.9 -> v0.3.10
    Updating rustc-serialize v0.3.15 -> v0.3.16
    Updating serde v0.5.1 -> v0.5.2
    Updating serde_codegen v0.5.0 -> v0.5.1
    Updating solicit v0.4.1 -> v0.4.2
    Updating tendril v0.1.4 -> v0.1.5
      Adding unicase v1.0.0
    Updating url v0.2.36 -> v0.2.37
    Updating user32-sys v0.1.1 -> v0.1.2
    Updating webdriver #22658948 -> #5008a3d0
    Updating winapi v0.2.1 -> v0.2.2

Note the # in front of the commit hash, which I found other Cargo code already doing. It's not idiomatic Git, though (which prepends a g to the shorthash). I figured it would be better to be internally consistent. Nitpicky I know, but somebody has to be.

alexcrichton · Aug 26, 2015

alexcrichton reviewed Aug 26, 2015

View changes

alexcrichton · Aug 26, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+        if removed.len() == 1 && added.len() == 1 {
+            if removed[0].source_id().is_git() {
+                try!(print_change("Updating", format!("{} #{} -> #{}",
+                    removed[0].name(),


This may want to just be removed[0] as that'll print out the git source this came from as well.

Doing that makes the update line look like this:

Updating offscreen_gl_context v0.1.0 (https://github.com/ecoal95/rust-offscreen-rendering-context#9efc32eb) -> #9704865b

Which I don't think is better, personally.

Hm yeah it can get long sometimes, but I think it's better than not printing it because there's real contextual information (e.g. foo from a git repo is different than foo from crates.io is different than foo from a path source)

Very well, I see your point.

alexcrichton · Aug 26, 2015

alexcrichton reviewed Aug 26, 2015

View changes

alexcrichton · Aug 26, 2015

src/cargo/ops/cargo_generate_lockfile.rs

+            } else {
+                try!(print_change("Updating", format!("{} v{} -> v{}",
+                    removed[0].name(),
+                    removed[0].version(),


These two lines can be replaced with removed[0] as it'll print the same thing for registry sources and otherwise print contextual information for things like path sources

Alright. I thought that it might be a little redundant to print that info for upgrades, but if the convention is to omit the source for registry packages, then it might be confusing to not specify that the package is from elsewhere.

alexcrichton · Aug 27, 2015

Ok, looks good to me, thanks @thirtythreeforty! There's some failing tests but with those fixed and a squash I think this is good to go

thirtythreeforty · Aug 28, 2015

Thanks for the review! Just squashed with all the tests fixed.

alexcrichton · Aug 28, 2015

@bors: r+ 492b582

Thanks!

bors · Aug 28, 2015

⌛️ Testing commit 492b582 with merge 1c27185...

bors · Aug 28, 2015

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64

thirtythreeforty · Aug 28, 2015

Glad to help!

rust-highfive assigned alexcrichton Aug 24, 2015

Print information about updated packages on cargo update
Closes #984.

Loading status checks…

492b582

bors merged commit 492b582 into rust-lang:master Aug 28, 2015
3 checks passed

3 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

thirtythreeforty deleted the thirtythreeforty:printinfo branch Aug 28, 2015

rust-lang/cargo

Join GitHub today

Print information about `update`d packages #1931

Conversation

thirtythreeforty commented Aug 24, 2015

rust-highfive assigned alexcrichton Aug 24, 2015

This comment has been minimized.

thirtythreeforty commented Aug 24, 2015

alexcrichton reviewed Aug 25, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 25, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 25, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 25, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Aug 25, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Aug 25, 2015

This comment has been minimized.

thirtythreeforty commented Aug 25, 2015

This comment has been minimized.

thirtythreeforty commented Aug 25, 2015

This comment has been minimized.

alexcrichton commented Aug 25, 2015

This comment has been minimized.

thirtythreeforty commented Aug 25, 2015

This comment has been minimized.

alexcrichton commented Aug 25, 2015

This comment has been minimized.

thirtythreeforty commented Aug 26, 2015

alexcrichton reviewed Aug 26, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Aug 26, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Aug 27, 2015

This comment has been minimized.

thirtythreeforty commented Aug 28, 2015

This comment has been minimized.

alexcrichton commented Aug 28, 2015

bors added a commit that referenced this pull request Aug 28, 2015

This comment has been minimized.

bors commented Aug 28, 2015

This comment has been minimized.

bors commented Aug 28, 2015

Hide details View details bors merged commit 492b582 into rust-lang:master Aug 28, 2015 3 checks passed

3 checks passed

thirtythreeforty deleted the thirtythreeforty:printinfo branch Aug 28, 2015

This comment has been minimized.

thirtythreeforty commented Aug 28, 2015

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton reviewed Aug 25, 2015

View changes

alexcrichton reviewed Aug 26, 2015

View changes

alexcrichton reviewed Aug 26, 2015

View changes

bors merged commit `492b582` into rust-lang:master Aug 28, 2015
3 checks passed