Auto merge of #6814 - ehuss:offline-optional-dep, r=alexcrichton

Resolve: Be less strict while offline.

When offline, the resolver was requiring everything to be downloaded, even dependencies that are not used. This changes it so that the resolver can still resolve unavailable dependencies when offline. This pushes the failure to a later stage of Cargo where it attempts to download the dependency. This makes `-Z offline` work for target-cfg or optional dependencies that are not being used.

Fixes #6014.

This changes the error message significantly for the "unavailable" case (see test diff). I personally think the new error message is clearer, although it is shorter and provides less information. The old error message seemed large and scary, and was a little hard for me to grok. However, I'd be willing to look at tweaking the error behavior if not everyone agrees.

Author: bors

src/cargo/core/package.rs

@@ -476,6 +476,12 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> {
     /// eventually be returned from `wait_for_download`. Returns `Some(pkg)` if
     /// the package is ready and doesn't need to be downloaded.
     pub fn start(&mut self, id: PackageId) -> CargoResult<Option<&'a Package>> {
+        Ok(self
+            .start_inner(id)
+            .chain_err(|| format!("failed to download `{}`", id))?)
+    }
+
+    fn start_inner(&mut self, id: PackageId) -> CargoResult<Option<&'a Package>> {
         // First up see if we've already cached this package, in which case
         // there's nothing to do.
         let slot = self

src/cargo/sources/registry/index.rs

@@ -200,7 +200,6 @@ impl<'cfg> RegistryIndex<'cfg> {
                 ret.reserve(contents.lines().count());
                 let lines = contents.lines().map(|s| s.trim()).filter(|l| !l.is_empty());
 
-                let online = !self.config.cli_unstable().offline;
                 // Attempt forwards-compatibility on the index by ignoring
                 // everything that we ourselves don't understand, that should
                 // allow future cargo implementations to break the
@@ -215,11 +214,7 @@ impl<'cfg> RegistryIndex<'cfg> {
                             return None;
                         }
                     };
-                    if online || load.is_crate_downloaded(summary.package_id()) {
-                        Some((summary, locked))
-                    } else {
-                        None
-                    }
+                    Some((summary, locked))
                 }));
 
                 Ok(())
@@ -274,17 +269,43 @@ impl<'cfg> RegistryIndex<'cfg> {
         yanked_whitelist: &HashSet<PackageId>,
         f: &mut dyn FnMut(Summary),
     ) -> CargoResult<()> {
+        if self.config.cli_unstable().offline {
+            if self.query_inner_with_online(dep, load, yanked_whitelist, f, false)? != 0 {
+                return Ok(());
+            }
+            // If offline, and there are no matches, try again with online.
+            // This is necessary for dependencies that are not used (such as
+            // target-cfg or optional), but are not downloaded. Normally the
+            // build should succeed if they are not downloaded and not used,
+            // but they still need to resolve. If they are actually needed
+            // then cargo will fail to download and an error message
+            // indicating that the required dependency is unavailable while
+            // offline will be displayed.
+        }
+        self.query_inner_with_online(dep, load, yanked_whitelist, f, true)?;
+        Ok(())
+    }
+
+    fn query_inner_with_online(
+        &mut self,
+        dep: &Dependency,
+        load: &mut dyn RegistryData,
+        yanked_whitelist: &HashSet<PackageId>,
+        f: &mut dyn FnMut(Summary),
+        online: bool,
+    ) -> CargoResult<usize> {
         let source_id = self.source_id;
         let name = dep.package_name().as_str();
         let summaries = self.summaries(name, load)?;
         let summaries = summaries
             .iter()
             .filter(|&(summary, yanked)| {
-                !yanked || {
-                    log::debug!("{:?}", yanked_whitelist);
-                    log::debug!("{:?}", summary.package_id());
-                    yanked_whitelist.contains(&summary.package_id())
-                }
+                (online || load.is_crate_downloaded(summary.package_id()))
+                    && (!yanked || {
+                        log::debug!("{:?}", yanked_whitelist);
+                        log::debug!("{:?}", summary.package_id());
+                        yanked_whitelist.contains(&summary.package_id())
+                    })
             })
             .map(|s| s.0.clone());
 
@@ -308,9 +329,11 @@ impl<'cfg> RegistryIndex<'cfg> {
             _ => true,
         });
 
+        let mut count = 0;
         for summary in summaries {
             f(summary);
+            count += 1;
         }
-        Ok(())
+        Ok(count)
     }
 }

tests/testsuite/build.rs

@@ -1140,6 +1140,77 @@ fn main(){
         .run();
 }
 
+#[test]
+fn offline_unused_target_dep() {
+    // -Z offline with a target dependency that is not used and not downloaded.
+    Package::new("unused_dep", "1.0.0").publish();
+    Package::new("used_dep", "1.0.0").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [project]
+            name = "foo"
+            version = "0.1.0"
+            [dependencies]
+            used_dep = "1.0"
+            [target.'cfg(unused)'.dependencies]
+            unused_dep = "1.0"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+    // Do a build that downloads only what is necessary.
+    p.cargo("build")
+        .with_stderr_contains("[DOWNLOADED] used_dep [..]")
+        .with_stderr_does_not_contain("[DOWNLOADED] unused_dep [..]")
+        .run();
+    p.cargo("clean").run();
+    // Build offline, make sure it works.
+    p.cargo("build -Z offline")
+        .masquerade_as_nightly_cargo()
+        .run();
+}
+
+#[test]
+fn offline_missing_optional() {
+    Package::new("opt_dep", "1.0.0").publish();
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+            [project]
+            name = "foo"
+            version = "0.1.0"
+            [dependencies]
+            opt_dep = { version = "1.0", optional = true }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+    // Do a build that downloads only what is necessary.
+    p.cargo("build")
+        .with_stderr_does_not_contain("[DOWNLOADED] opt_dep [..]")
+        .run();
+    p.cargo("clean").run();
+    // Build offline, make sure it works.
+    p.cargo("build -Z offline")
+        .masquerade_as_nightly_cargo()
+        .run();
+    p.cargo("build -Z offline --features=opt_dep")
+        .masquerade_as_nightly_cargo()
+        .with_stderr(
+            "\
+[ERROR] failed to download `opt_dep v1.0.0`
+
+Caused by:
+  can't make HTTP request in the offline mode
+",
+        )
+        .with_status(101)
+        .run();
+}
+
 #[test]
 fn incompatible_dependencies() {
     Package::new("bad", "0.1.0").publish();
@@ -1282,14 +1353,11 @@ fn compile_offline_while_transitive_dep_not_cached() {
         .with_status(101)
         .with_stderr(
             "\
-error: no matching package named `baz` found
-location searched: registry `[..]`
-required by package `bar v0.1.0`
-    ... which is depended on by `foo v0.0.1 ([CWD])`
-As a reminder, you're using offline mode (-Z offline) \
-which can sometimes cause surprising resolution failures, \
-if this error is too confusing you may wish to retry \
-without the offline flag.",
+[ERROR] failed to download `baz v1.0.0`
+
+Caused by:
+  can't make HTTP request in the offline mode
+",
         )
         .run();
 }

tests/testsuite/support/registry.rs

@@ -126,6 +126,7 @@ pub fn alt_api_url() -> Url {
 ///
 /// p.cargo("run").with_stdout("24").run();
 /// ```
+#[must_use]
 pub struct Package {
     name: String,
     vers: String,