Auto merge of #5831 - Eh2406:i5684, r=alexcrichton

cargo can silently fix some bad lockfiles (use --locked to disable)

Lock files often get corrupted by git merge. This makes all cargo commands silently fix that kind of corruption.

If you want to be sure that your CI does not change the lock file you have commited
---

Then make sure to use `--locked` in your CI

Edit: original description below

---------------

This is a continuation of @dwijnand work in #5809, and closes #5684

This adds a `ignore_errors` arg to reading a lock file which ignores sections it doesn't understand. Specifically things that depend on versions that don't exist in the lock file. Then all users pass false except for the two that relate to `update` command.

I think the open questions for this pr relate to testing.
- Now that we are passing false in all other commands, do they each need a test for a bad lockfile?
- Do we need a test with a more subtly corrupted lock file, or is this always sufficient for `update` to clean up?

Author: bors

src/cargo/core/resolver/encode.rs

@@ -2,12 +2,12 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 use std::fmt;
 use std::str::FromStr;
 
-use serde::ser;
 use serde::de;
+use serde::ser;
 
 use core::{Dependency, Package, PackageId, SourceId, Workspace};
-use util::{internal, Graph};
 use util::errors::{CargoError, CargoResult, CargoResultExt};
+use util::{internal, Graph};
 
 use super::Resolve;
 
