When upgrading to lockfile, process manifests one at a time

Multiple manifests in a workspace may request the same dependency
at different versions. The version of each dependency should be
updated to the corresponding semver compatible version in the
lockfile.

Author: tofay

src/bin/upgrade/main.rs

@@ -95,6 +95,33 @@ struct Args {
 /// A collection of manifests.
 struct Manifests(Vec<(LocalManifest, cargo_metadata::Package)>);
 
+/// Helper function to check whether a `cargo_metadata::Dependency` is a version dependency.
+fn is_version_dep(dependency: &cargo_metadata::Dependency) -> bool {
+    match dependency.source {
+        // This is the criterion cargo uses (in `SourceId::from_url`) to decide whether a
+        // dependency has the 'registry' kind.
+        Some(ref s) => s.splitn(2, '+').next() == Some("registry"),
+        _ => false,
+    }
+}
+
+fn dry_run_message() -> Result<()> {
+    let bufwtr = BufferWriter::stdout(ColorChoice::Always);
+    let mut buffer = bufwtr.buffer();
+    buffer
+        .set_color(ColorSpec::new().set_fg(Some(Color::Cyan)).set_bold(true))
+        .chain_err(|| "Failed to set output colour")?;
+    write!(&mut buffer, "Starting dry run. ").chain_err(|| "Failed to write dry run message")?;
+    buffer
+        .set_color(&ColorSpec::new())
+        .chain_err(|| "Failed to clear output colour")?;
+    writeln!(&mut buffer, "Changes will not be saved.")
+        .chain_err(|| "Failed to write dry run message")?;
+    bufwtr
+        .print(&buffer)
+        .chain_err(|| "Failed to print dry run message")
+}
+
 impl Manifests {
     /// Get all manifests in the workspace.
     fn get_all(manifest_path: &Option<PathBuf>) -> Result<Self> {
@@ -148,80 +175,16 @@ impl Manifests {
         Ok(Manifests(vec![(manifest, package.to_owned())]))
     }
 
-    fn get_all_requested_dependencies(&self) -> Vec<cargo_metadata::Dependency> {
-        /// Helper function to check whether a `cargo_metadata::Dependency` is a version dependency.
-        fn is_version_dep(dependency: &cargo_metadata::Dependency) -> bool {
-            match dependency.source {
-                // This is the criterion cargo uses (in `SourceId::from_url`) to decide whether a
-                // dependency has the 'registry' kind.
-                Some(ref s) => s.splitn(2, '+').next() == Some("registry"),
-                _ => false,
-            }
-        }
-
-        self.0
-            .iter()
-            .flat_map(|&(_, ref package)| package.dependencies.clone())
-            .filter(is_version_dep)
-            .collect()
-    }
-
-    fn get_locked_dependencies(&self) -> Result<Vec<cargo_metadata::Package>> {
-        Ok(self
-            .0
-            .iter()
-            .map(|(manifest, _package)| {
-                let mut cmd = cargo_metadata::MetadataCommand::new();
-                cmd.manifest_path(manifest.path.clone());
-                cmd.other_options(vec!["--locked".to_string()]);
-
-                let result = cmd
-                    .exec()
-                    .map_err(|e| Error::from(e.compat()).chain_err(|| "Invalid manifest"))?;
-
-                Ok(result
-                    .packages
-                    .into_iter()
-                    .filter(|p| p.source.is_some())
-                    .collect::<Vec<_>>())
-            })
-            .collect::<Result<Vec<_>>>()?
-            .into_iter()
-            .flatten()
-            .collect())
-    }
-
     /// Get the the combined set of dependencies to upgrade. If the user has specified
     /// per-dependency desired versions, extract those here.
-    fn get_dependencies(
-        &self,
-        only_update: Vec<String>,
-        to_lockfile: bool,
-    ) -> Result<DesiredUpgrades> {
-        if to_lockfile {
-            let locked = self.get_locked_dependencies()?;
-            return Ok(DesiredUpgrades(
-                self.get_all_requested_dependencies()
-                    .into_iter()
-                    .filter_map(|d| {
-                        for p in &locked {
-                            // The requested depenency may be present in the lock file with different versions,
-                            // but only one will be semver-compatible with requested version.
-                            if d.name == p.name && d.req.matches(&p.version) {
-                                return Some((d.name, Some(p.version.to_string())));
-                            }
-                        }
-                        return None;
-                    })
-                    .collect(),
-            ));
-        }
-
+    fn get_dependencies(&self, only_update: Vec<String>) -> Result<DesiredUpgrades> {
         Ok(DesiredUpgrades(if only_update.is_empty() {
             // User hasn't asked for any specific dependencies to be upgraded, so upgrade all the
             // dependencies.
-            self.get_all_requested_dependencies()
-                .into_iter()
+            self.0
+                .iter()
+                .flat_map(|&(_, ref package)| package.dependencies.clone())
+                .filter(is_version_dep)
                 .map(|dependency| (dependency.name, None))
                 .collect()
         } else {
@@ -244,21 +207,7 @@ impl Manifests {
     /// Upgrade the manifests on disk following the previously-determined upgrade schema.
     fn upgrade(self, upgraded_deps: &ActualUpgrades, dry_run: bool) -> Result<()> {
         if dry_run {
-            let bufwtr = BufferWriter::stdout(ColorChoice::Always);
-            let mut buffer = bufwtr.buffer();
-            buffer
-                .set_color(ColorSpec::new().set_fg(Some(Color::Cyan)).set_bold(true))
-                .chain_err(|| "Failed to set output colour")?;
-            write!(&mut buffer, "Starting dry run. ")
-                .chain_err(|| "Failed to write dry run message")?;
-            buffer
-                .set_color(&ColorSpec::new())
-                .chain_err(|| "Failed to clear output colour")?;
-            writeln!(&mut buffer, "Changes will not be saved.")
-                .chain_err(|| "Failed to write dry run message")?;
-            bufwtr
-                .print(&buffer)
-                .chain_err(|| "Failed to print dry run message")?;
+            dry_run_message()?;
         }
 
         for (mut manifest, package) in self.0 {
@@ -271,6 +220,58 @@ impl Manifests {
 
         Ok(())
     }
+
+    /// Update dependencies in Cargo.toml file(s) to match the corresponding
+    /// version in Cargo.lock.
+    fn sync_to_lockfile(self, dry_run: bool) -> Result<()> {
+        // Get locked dependencies. If there may be multiple Cargo.toml
+        // files in the workspace, then they all share the same lockfile,
+        // so it suffices to get the query metadata for any one of them.
+        let (manifest, _package) = self.0.iter().next().expect("should have one");
+        let mut cmd = cargo_metadata::MetadataCommand::new();
+        cmd.manifest_path(manifest.path.clone());
+        cmd.other_options(vec!["--locked".to_string()]);
+
+        let result = cmd
+            .exec()
+            .map_err(|e| Error::from(e.compat()).chain_err(|| "Invalid manifest"))?;
+
+        let locked = result
+            .packages
+            .into_iter()
+            .filter(|p| p.source.is_some()) // Source is none for local packages
+            .collect::<Vec<_>>();
+
+        if dry_run {
+            dry_run_message()?;
+        }
+
+        for (mut manifest, package) in self.0 {
+            println!("{}:", package.name);
+
+            // Upgrade the manifests one at a time, as multiple manifests may
+            // request the same dependency at differing versions.
+            for (name, version) in package
+                .dependencies
+                .clone()
+                .into_iter()
+                .filter(is_version_dep)
+                .filter_map(|d| {
+                    for p in &locked {
+                        // The requested depenency may be present in the lock file with different versions,
+                        // but only one will be semver-compatible with requested version.
+                        if d.name == p.name && d.req.matches(&p.version) {
+                            return Some((d.name, p.version.to_string()));
+                        }
+                    }
+                    return None;
+                })
+            {
+                manifest.upgrade(&Dependency::new(&name).set_version(&version), dry_run)?;
+            }
+        }
+        Ok(())
+    }
 }
 
 /// The set of dependencies to be upgraded, alongside desired versions, if specified by the user.
@@ -331,12 +332,16 @@ fn process(args: Args) -> Result<()> {
         Manifests::get_local_one(&manifest_path)
     }?;
 
-    let existing_dependencies = manifests.get_dependencies(dependency, to_lockfile)?;
+    if to_lockfile {
+        manifests.sync_to_lockfile(dry_run)
+    } else {
+        let existing_dependencies = manifests.get_dependencies(dependency)?;
 
-    let upgraded_dependencies =
-        existing_dependencies.get_upgraded(allow_prerelease, &find(&manifest_path)?)?;
+        let upgraded_dependencies =
+            existing_dependencies.get_upgraded(allow_prerelease, &find(&manifest_path)?)?;
 
-    manifests.upgrade(&upgraded_dependencies, dry_run)
+        manifests.upgrade(&upgraded_dependencies, dry_run)
+    }
 }
 
 fn main() {

tests/cargo-upgrade.rs

@@ -38,6 +38,7 @@ pub fn copy_workspace_test() -> (tempdir::TempDir, String, Vec<String>) {
 
         let root_manifest_path = copy_in(".", "Cargo.toml");
         copy_in(".", "dummy.rs");
+        copy_in(".", "Cargo.lock");
 
         let workspace_manifest_paths = ["one", "two", "implicit/three", "explicit/four"]
             .iter()
@@ -326,6 +327,23 @@ fn upgrade_to_lockfile() {
     assert_eq!(target.to_string(), upgraded.to_string());
 }
 
+#[test]
+fn upgrade_workspace_to_lockfile() {
+    let (tmpdir, root_manifest, _workspace_manifests) = copy_workspace_test();
+
+    execute_command(&["upgrade", "--all", "--to-lockfile"], &root_manifest);
+
+    // The members one and two both request different, semver incompatible
+    // versions of rand. Test that both were upgraded correctly.
+    let one_upgraded = get_toml(tmpdir.path().join("one/Cargo.toml").to_str().unwrap());
+    let one_target = get_toml("tests/fixtures/workspace/one/Cargo.toml.lockfile_target");
+    assert_eq!(one_target.to_string(), one_upgraded.to_string());
+
+    let two_upgraded = get_toml(tmpdir.path().join("two/Cargo.toml").to_str().unwrap());
+    let two_target = get_toml("tests/fixtures/workspace/two/Cargo.toml.lockfile_target");
+    assert_eq!(two_target.to_string(), two_upgraded.to_string());
+}
+
 #[test]
 #[cfg(feature = "test-external-apis")]
 fn upgrade_prints_messages() {