From e82443dfd8944d243cfeea8d7b49f4690ebf7892 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 7 Sep 2018 09:37:06 -0700 Subject: [PATCH] Fix publishing renamed dependencies to crates.io This commit fixes publishing crates which contain locally renamed dependencies to crates.io. Previously this lack of information meant that although we could resolve the crate graph correctly it wouldn't work well with respect to optional features and optional dependencies. The fix here is to persist this information into the registry about the crate being renamed in `Cargo.toml`, allowing Cargo to correctly deduce feature names as it does when it has `Cargo.toml` locally. A dual side of this commit is to publish this information to crates.io. We'll want to merge the associated PR (link to come soon) on crates.io first and make sure that's deployed as well before we stabilize the crate renaming feature. The index format is updated as well as part of this change. The `name` key for dependencies is now unconditionally what was written in `Cargo.toml` as the left-hand-side of the dependency specification. In other words this is the raw crate name, but only for the local crate. A new key, `package`, is added to dependencies (and it can be `None`). This key indicates the crates.io package is being linked against, an represents the `package` key in `Cargo.toml`. It's important to consider the interaction with older Cargo implementations which don't support the `package` key in the index. In these situations older Cargo binaries will likely fail to resolve entirely as the renamed name is unlikely to exist on crates.io. For example the `futures` crate now has an optional dependency with the name `futures01` which depends on an older version of `futures` on crates.io. The string `futures01` will be listed in the index under the `"name"` key, but no `futures01` crate exists on crates.io so older Cargo will generate an error. If the crate does exist on crates.io, then even weirder error messages will likely result. Closes #5962 --- src/cargo/core/dependency.rs | 16 ++-- src/cargo/core/resolver/resolve.rs | 2 +- src/cargo/ops/registry.rs | 1 + src/cargo/sources/registry/mod.rs | 12 ++- src/cargo/util/toml/mod.rs | 26 +++--- src/crates-io/lib.rs | 5 +- tests/testsuite/registry.rs | 56 ++++++++++++- tests/testsuite/support/registry.rs | 119 ++++++++++++++++++++-------- 8 files changed, 179 insertions(+), 58 deletions(-) diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index 0d98b3e884a..c24853f5192 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -28,7 +28,7 @@ struct Inner { specified_req: bool, kind: Kind, only_match_name: bool, - rename: Option, + explicit_name_in_toml: Option, optional: bool, default_features: bool, @@ -73,7 +73,7 @@ impl ser::Serialize for Dependency { uses_default_features: self.uses_default_features(), features: self.features(), target: self.platform(), - rename: self.rename().map(|s| s.as_str()), + rename: self.explicit_name_in_toml().map(|s| s.as_str()), }.serialize(s) } } @@ -199,7 +199,7 @@ impl Dependency { default_features: true, specified_req: false, platform: None, - rename: None, + explicit_name_in_toml: None, }), } } @@ -229,7 +229,7 @@ impl Dependency { /// foo = { version = "0.1", package = 'bar' } /// ``` pub fn name_in_toml(&self) -> InternedString { - self.rename().unwrap_or(self.inner.name) + self.explicit_name_in_toml().unwrap_or(self.inner.name) } /// The name of the package that this `Dependency` depends on. @@ -285,8 +285,8 @@ impl Dependency { /// /// If the `package` key is used in `Cargo.toml` then this returns the same /// value as `name_in_toml`. - pub fn rename(&self) -> Option { - self.inner.rename + pub fn explicit_name_in_toml(&self) -> Option { + self.inner.explicit_name_in_toml } pub fn set_kind(&mut self, kind: Kind) -> &mut Dependency { @@ -330,8 +330,8 @@ impl Dependency { self } - pub fn set_rename(&mut self, rename: &str) -> &mut Dependency { - Rc::make_mut(&mut self.inner).rename = Some(InternedString::new(rename)); + pub fn set_explicit_name_in_toml(&mut self, name: &str) -> &mut Dependency { + Rc::make_mut(&mut self.inner).explicit_name_in_toml = Some(InternedString::new(name)); self } diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 2aad009a143..3867b5566a3 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -227,7 +227,7 @@ unable to verify that `{0}` is the same as when the lockfile was generated let crate_name = to_target.crate_name(); let mut names = deps.iter() - .map(|d| d.rename().map(|s| s.as_str()).unwrap_or(&crate_name)); + .map(|d| d.explicit_name_in_toml().map(|s| s.as_str()).unwrap_or(&crate_name)); let name = names.next().unwrap_or(&crate_name); for n in names { if n == name { diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 9216f903599..34c8fa0bbe4 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -177,6 +177,7 @@ fn transmit( Kind::Development => "dev", }.to_string(), registry: dep_registry, + explicit_name_in_toml: dep.explicit_name_in_toml().map(|s| s.to_string()), }) }) .collect::>>()?; diff --git a/src/cargo/sources/registry/mod.rs b/src/cargo/sources/registry/mod.rs index 7dc1ea2813f..fa36a0c0ce9 100644 --- a/src/cargo/sources/registry/mod.rs +++ b/src/cargo/sources/registry/mod.rs @@ -275,6 +275,7 @@ struct RegistryDependency<'a> { target: Option>, kind: Option>, registry: Option>, + package: Option>, } impl<'a> RegistryDependency<'a> { @@ -289,6 +290,7 @@ impl<'a> RegistryDependency<'a> { target, kind, registry, + package, } = self; let id = if let Some(registry) = registry { @@ -297,7 +299,15 @@ impl<'a> RegistryDependency<'a> { default.clone() }; - let mut dep = Dependency::parse_no_deprecated(&name, Some(&req), &id)?; + + let mut dep = Dependency::parse_no_deprecated( + package.as_ref().unwrap_or(&name), + Some(&req), + &id, + )?; + if package.is_some() { + dep.set_explicit_name_in_toml(&name); + } let kind = match kind.as_ref().map(|s| &s[..]).unwrap_or("") { "dev" => Kind::Development, "build" => Kind::Build, diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 9d83668569a..78a8407b8b4 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1239,7 +1239,7 @@ impl TomlDependency { impl DetailedTomlDependency { fn to_dependency( &self, - name: &str, + name_in_toml: &str, cx: &mut Context, kind: Option, ) -> CargoResult { @@ -1249,7 +1249,7 @@ impl DetailedTomlDependency { providing a local path, Git repository, or \ version to use. This will be considered an \ error in future versions", - name + name_in_toml ); cx.warnings.push(msg); } @@ -1266,7 +1266,7 @@ impl DetailedTomlDependency { let msg = format!( "key `{}` is ignored for dependency ({}). \ This will be considered an error in future versions", - key_name, name + key_name, name_in_toml ); cx.warnings.push(msg) } @@ -1290,12 +1290,12 @@ impl DetailedTomlDependency { (Some(_), _, Some(_), _) | (Some(_), _, _, Some(_)) => bail!( "dependency ({}) specification is ambiguous. \ Only one of `git` or `registry` is allowed.", - name + name_in_toml ), (_, _, Some(_), Some(_)) => bail!( "dependency ({}) specification is ambiguous. \ Only one of `registry` or `registry-index` is allowed.", - name + name_in_toml ), (Some(git), maybe_path, _, _) => { if maybe_path.is_some() { @@ -1303,7 +1303,7 @@ impl DetailedTomlDependency { "dependency ({}) specification is ambiguous. \ Only one of `git` or `path` is allowed. \ This will be considered an error in future versions", - name + name_in_toml ); cx.warnings.push(msg) } @@ -1318,7 +1318,7 @@ impl DetailedTomlDependency { "dependency ({}) specification is ambiguous. \ Only one of `branch`, `tag` or `rev` is allowed. \ This will be considered an error in future versions", - name + name_in_toml ); cx.warnings.push(msg) } @@ -1359,15 +1359,15 @@ impl DetailedTomlDependency { (None, None, None, None) => SourceId::crates_io(cx.config)?, }; - let (pkg_name, rename) = match self.package { - Some(ref s) => (&s[..], Some(name)), - None => (name, None), + let (pkg_name, explicit_name_in_toml) = match self.package { + Some(ref s) => (&s[..], Some(name_in_toml)), + None => (name_in_toml, None), }; let version = self.version.as_ref().map(|v| &v[..]); let mut dep = match cx.pkgid { Some(id) => Dependency::parse(pkg_name, version, &new_source_id, id, cx.config)?, - None => Dependency::parse_no_deprecated(name, version, &new_source_id)?, + None => Dependency::parse_no_deprecated(pkg_name, version, &new_source_id)?, }; dep.set_features(self.features.iter().flat_map(|x| x)) .set_default_features( @@ -1381,9 +1381,9 @@ impl DetailedTomlDependency { if let Some(kind) = kind { dep.set_kind(kind); } - if let Some(rename) = rename { + if let Some(name_in_toml) = explicit_name_in_toml { cx.features.require(Feature::rename_dependency())?; - dep.set_rename(rename); + dep.set_explicit_name_in_toml(name_in_toml); } Ok(dep) } diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs index c3267c706fc..6de2f30533a 100644 --- a/src/crates-io/lib.rs +++ b/src/crates-io/lib.rs @@ -68,7 +68,10 @@ pub struct NewCrateDependency { pub version_req: String, pub target: Option, pub kind: String, - #[serde(skip_serializing_if = "Option::is_none")] pub registry: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub registry: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub explicit_name_in_toml: Option, } #[derive(Deserialize)] diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index c449198e528..00939ea4f19 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -6,7 +6,7 @@ use cargo::util::paths::remove_dir_all; use support::cargo_process; use support::git; use support::paths::{self, CargoPathExt}; -use support::registry::{self, Package}; +use support::registry::{self, Package, Dependency}; use support::{basic_manifest, project}; use url::Url; @@ -1711,3 +1711,57 @@ fn git_init_templatedir_missing() { p.cargo("build").run(); p.cargo("build").run(); } + +#[test] +fn rename_deps_and_features() { + Package::new("foo", "0.1.0") + .file("src/lib.rs", "pub fn f1() {}") + .publish(); + Package::new("foo", "0.2.0") + .file("src/lib.rs", "pub fn f2() {}") + .publish(); + Package::new("bar", "0.2.0") + .add_dep(Dependency::new("foo01", "0.1.0").package("foo").optional(true)) + .add_dep(Dependency::new("foo02", "0.2.0").package("foo")) + .feature("another", &["foo01"]) + .file( + "src/lib.rs", + r#" + extern crate foo02; + #[cfg(feature = "foo01")] + extern crate foo01; + + pub fn foo() { + foo02::f2(); + #[cfg(feature = "foo01")] + foo01::f1(); + } + "#, + ) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "a" + version = "0.5.0" + authors = [] + + [dependencies] + bar = "0.2" + "#, + ).file( + "src/main.rs", + " + extern crate bar; + fn main() { bar::foo(); } + ", + ) + .build(); + + p.cargo("build").run(); + p.cargo("build --features bar/foo01").run(); + p.cargo("build --features bar/another").run(); +} diff --git a/tests/testsuite/support/registry.rs b/tests/testsuite/support/registry.rs index 9ae7212d4b7..9fda183c907 100644 --- a/tests/testsuite/support/registry.rs +++ b/tests/testsuite/support/registry.rs @@ -123,13 +123,16 @@ pub struct Package { alternative: bool, } -struct Dependency { +#[derive(Clone)] +pub struct Dependency { name: String, vers: String, kind: String, target: Option, features: Vec, registry: Option, + package: Option, + optional: bool, } pub fn init() { @@ -250,7 +253,7 @@ impl Package { /// foo = {version = "1.0"} /// ``` pub fn dep(&mut self, name: &str, vers: &str) -> &mut Package { - self.full_dep(name, vers, None, "normal", &[], None) + self.add_dep(&Dependency::new(name, vers)) } /// Add a dependency with the given feature. Example: @@ -259,7 +262,7 @@ impl Package { /// foo = {version = "1.0", "features": ["feat1", "feat2"]} /// ``` pub fn feature_dep(&mut self, name: &str, vers: &str, features: &[&str]) -> &mut Package { - self.full_dep(name, vers, None, "normal", features, None) + self.add_dep(Dependency::new(name, vers).enable_features(features)) } /// Add a platform-specific dependency. Example: @@ -268,13 +271,13 @@ impl Package { /// foo = {version = "1.0"} /// ``` pub fn target_dep(&mut self, name: &str, vers: &str, target: &str) -> &mut Package { - self.full_dep(name, vers, Some(target), "normal", &[], None) + self.add_dep(Dependency::new(name, vers).target(target)) } /// Add a dependency to an alternative registry. /// The given registry should be a URI to the alternative registry. pub fn registry_dep(&mut self, name: &str, vers: &str, registry: &str) -> &mut Package { - self.full_dep(name, vers, None, "normal", &[], Some(registry)) + self.add_dep(Dependency::new(name, vers).registry(registry)) } /// Add a dev-dependency. Example: @@ -283,7 +286,7 @@ impl Package { /// foo = {version = "1.0"} /// ``` pub fn dev_dep(&mut self, name: &str, vers: &str) -> &mut Package { - self.full_dep(name, vers, None, "dev", &[], None) + self.add_dep(Dependency::new(name, vers).dev()) } /// Add a build-dependency. Example: @@ -292,26 +295,11 @@ impl Package { /// foo = {version = "1.0"} /// ``` pub fn build_dep(&mut self, name: &str, vers: &str) -> &mut Package { - self.full_dep(name, vers, None, "build", &[], None) + self.add_dep(Dependency::new(name, vers).build()) } - fn full_dep( - &mut self, - name: &str, - vers: &str, - target: Option<&str>, - kind: &str, - features: &[&str], - registry: Option<&str>, - ) -> &mut Package { - self.deps.push(Dependency { - name: name.to_string(), - vers: vers.to_string(), - kind: kind.to_string(), - target: target.map(|s| s.to_string()), - features: features.iter().map(|s| s.to_string()).collect(), - registry: registry.map(|s| s.to_string()), - }); + pub fn add_dep(&mut self, dep: &Dependency) -> &mut Package { + self.deps.push(dep.clone()); self } @@ -321,6 +309,13 @@ impl Package { self } + /// Add an entry in the `[features]` section + pub fn feature(&mut self, name: &str, deps: &[&str]) -> &mut Package { + let deps = deps.iter().map(|s| s.to_string()).collect(); + self.features.insert(name.to_string(), deps); + self + } + /// Create the package and place it in the registry. /// /// This does not actually use Cargo's publishing system, but instead @@ -336,15 +331,16 @@ impl Package { .iter() .map(|dep| { json!({ - "name": dep.name, - "req": dep.vers, - "features": dep.features, - "default_features": true, - "target": dep.target, - "optional": false, - "kind": dep.kind, - "registry": dep.registry, - }) + "name": dep.name, + "req": dep.vers, + "features": dep.features, + "default_features": true, + "target": dep.target, + "optional": dep.optional, + "kind": dep.kind, + "registry": dep.registry, + "package": dep.package, + }) }).collect::>(); let cksum = { let mut c = Vec::new(); @@ -492,3 +488,60 @@ pub fn cksum(s: &[u8]) -> String { sha.update(s); hex::encode(&sha.finish()) } + +impl Dependency { + pub fn new(name: &str, vers: &str) -> Dependency { + Dependency { + name: name.to_string(), + vers: vers.to_string(), + kind: "normal".to_string(), + target: None, + features: Vec::new(), + package: None, + optional: false, + registry: None, + } + } + + /// Change this to `[build-dependencies]` + pub fn build(&mut self) -> &mut Self { + self.kind = "build".to_string(); + self + } + + /// Change this to `[dev-dependencies]` + pub fn dev(&mut self) -> &mut Self { + self.kind = "dev".to_string(); + self + } + + /// Change this to `[target.$target.dependencies]` + pub fn target(&mut self, target: &str) -> &mut Self { + self.target = Some(target.to_string()); + self + } + + /// Add `registry = $registry` to this dependency + pub fn registry(&mut self, registry: &str) -> &mut Self { + self.registry = Some(registry.to_string()); + self + } + + /// Add `features = [ ... ]` to this dependency + pub fn enable_features(&mut self, features: &[&str]) -> &mut Self { + self.features.extend(features.iter().map(|s| s.to_string())); + self + } + + /// Add `package = ...` to this dependency + pub fn package(&mut self, pkg: &str) -> &mut Self { + self.package = Some(pkg.to_string()); + self + } + + /// Change this to an optional dependency + pub fn optional(&mut self, optional: bool) -> &mut Self { + self.optional = optional; + self + } +}