@@ -43,7 +43,7 @@ impl EncodableResolve {
 
         // `PackageId`s in the lock file don't include the `source` part
         // for workspace members, so we reconstruct proper ids.
-        let (live_pkgs, all_pkgs) = {
+        let live_pkgs = {
             let mut live_pkgs = HashMap::new();
             let mut all_pkgs = HashSet::new();
             for pkg in packages.iter() {
@@ -54,10 +54,7 @@ impl EncodableResolve {
                 };
 
                 if !all_pkgs.insert(enc_id.clone()) {
-                    bail!(
-                        "package `{}` is specified twice in the lockfile",
-                        pkg.name
-                    );
+                    bail!("package `{}` is specified twice in the lockfile", pkg.name);
                 }
                 let id = match pkg.source.as_ref().or_else(|| path_deps.get(&pkg.name)) {
                     // We failed to find a local package in the workspace.
@@ -71,24 +68,11 @@ impl EncodableResolve {
 
                 assert!(live_pkgs.insert(enc_id, (id, pkg)).is_none())
             }
-            (live_pkgs, all_pkgs)
+            live_pkgs
         };
 
-        let lookup_id = |enc_id: &EncodablePackageId| -> CargoResult<Option<PackageId>> {
-            match live_pkgs.get(enc_id) {
-                Some(&(ref id, _)) => Ok(Some(id.clone())),
-                None => if all_pkgs.contains(enc_id) {
-                    // Package is found in the lockfile, but it is
-                    // no longer a member of the workspace.
-                    Ok(None)
-                } else {
-                    bail!(
-                        "package `{}` is specified as a dependency, \
-                         but is missing from the package list",
-                        enc_id
-                    );
-                },
-            }
+        let lookup_id = |enc_id: &EncodablePackageId| -> Option<PackageId> {
+            live_pkgs.get(enc_id).map(|&(ref id, _)| id.clone())
         };
 
         let g = {
@@ -105,7 +89,7 @@ impl EncodableResolve {
                 };
 
                 for edge in deps.iter() {
-                    if let Some(to_depend_on) = lookup_id(edge)? {
+                    if let Some(to_depend_on) = lookup_id(edge) {
                         g.link(id.clone(), to_depend_on);
                     }
                 }
@@ -118,7 +102,7 @@ impl EncodableResolve {
             for &(ref id, pkg) in live_pkgs.values() {
                 if let Some(ref replace) = pkg.replace {
                     assert!(pkg.dependencies.is_none());
-                    if let Some(replace_id) = lookup_id(replace)? {
+                    if let Some(replace_id) = lookup_id(replace) {
                         replacements.insert(id.clone(), replace_id);
                     }
                 }
@@ -149,10 +133,11 @@ impl EncodableResolve {
         for (k, v) in metadata.iter().filter(|p| p.0.starts_with(prefix)) {
             to_remove.push(k.to_string());
             let k = &k[prefix.len()..];
-            let enc_id: EncodablePackageId = k.parse()
+            let enc_id: EncodablePackageId = k
+                .parse()
                 .chain_err(|| internal("invalid encoding of checksum in lockfile"))?;
             let id = match lookup_id(&enc_id) {
-                Ok(Some(id)) => id,
+                Some(id) => id,
                 _ => continue,
             };
 
@@ -193,7 +178,8 @@ fn build_path_deps(ws: &Workspace) -> HashMap<String, SourceId> {
     // such as `cargo install` with a lock file from a remote dependency. In
     // that case we don't need to fixup any path dependencies (as they're not
     // actually path dependencies any more), so we ignore them.
-    let members = ws.members()
+    let members = ws
+        .members()
         .filter(|p| p.package_id().source_id().is_path())
         .collect::<Vec<_>>();
 
@@ -293,7 +279,8 @@ impl FromStr for EncodablePackageId {
     fn from_str(s: &str) -> CargoResult<EncodablePackageId> {
         let mut s = s.splitn(3, ' ');
         let name = s.next().unwrap();
-        let version = s.next()
+        let version = s
+            .next()
             .ok_or_else(|| internal("invalid serialized PackageId"))?;
         let source_id = match s.next() {
             Some(s) => {
@@ -349,7 +336,8 @@ impl<'a, 'cfg> ser::Serialize for WorkspaceResolve<'a, 'cfg> {
         let mut ids: Vec<_> = self.resolve.iter().collect();
         ids.sort();
 
-        let encodable = ids.iter()
+        let encodable = ids
+            .iter()
             .filter_map(|&id| Some(encodable_resolve_node(id, self.resolve)))
             .collect::<Vec<_>>();
 
@@ -371,7 +359,8 @@ impl<'a, 'cfg> ser::Serialize for WorkspaceResolve<'a, 'cfg> {
         };
 
         let patch = Patch {
-            unused: self.resolve
+            unused: self
+                .resolve
                 .unused_patches()
                 .iter()
                 .map(|id| EncodableDependency {

src/cargo/ops/cargo_generate_lockfile.rs

@@ -2,10 +2,10 @@ use std::collections::{BTreeMap, HashSet};
 
 use termcolor::Color::{self, Cyan, Green, Red};
 
-use core::PackageId;
 use core::registry::PackageRegistry;
-use core::{Resolve, SourceId, Workspace};
 use core::resolver::Method;
+use core::PackageId;
+use core::{Resolve, SourceId, Workspace};
 use ops;
 use util::config::Config;
 use util::CargoResult;

src/cargo/ops/cargo_pkgid.rs

@@ -1,5 +1,5 @@
-use ops;
 use core::{PackageIdSpec, Workspace};
+use ops;
 use util::CargoResult;
 
 pub fn pkgid(ws: &Workspace, spec: Option<&str>) -> CargoResult<PackageIdSpec> {

src/cargo/ops/lockfile.rs

@@ -2,11 +2,11 @@ use std::io::prelude::*;
 
 use toml;
 
-use core::{resolver, Resolve, Workspace};
 use core::resolver::WorkspaceResolve;
-use util::Filesystem;
+use core::{resolver, Resolve, Workspace};
 use util::errors::{CargoResult, CargoResultExt};
 use util::toml as cargo_toml;
+use util::Filesystem;
 
 pub fn load_pkg_lockfile(ws: &Workspace) -> CargoResult<Option<Resolve>> {
     if !ws.root().join("Cargo.lock").exists() {
@@ -25,8 +25,7 @@ pub fn load_pkg_lockfile(ws: &Workspace) -> CargoResult<Option<Resolve>> {
             let resolve: toml::Value = cargo_toml::parse(&s, f.path(), ws.config())?;
             let v: resolver::EncodableResolve = resolve.try_into()?;
             Ok(Some(v.into_resolve(ws)?))
-        })()
-            .chain_err(|| format!("failed to parse lock file at: {}", f.path().display()))?;
+        })().chain_err(|| format!("failed to parse lock file at: {}", f.path().display()))?;
     Ok(resolve)
 }
 

src/cargo/ops/resolve.rs

@@ -1,12 +1,12 @@
 use std::collections::HashSet;
 
-use core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace};
 use core::registry::PackageRegistry;
 use core::resolver::{self, Method, Resolve};
-use sources::PathSource;
+use core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace};
 use ops;
-use util::profile;
+use sources::PathSource;
 use util::errors::{CargoResult, CargoResultExt};
+use util::profile;
 
 /// Resolve all dependencies for the workspace using the previous
 /// lockfile as a guide if present.
@@ -272,7 +272,11 @@ pub fn resolve_with_previous<'a, 'cfg>(
                 // workspace, then we use `method` specified. Otherwise we use a
                 // base method with no features specified but using default features
                 // for any other packages specified with `-p`.
-                Method::Required { dev_deps, all_features, .. } => {
+                Method::Required {
+                    dev_deps,
+                    all_features,
+                    ..
+                } => {
                     let base = Method::Required {
                         dev_deps,
                         features: &[],
@@ -335,7 +339,10 @@ pub fn resolve_with_previous<'a, 'cfg>(
 
 /// Read the `paths` configuration variable to discover all path overrides that
 /// have been configured.
-pub fn add_overrides<'a>(registry: &mut PackageRegistry<'a>, ws: &Workspace<'a>) -> CargoResult<()> {
+pub fn add_overrides<'a>(
+    registry: &mut PackageRegistry<'a>,
+    ws: &Workspace<'a>,
+) -> CargoResult<()> {
     let paths = match ws.config().get_list("paths")? {
         Some(list) => list,
         None => return Ok(()),
@@ -364,7 +371,10 @@ pub fn add_overrides<'a>(registry: &mut PackageRegistry<'a>, ws: &Workspace<'a>)
     Ok(())
 }
 
-pub fn get_resolved_packages<'a>(resolve: &Resolve, registry: PackageRegistry<'a>) -> PackageSet<'a> {
+pub fn get_resolved_packages<'a>(
+    resolve: &Resolve,
+    registry: PackageRegistry<'a>,
+) -> PackageSet<'a> {
     let ids: Vec<PackageId> = resolve.iter().cloned().collect();
     registry.get(&ids)
 }
@@ -494,7 +504,8 @@ fn register_previous_locks<'a>(
             //       dependency on that crate to enable the feature. For now
             //       this bug is better than the always updating registry
             //       though...
-            if !ws.members()
+            if !ws
+                .members()
                 .any(|pkg| pkg.package_id() == member.package_id())
                 && (dep.is_optional() || !dep.is_transitive())
             {

tests/testsuite/bad_config.rs

@@ -1,6 +1,6 @@
-use support::{basic_manifest, execs, project};
-use support::registry::Package;
 use support::hamcrest::assert_that;
+use support::registry::Package;
+use support::{basic_manifest, execs, project};
 
 #[test]
 fn bad1() {
@@ -153,13 +153,13 @@ fn bad_cargo_config_jobs() {
         .build();
     assert_that(
         p.cargo("build").arg("-v"),
-        execs()
-            .with_status(101)
-            .with_stderr("\
+        execs().with_status(101).with_stderr(
+            "\
 [ERROR] error in [..].cargo[/]config: \
 could not load config key `build.jobs`: \
 invalid value: integer `-1`, expected u32
-"),
+",
+        ),
     );
 }
 
@@ -371,17 +371,7 @@ fn bad_dependency_in_lockfile() {
         )
         .build();
 
-    assert_that(
-        p.cargo("build"),
-        execs().with_status(101).with_stderr(
-            "\
-[ERROR] failed to parse lock file at: [..]
-
-Caused by:
-  package `bar 0.1.0 ([..])` is specified as a dependency, but is missing from the package list
-",
-        ),
-    );
+    assert_that(p.cargo("build"), execs().with_status(0));
 }
 
 #[test]
@@ -754,7 +744,8 @@ warning: unused manifest key: project.bulid
         ),
     );
 
-    let p = project().at("bar")
+    let p = project()
+        .at("bar")
         .file(
             "Cargo.toml",
             r#"