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.

Sign up for GitHub

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

Jump to bottom

Introduce Requirements struct to clarify code #4683

Merged
merged 8 commits into from
Oct 31, 2017
Merged

Introduce Requirements struct to clarify code #4683

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e87f151
Move resolution requirement state into a separate struct
djc Oct 2, 2017
e1303ec
Extract method for requiring crate features
djc Oct 2, 2017
63281c7
Extract function to record if a feature was seen before
djc Oct 2, 2017
c3c387d
Extract method for recording a requested dependency
djc Oct 2, 2017
1b01e36
Extract method for handling feature requirements
djc Oct 2, 2017
8278876
Move shared logic into separated methods
djc Oct 2, 2017
8890568
Move logic for walking over dependent features around
djc Oct 4, 2017
abf4122
Use specific methods to add requirements
djc Oct 4, 2017
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
181 changes: 102 additions & 79 deletions src/cargo/core/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,55 +859,68 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool {
a.patch == b.patch
}

// Returns a pair of (feature dependencies, all used features)
//
// The feature dependencies map is a mapping of package name to list of features
// enabled. Each package should be enabled, and each package should have the
// specified set of features enabled. The boolean indicates whether this
// package was specifically requested (rather than just requesting features
// *within* this package).
//
// The all used features set is the set of features which this local package had
// enabled, which is later used when compiling to instruct the code what
// features were enabled.
fn build_features<'a>(s: &'a Summary, method: &'a Method)
-> CargoResult<(HashMap<&'a str, (bool, Vec<String>)>, HashSet<&'a str>)> {
let mut deps = HashMap::new();
let mut used = HashSet::new();
let mut visited = HashSet::new();
match *method {
Method::Everything => {
for key in s.features().keys() {
add_feature(s, key, &mut deps, &mut used, &mut visited)?;
}
for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
add_feature(s, dep.name(), &mut deps, &mut used,
&mut visited)?;
}
struct Requirements<'a> {
summary: &'a Summary,
// The deps map is a mapping of package name to list of features enabled.
// Each package should be enabled, and each package should have the
// specified set of features enabled. The boolean indicates whether this
// package was specifically requested (rather than just requesting features
// *within* this package).
deps: HashMap<&'a str, (bool, Vec<String>)>,
// The used features set is the set of features which this local package had
// enabled, which is later used when compiling to instruct the code what
// features were enabled.
used: HashSet<&'a str>,
visited: HashSet<&'a str>,
}

impl<'r> Requirements<'r> {
fn new<'a>(summary: &'a Summary) -> Requirements<'a> {
Requirements {
summary,
deps: HashMap::new(),
used: HashSet::new(),
visited: HashSet::new(),
}
Method::Required { features: requested_features, .. } => {
for feat in requested_features.iter() {
add_feature(s, feat, &mut deps, &mut used, &mut visited)?;
}
}

fn require_crate_feature(&mut self, package: &'r str, feat: &'r str) {
self.used.insert(package);
Copy link
Member

Choose a reason for hiding this comment

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

I'd naively expect this to call require_dependency and then fill in the deps map list, but that also seems like the kind of thing that may break tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still a little fuzzy on what used means exactly, and how it's different from the deps boolean being true. This is different at least in that it leaves the deps boolean at false, which seems meaningful because setting a "forwarded" feature does not mean that we should actually pull that dependency in.

Copy link
Member

Choose a reason for hiding this comment

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

I think it may be related to #4364? I think that changed a few things in this area

self.deps.entry(package)
.or_insert((false, Vec::new()))
.1.push(feat.to_string());
}

fn seen(&mut self, feat: &'r str) -> bool {
if self.visited.insert(feat) {
self.used.insert(feat);
false
} else {
true
}
}
match *method {
Method::Everything |
Method::Required { uses_default_features: true, .. } => {
if s.features().get("default").is_some() {
add_feature(s, "default", &mut deps, &mut used,
&mut visited)?;

fn require_dependency(&mut self, pkg: &'r str) {
if self.seen(pkg) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

I think this early return may cause the boolean in the deps entry to be false when it should unconditionally be true if this function is called, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's also what happens in the old version, right? I tried to keep it functionally the same; reviewing it now, this seems to be the behavior of the old code, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also, wouldn't you say that the boolean in deps must already be true if pkg is self.seen() here?)

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok I think I see that now, makes sense! I find this code always pretty hard to follow...

I completely forget at this point why these all exist as they are, this has changed a fair bit since first written.

}
self.deps.entry(pkg).or_insert((false, Vec::new())).0 = true;
}

fn require_feature(&mut self, feat: &'r str) -> CargoResult<()> {
if self.seen(feat) {
return Ok(());
}
for f in self.summary.features().get(feat).expect("must be a valid feature") {
if f == &feat {
bail!("Cyclic feature dependency: feature `{}` depends on itself", feat);
}
self.add_feature(f)?;
}
Method::Required { uses_default_features: false, .. } => {}
Ok(())
}
return Ok((deps, used));

fn add_feature<'a>(s: &'a Summary,
feat: &'a str,
deps: &mut HashMap<&'a str, (bool, Vec<String>)>,
used: &mut HashSet<&'a str>,
visited: &mut HashSet<&'a str>) -> CargoResult<()> {
fn add_feature(&mut self, feat: &'r str) -> CargoResult<()> {
if feat.is_empty() { return Ok(()) }

// If this feature is of the form `foo/bar`, then we just lookup package
Expand All @@ -919,44 +932,53 @@ fn build_features<'a>(s: &'a Summary, method: &'a Method)
let feat_or_package = parts.next().unwrap();
match parts.next() {
Some(feat) => {
let package = feat_or_package;
used.insert(package);
deps.entry(package)
.or_insert((false, Vec::new()))
.1.push(feat.to_string());
self.require_crate_feature(feat_or_package, feat);
}
None => {
let feat = feat_or_package;

//if this feature has already been added, then just return Ok
if !visited.insert(feat) {
return Ok(());
}

used.insert(feat);
match s.features().get(feat) {
Some(recursive) => {
// This is a feature, add it recursively.
for f in recursive {
if f == feat {
bail!("Cyclic feature dependency: feature `{}` depends \
on itself", feat);
}

add_feature(s, f, deps, used, visited)?;
}
}
None => {
// This is a dependency, mark it as explicitly requested.
deps.entry(feat).or_insert((false, Vec::new())).0 = true;
}
if self.summary.features().contains_key(feat_or_package) {
self.require_feature(feat_or_package)?;
} else {
self.require_dependency(feat_or_package);
}
}
}
Ok(())
}
}

// Takes requested features for a single package from the input Method and
// recurses to find all requested features, dependencies and requested
// dependency features in a Requirements object, returning it to the resolver.
fn build_requirements<'a, 'b: 'a>(s: &'a Summary, method: &'b Method)
-> CargoResult<Requirements<'a>> {
let mut reqs = Requirements::new(s);
match *method {
Method::Everything => {
for key in s.features().keys() {
reqs.require_feature(key)?;
}
for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
reqs.require_dependency(dep.name());
}
}
Method::Required { features: requested_features, .. } => {
for feat in requested_features.iter() {
reqs.add_feature(feat)?;
}
}
}
match *method {
Method::Everything |
Method::Required { uses_default_features: true, .. } => {
if s.features().get("default").is_some() {
reqs.require_feature("default")?;
}
}
Method::Required { uses_default_features: false, .. } => {}
}
return Ok(reqs);
}

