Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use listed dependency name for feature names #5811

Merged
merged 1 commit into from
Jul 31, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {

let crate_name = dep.target.crate_name();
let mut names = deps.iter()
.map(|d| d.rename().unwrap_or(&crate_name));
.map(|d| d.rename().map(|s| s.as_str()).unwrap_or(&crate_name));
let name = names.next().unwrap_or(&crate_name);
for n in names {
if n == name {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/context/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ fn compute_deps<'a, 'cfg>(

// If the dependency is optional, then we're only activating it
// if the corresponding feature was activated
if dep.is_optional() && !bcx.resolve.features(id).contains(&*dep.name()) {
if dep.is_optional() && !bcx.resolve.features(id).contains(&*dep.name_in_toml()) {
return false;
}

Expand Down
64 changes: 55 additions & 9 deletions src/cargo/core/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ struct Inner {
specified_req: bool,
kind: Kind,
only_match_name: bool,
rename: Option<String>,
rename: Option<InternedString>,

optional: bool,
default_features: bool,
Expand Down Expand Up @@ -66,15 +66,15 @@ impl ser::Serialize for Dependency {
{
let string_features: Vec<_> = self.features().iter().map(|s| s.to_string()).collect();
SerializedDependency {
name: &*self.name(),
name: &*self.package_name(),
source: self.source_id(),
req: self.version_req().to_string(),
kind: self.kind(),
optional: self.is_optional(),
uses_default_features: self.uses_default_features(),
features: &string_features,
target: self.platform(),
rename: self.rename(),
rename: self.rename().map(|s| s.as_str()),
}.serialize(s)
}
}
Expand Down Expand Up @@ -209,7 +209,49 @@ impl Dependency {
&self.inner.req
}

pub fn name(&self) -> InternedString {
/// This is the name of this `Dependency` as listed in `Cargo.toml`.
///
/// Or in other words, this is what shows up in the `[dependencies]` section
/// on the left hand side. This is **not** the name of the package that's
/// being depended on as the dependency can be renamed. For that use
/// `package_name` below.
///
/// Both of the dependencies below return `foo` for `name_in_toml`:
///
/// ```toml
/// [dependencies]
/// foo = "0.1"
/// ```
///
/// and ...
///
/// ```toml
/// [dependencies]
/// foo = { version = "0.1", package = 'bar' }
/// ```
pub fn name_in_toml(&self) -> InternedString {
self.rename().unwrap_or(self.inner.name)
}

/// The name of the package that this `Dependency` depends on.
///
/// Usually this is what's written on the left hand side of a dependencies
/// section, but it can also be renamed via the `package` key.
///
/// Both of the dependencies below return `foo` for `package_name`:
///
/// ```toml
/// [dependencies]
/// foo = "0.1"
/// ```
///
/// and ...
///
/// ```toml
/// [dependencies]
/// bar = { version = "0.1", package = 'foo' }
/// ```
pub fn package_name(&self) -> InternedString {
self.inner.name
}

Expand Down Expand Up @@ -240,8 +282,12 @@ impl Dependency {
self.inner.platform.as_ref()
}

pub fn rename(&self) -> Option<&str> {
self.inner.rename.as_ref().map(|s| &**s)
/// The renamed name of this dependency, if any.
///
/// If the `package` key is used in `Cargo.toml` then this returns the same
/// value as `name_in_toml`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to stall this PR, but after several attempts I still can't wrap my head around this explanation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry about that! I'll try to get around to cleaning this up soon.

pub fn rename(&self) -> Option<InternedString> {
self.inner.rename
}

pub fn set_kind(&mut self, kind: Kind) -> &mut Dependency {
Expand Down Expand Up @@ -286,7 +332,7 @@ impl Dependency {
}

pub fn set_rename(&mut self, rename: &str) -> &mut Dependency {
Rc::make_mut(&mut self.inner).rename = Some(rename.to_string());
Rc::make_mut(&mut self.inner).rename = Some(InternedString::new(rename));
self
}

Expand All @@ -296,7 +342,7 @@ impl Dependency {
assert!(self.inner.req.matches(id.version()));
trace!(
"locking dep from `{}` with `{}` at {} to {}",
self.name(),
self.package_name(),
self.version_req(),
self.source_id(),
id
Expand Down Expand Up @@ -347,7 +393,7 @@ impl Dependency {

/// Returns true if the package (`sum`) can fulfill this dependency request.
pub fn matches_ignoring_source(&self, id: &PackageId) -> bool {
self.name() == id.name() && self.version_req().matches(id.version())
self.package_name() == id.name() && self.version_req().matches(id.version())
}

/// Returns true if the package (`id`) can fulfill this dependency request.
Expand Down
28 changes: 14 additions & 14 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ impl<'cfg> PackageRegistry<'cfg> {
// of summaries which should be the same length as `deps` above.
let unlocked_summaries = deps.iter()
.map(|dep| {
debug!("registring a patch for `{}` with `{}`", url, dep.name());
debug!("registring a patch for `{}` with `{}`", url, dep.package_name());

// Go straight to the source for resolving `dep`. Load it as we
// normally would and then ask it directly for the list of summaries
Expand All @@ -207,7 +207,7 @@ impl<'cfg> PackageRegistry<'cfg> {
format_err!(
"failed to load source for a dependency \
on `{}`",
dep.name()
dep.package_name()
)
})?;

Expand All @@ -223,22 +223,22 @@ impl<'cfg> PackageRegistry<'cfg> {
"patch for `{}` in `{}` did not resolve to any crates. If this is \
unexpected, you may wish to consult: \
https://github.com/rust-lang/cargo/issues/4678",
dep.name(),
dep.package_name(),
url
),
};
if summaries.next().is_some() {
bail!(
"patch for `{}` in `{}` resolved to more than one candidate",
dep.name(),
dep.package_name(),
url
)
}
if summary.package_id().source_id().url() == url {
bail!(
"patch for `{}` in `{}` points to the same source, but \
patches must point to different sources",
dep.name(),
dep.package_name(),
url
);
}
Expand Down Expand Up @@ -306,7 +306,7 @@ impl<'cfg> PackageRegistry<'cfg> {
fn query_overrides(&mut self, dep: &Dependency) -> CargoResult<Option<Summary>> {
for s in self.overrides.iter() {
let src = self.sources.get_mut(s).unwrap();
let dep = Dependency::new_override(&*dep.name(), s);
let dep = Dependency::new_override(&*dep.package_name(), s);
let mut results = src.query_vec(&dep)?;
if !results.is_empty() {
return Ok(Some(results.remove(0)));
Expand Down Expand Up @@ -369,21 +369,21 @@ http://doc.crates.io/specifying-dependencies.html#overriding-dependencies
modified to not match the previously resolved version\n\n\
{}",
override_summary.package_id().name(),
dep.name(),
dep.package_name(),
boilerplate
);
self.source_config.config().shell().warn(&msg)?;
return Ok(());
}

if let Some(id) = real_deps.get(0) {
if let Some(dep) = real_deps.get(0) {
let msg = format!(
"\
path override for crate `{}` has altered the original list of
dependencies; the dependency on `{}` was removed\n\n
{}",
override_summary.package_id().name(),
id.name(),
dep.package_name(),
boilerplate
);
self.source_config.config().shell().warn(&msg)?;
Expand Down Expand Up @@ -439,7 +439,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
with `{}`, \
looking at sources",
patches.len(),
dep.name(),
dep.package_name(),
dep.source_id(),
dep.version_req()
);
Expand All @@ -451,7 +451,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
format_err!(
"failed to load source for a dependency \
on `{}`",
dep.name()
dep.package_name()
)
})?;

Expand Down Expand Up @@ -542,7 +542,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
None => summary,
};
summary.map_dependencies(|dep| {
trace!("\t{}/{}/{}", dep.name(), dep.version_req(), dep.source_id());
trace!("\t{}/{}/{}", dep.package_name(), dep.version_req(), dep.source_id());

// If we've got a known set of overrides for this summary, then
// one of a few cases can arise:
Expand Down Expand Up @@ -578,7 +578,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
// If anything does then we lock it to that and move on.
let v = locked
.get(dep.source_id())
.and_then(|map| map.get(&*dep.name()))
.and_then(|map| map.get(&*dep.package_name()))
.and_then(|vec| vec.iter().find(|&&(ref id, _)| dep.matches_id(id)));
if let Some(&(ref id, _)) = v {
trace!("\tsecond hit on {}", id);
Expand All @@ -592,7 +592,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
let v = patches.get(dep.source_id().url()).map(|vec| {
let dep2 = dep.clone();
let mut iter = vec.iter().filter(move |p| {
dep2.name() == p.name() && dep2.version_req().matches(p.version())
dep2.matches_ignoring_source(p)
});
(iter.next(), iter)
});
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/resolver/conflict_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ impl ConflictCache {
.entry(dep.clone())
.or_insert_with(Vec::new);
if !past.contains(con) {
trace!("{} adding a skip {:?}", dep.name(), con);
trace!("{} adding a skip {:?}", dep.package_name(), con);
past.push(con.clone());
for c in con.keys() {
self.dep_from_pid
Expand Down
22 changes: 11 additions & 11 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ impl Context {

pub fn prev_active(&self, dep: &Dependency) -> &[Summary] {
self.activations
.get(&(dep.name(), dep.source_id().clone()))
.get(&(dep.package_name(), dep.source_id().clone()))
.map(|v| &v[..])
.unwrap_or(&[])
}
Expand Down Expand Up @@ -178,27 +178,27 @@ impl Context {
for dep in deps {
// Skip optional dependencies, but not those enabled through a
// feature
if dep.is_optional() && !reqs.deps.contains_key(&dep.name()) {
if dep.is_optional() && !reqs.deps.contains_key(&dep.name_in_toml()) {
continue;
}
// So we want this dependency. Move the features we want from
// `feature_deps` to `ret` and register ourselves as using this
// name.
let base = reqs.deps.get(&dep.name()).unwrap_or(&default_dep);
used_features.insert(dep.name());
let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep);
used_features.insert(dep.name_in_toml());
let always_required = !dep.is_optional()
&& !s
.dependencies()
.iter()
.any(|d| d.is_optional() && d.name() == dep.name());
.any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
if always_required && base.0 {
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()
dep.name_in_toml()
));
}
let mut base = base.1.clone();
Expand All @@ -220,8 +220,8 @@ impl Context {
let remaining = reqs
.deps
.keys()
.filter(|&s| !used_features.contains(s))
.map(|s| s.as_str())
.cloned()
.filter(|s| !used_features.contains(s))
.collect::<Vec<_>>();
if !remaining.is_empty() {
let features = remaining.join(", ");
Expand Down Expand Up @@ -298,10 +298,10 @@ fn build_requirements<'a, 'b: 'a>(
all_features: true, ..
} => {
for key in s.features().keys() {
reqs.require_feature(InternedString::new(&key))?;
reqs.require_feature(*key)?;
}
for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
reqs.require_dependency(dep.name());
reqs.require_dependency(dep.name_in_toml());
}
}
Method::Required {
Expand Down Expand Up @@ -389,7 +389,7 @@ impl<'r> Requirements<'r> {
for fv in self
.summary
.features()
.get(&feat)
.get(feat.as_str())
.expect("must be a valid feature")
{
match *fv {
Expand Down
Loading