Using required dep as a feature is a warning for now, not an error

Author: RalfJung

src/cargo/core/resolver/mod.rs

@@ -57,6 +57,7 @@ use url::Url;
 
 use core::{PackageId, Registry, SourceId, Summary, Dependency};
 use core::PackageIdSpec;
+use core::shell::Shell;
 use util::Graph;
 use util::errors::{CargoResult, CargoError};
 use util::profile;
@@ -330,20 +331,25 @@ struct Context<'a> {
     resolve_replacements: RcList<(PackageId, PackageId)>,
 
     replacements: &'a [(PackageIdSpec, Dependency)],
+
+    // These warnings are printed after resolution.
+    warnings: RcList<String>,
 }
 
 type Activations = HashMap<String, HashMap<SourceId, Vec<Summary>>>;
 
 /// Builds the list of all packages required to build the first argument.
 pub fn resolve(summaries: &[(Summary, Method)],
                replacements: &[(PackageIdSpec, Dependency)],
-               registry: &mut Registry) -> CargoResult<Resolve> {
+               registry: &mut Registry,
+               shell: Option<&mut Shell>) -> CargoResult<Resolve> {
     let cx = Context {
         resolve_graph: RcList::new(),
         resolve_features: HashMap::new(),
         resolve_replacements: RcList::new(),
         activations: HashMap::new(),
         replacements: replacements,
+        warnings: RcList::new(),
     };
     let _p = profile::start(format!("resolving"));
     let cx = activate_deps_loop(cx, registry, summaries)?;
@@ -368,8 +374,17 @@ pub fn resolve(summaries: &[(Summary, Method)],
     }
 
     check_cycles(&resolve, &cx.activations)?;
-
     trace!("resolved: {:?}", resolve);
+
+    // If we have a shell, emit warnings about required deps used as feature.
+    if let Some(shell) = shell {
+        let mut warnings = &cx.warnings;
+        while let Some(ref head) = warnings.head {
+            shell.warn(&head.0)?;
+            warnings = &head.1;
+        }
+    }
+
     Ok(resolve)
 }
 
@@ -1088,9 +1103,13 @@ impl<'a> Context<'a> {
             // to `ret`.
             let base = feature_deps.remove(dep.name()).unwrap_or((false, vec![]));
             if !dep.is_optional() && base.0 {
-                bail!("Package `{}` does not have feature `{}`. It has a required dependency with \
-                       that name, but only optional dependencies can be used as features.",
-                      s.package_id(), dep.name());
+                self.warnings.push(
+                    format!("Package `{}` does not have feature `{}`. It has a required dependency \
+                       with that name, but only optional dependencies can be used as features. \
+                       This is currently a warning to ease the transition, but it will become an \
+                       error in the future.",
+                       s.package_id(), dep.name())
+                );
             }
             let mut base = base.1;
             base.extend(dep.features().iter().cloned());

src/cargo/ops/cargo_generate_lockfile.rs

@@ -19,7 +19,7 @@ pub fn generate_lockfile(ws: &Workspace) -> CargoResult<()> {
     let mut registry = PackageRegistry::new(ws.config())?;
     let resolve = ops::resolve_with_previous(&mut registry, ws,
                                              Method::Everything,
-                                             None, None, &[])?;
+                                             None, None, &[], true)?;
     ops::write_pkg_lockfile(ws, &resolve)?;
     Ok(())
 }
@@ -79,7 +79,8 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions)
                                                   Method::Everything,
                                                   Some(&previous_resolve),
                                                   Some(&to_avoid),
-                                                  &[])?;
+                                                  &[],
+                                                  true)?;
 
     // Summarize what is changing for the user.
     let print_change = |status: &str, msg: String| {

src/cargo/ops/resolve.rs

@@ -15,7 +15,7 @@ use util::errors::{CargoResult, CargoResultExt};
 /// lockfile.
 pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolve)> {
     let mut registry = PackageRegistry::new(ws.config())?;
-    let resolve = resolve_with_registry(ws, &mut registry)?;
+    let resolve = resolve_with_registry(ws, &mut registry, true)?;
     let packages = get_resolved_packages(&resolve, registry);
     Ok((packages, resolve))
 }
@@ -44,7 +44,7 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>,
     let resolve = if ws.require_optional_deps() {
         // First, resolve the root_package's *listed* dependencies, as well as
         // downloading and updating all remotes and such.
-        let resolve = resolve_with_registry(ws, &mut registry)?;
+        let resolve = resolve_with_registry(ws, &mut registry, false)?;
 
         // Second, resolve with precisely what we're doing. Filter out
         // transitive dependencies if necessary, specify features, handle
@@ -79,19 +79,19 @@ pub fn resolve_ws_precisely<'a>(ws: &Workspace<'a>,
     let resolved_with_overrides =
     ops::resolve_with_previous(&mut registry, ws,
                                method, resolve.as_ref(), None,
-                               specs)?;
+                               specs, true)?;
 
     let packages = get_resolved_packages(&resolved_with_overrides, registry);
 
     Ok((packages, resolved_with_overrides))
 }
 
-fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry)
+fn resolve_with_registry(ws: &Workspace, registry: &mut PackageRegistry, warn: bool)
                          -> CargoResult<Resolve> {
     let prev = ops::load_pkg_lockfile(ws)?;
     let resolve = resolve_with_previous(registry, ws,
                                         Method::Everything,
-                                        prev.as_ref(), None, &[])?;
+                                        prev.as_ref(), None, &[], warn)?;
 
     if !ws.is_ephemeral() {
         ops::write_pkg_lockfile(ws, &resolve)?;
@@ -114,7 +114,8 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
                                  method: Method,
                                  previous: Option<&'a Resolve>,
                                  to_avoid: Option<&HashSet<&'a PackageId>>,
-                                 specs: &[PackageIdSpec])
+                                 specs: &[PackageIdSpec],
+                                 warn: bool)
                                  -> CargoResult<Resolve> {
     // Here we place an artificial limitation that all non-registry sources
     // cannot be locked at more than one revision. This means that if a git
@@ -256,9 +257,17 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
         None => root_replace.to_vec(),
     };
 
+    let mut shell;
+    let opt_shell = if warn {
+        shell = ws.config().shell();
+        Some(&mut *shell)
+    } else {
+        None
+    };
     let mut resolved = resolver::resolve(&summaries,
                                          &replace,
-                                         registry)?;
+                                         registry,
+                                         opt_shell)?;
     resolved.register_used_patches(registry.patches());
     if let Some(previous) = previous {
         resolved.merge_from(previous)?;

tests/features.rs

@@ -237,7 +237,7 @@ fn invalid9() {
             [dependencies.bar]
             path = "bar"
         "#)
-        .file("src/main.rs", "")
+        .file("src/main.rs", "fn main() {}")
         .file("bar/Cargo.toml", r#"
             [package]
             name = "bar"
@@ -247,9 +247,12 @@ fn invalid9() {
         .file("bar/src/lib.rs", "");
 
     assert_that(p.cargo_process("build").arg("--features").arg("bar"),
-                execs().with_status(101).with_stderr("\
-[ERROR] Package `foo v0.0.1 ([..])` does not have feature `bar`. It has a required dependency with \
-that name, but only optional dependencies can be used as features.
+                execs().with_status(0).with_stderr("\
+warning: Package `foo v0.0.1 ([..])` does not have feature `bar`. It has a required dependency with \
+that name, but only optional dependencies can be used as features. [..]
+   Compiling bar v0.0.1 ([..])
+   Compiling foo v0.0.1 ([..])
+    Finished dev [unoptimized + debuginfo] target(s) in [..] secs
 "));
 }
 
@@ -266,7 +269,7 @@ fn invalid10() {
             path = "bar"
             features = ["baz"]
         "#)
-        .file("src/main.rs", "")
+        .file("src/main.rs", "fn main() {}")
         .file("bar/Cargo.toml", r#"
             [package]
             name = "bar"
@@ -286,9 +289,13 @@ fn invalid10() {
         .file("bar/baz/src/lib.rs", "");
 
     assert_that(p.cargo_process("build"),
-                execs().with_status(101).with_stderr("\
-[ERROR] Package `bar v0.0.1 ([..])` does not have feature `baz`. It has a required dependency with \
-that name, but only optional dependencies can be used as features.
+                execs().with_status(0).with_stderr("\
+warning: Package `bar v0.0.1 ([..])` does not have feature `baz`. It has a required dependency with \
+that name, but only optional dependencies can be used as features. [..]
+   Compiling baz v0.0.1 ([..])
+   Compiling bar v0.0.1 ([..])
+   Compiling foo v0.0.1 ([..])
+    Finished dev [unoptimized + debuginfo] target(s) in [..] secs
 "));
 }
 

tests/resolve.rs

@@ -32,7 +32,7 @@ fn resolve(pkg: PackageId, deps: Vec<Dependency>, registry: &[Summary])
     let mut registry = MyRegistry(registry);
     let summary = Summary::new(pkg.clone(), deps, HashMap::new()).unwrap();
     let method = Method::Everything;
-    let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry)?;
+    let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry, None)?;
     let res = resolve.iter().cloned().collect();
     Ok(res)
 }