impl<'a> Context<'a> {
// Activate this summary by inserting it into our list of known activations.
//
Expand Down Expand Up @@ -1117,18 +1139,18 @@ impl<'a> Context<'a> {
let deps = s.dependencies();
let deps = deps.iter().filter(|d| d.is_transitive() || dev_deps);

let (mut feature_deps, used_features) = build_features(s, method)?;
let mut reqs = build_requirements(s, method)?;
let mut ret = Vec::new();

// Next, collect all actually enabled dependencies and their features.
for dep in deps {
// Skip optional dependencies, but not those enabled through a feature
if dep.is_optional() && !feature_deps.contains_key(dep.name()) {
if dep.is_optional() && !reqs.deps.contains_key(dep.name()) {
continue
}
// So we want this dependency. Move the features we want from `feature_deps`
// to `ret`.
let base = feature_deps.remove(dep.name()).unwrap_or((false, vec![]));
let base = reqs.deps.remove(dep.name()).unwrap_or((false, vec![]));
if !dep.is_optional() && base.0 {
self.warnings.push(
format!("Package `{}` does not have feature `{}`. It has a required dependency \
Expand All @@ -1151,21 +1173,22 @@ impl<'a> Context<'a> {
// Any remaining entries in feature_deps are bugs in that the package does not actually
// have those dependencies. We classified them as dependencies in the first place
// because there is no such feature, either.
if !feature_deps.is_empty() {
let unknown = feature_deps.keys().map(|s| &s[..])
.collect::<Vec<&str>>();
if !reqs.deps.is_empty() {
let unknown = reqs.deps.keys()
.map(|s| &s[..])
.collect::<Vec<&str>>();
let features = unknown.join(", ");
bail!("Package `{}` does not have these features: `{}`",
s.package_id(), features)
}

// Record what list of features is active for this package.
if !used_features.is_empty() {
if !reqs.used.is_empty() {
let pkgid = s.package_id();

let set = self.resolve_features.entry(pkgid.clone())
.or_insert_with(HashSet::new);
for feature in used_features {
for feature in reqs.used {
if !set.contains(feature) {
set.insert(feature.to_string());
}
Expand Down