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.

Sign up for GitHub

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

Print information about updated packages on cargo update. #1621

Closed
wants to merge 3 commits into from
Closed

Print information about updated packages on cargo update. #1621

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb4e354
Print information about updated packages on cargo update.
pyfisch May 17, 2015
a62b3a9
Add tests, use `Shell` and rewrite compare_dependency_graphs()
pyfisch May 19, 2015
8c38f68
Nicer code.
pyfisch May 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion src/cargo/ops/cargo_generate_lockfile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use std::path::Path;

use core::PackageId;
Expand Down Expand Up @@ -89,6 +89,21 @@ pub fn update_lockfile(manifest_path: &Path,
Method::Everything,
Some(&previous_resolve),
Some(&to_avoid)));
for dep in compare_dependency_graphs(&previous_resolve, &resolve) {
try!(match dep {
(None, Some(pkg)) => opts.config.shell().status("Adding",
format!("{} v{}", pkg.name(), pkg.version())),
(Some(pkg), None) => opts.config.shell().status("Removing",
format!("{} v{}", pkg.name(), pkg.version())),
(Some(pkg1), Some(pkg2)) => {
if pkg1.version() != pkg2.version() {
opts.config.shell().status("Updating",
format!("{} v{} -> v{}", pkg1.name(), pkg1.version(), pkg2.version()))
} else {Ok(())}
}
(None, None) => unreachable!(),
});
Copy link
Member

Choose a reason for hiding this comment

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

This may be better structured like:

let (status, msg) = match dep {
    (None, Some(pkg)) => ("Adding", format!(...)),
    (Some(pkg), None) => ("Removing", format!(...)),
    (Some(pkg1), Some(pkg2)) => {
        if pkg1.version() == pkg2.version() { continue }
        ("Updating", format!(...))
    }
    (None, None) => continue,
};
try!(opts.config.shell().status(...))

}
try!(ops::write_pkg_lockfile(&package, &resolve));
return Ok(());

Expand All @@ -106,4 +121,23 @@ pub fn update_lockfile(manifest_path: &Path,
None => {}
}
}

fn compare_dependency_graphs<'a>(previous_resolve: &'a Resolve,
resolve: &'a Resolve) ->
Vec<(Option<&'a PackageId>, Option<&'a PackageId>)> {
let mut changes = HashMap::new();
for dep in previous_resolve.iter() {
changes.insert(dep.name(), (Some(dep), None));
Copy link
Member

Choose a reason for hiding this comment

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

I think that this may not be quite right as it's possible to have a few different situations:

  • There could be multiple packages in the graph with the same name, but from different sources.
  • There could be multiple versions of the same package in the graph, locked at different versions.

I think that this could be solved by having the key of the map being (&str, &SourceId) (e.g. .name() and .source()), and then the values would be (Vec<&PackageId>, Vec<&PackageId>) where the first element is all activated versions beforehand and the latter is all activated versions afterwards.

When the map is returned the left vector could represent "versions removed" and the right vector could represent "versions added". In the common case of updating a dep we'll remove one version and add one, but it could possibly be more complicated.

Copy link
Author

Choose a reason for hiding this comment

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

Is it really possible to have multiple versions of the same package in the graph? Some time ago hyper depended indirectly on two different versions of openssl and cargo complained and did not build.

Copy link
Member

Choose a reason for hiding this comment

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

That's a special case because openssl was a system library (so only one is allowed), but in general multiple Rust libraries are indeed allowed.

}
for dep in resolve.iter() {
if !changes.contains_key(dep.name()) {
changes.insert(dep.name(), (None, None));
}
let value = changes.get_mut(dep.name()).unwrap();
value.1 = Some(dep);
}
let mut package_names: Vec<&str> = changes.keys().map(|x| *x).collect();
package_names.sort();
package_names.iter().map(|name| *changes.get(name).unwrap()).collect()
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can just use changes[name] here instead of changes.get(name).unwrap()

}
}
2 changes: 2 additions & 0 deletions tests/support/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,8 @@ pub static RUNNING: &'static str = " Running";
pub static COMPILING: &'static str = " Compiling";
pub static FRESH: &'static str = " Fresh";
pub static UPDATING: &'static str = " Updating";
pub static ADDING: &'static str = " Adding";
pub static REMOVING: &'static str = " Removing";
pub static DOCTEST: &'static str = " Doc-tests";
pub static PACKAGING: &'static str = " Packaging";
pub static DOWNLOADING: &'static str = " Downloading";
Expand Down
26 changes: 25 additions & 1 deletion tests/test_cargo_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::io::prelude::*;
use cargo::util::process;

use support::{project, execs, cargo_dir};
use support::{UPDATING, DOWNLOADING, COMPILING, PACKAGING, VERIFYING};
use support::{UPDATING, DOWNLOADING, COMPILING, PACKAGING, VERIFYING, ADDING, REMOVING};
use support::paths::{self, CargoPathExt};
use support::registry as r;
use support::git;
Expand Down Expand Up @@ -476,6 +476,7 @@ test!(update_lockfile {
.arg("-p").arg("bar").arg("--precise").arg("0.0.2"),
execs().with_status(0).with_stdout(&format!("\
{updating} registry `[..]`
{updating} bar v0.0.1 -> v0.0.2
", updating = UPDATING)));

println!("0.0.2 build");
Expand All @@ -492,6 +493,7 @@ test!(update_lockfile {
.arg("-p").arg("bar"),
execs().with_status(0).with_stdout(&format!("\
{updating} registry `[..]`
{updating} bar v0.0.2 -> v0.0.3
", updating = UPDATING)));

println!("0.0.3 build");
Expand All @@ -502,6 +504,27 @@ test!(update_lockfile {
{compiling} foo v0.0.1 ({dir})
", downloading = DOWNLOADING, compiling = COMPILING,
dir = p.url())));

println!("new dependencies update");
r::mock_pkg("bar", "0.0.4", &[("spam", "0.2.5", "")]);
r::mock_pkg("spam", "0.2.5", &[]);
assert_that(p.cargo("update")
.arg("-p").arg("bar"),
execs().with_status(0).with_stdout(&format!("\
{updating} registry `[..]`
{updating} bar v0.0.3 -> v0.0.4
{adding} spam v0.2.5
", updating = UPDATING, adding = ADDING)));

println!("new dependencies update");
r::mock_pkg("bar", "0.0.5", &[]);
assert_that(p.cargo("update")
.arg("-p").arg("bar"),
execs().with_status(0).with_stdout(&format!("\
{updating} registry `[..]`
{updating} bar v0.0.4 -> v0.0.5
{removing} spam v0.2.5
", updating = UPDATING, removing = REMOVING)));
});

test!(dev_dependency_not_used {
Expand Down Expand Up @@ -760,6 +783,7 @@ test!(update_transitive_dependency {
execs().with_status(0)
.with_stdout(format!("\
{updating} registry `[..]`
{updating} b v0.1.0 -> v0.1.1
", updating = UPDATING)));

assert_that(p.cargo("build"),
Expand Down