From f76cc8a2746f946afbb4582efc90a9298a1d8f18 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 24 May 2018 16:41:38 -0700 Subject: [PATCH 01/13] Rename Variants to Params, and replace subjects and variants with Params in Node implementations. --- src/rust/engine/src/core.rs | 50 ++++++------ src/rust/engine/src/nodes.rs | 134 ++++++++----------------------- src/rust/engine/src/scheduler.rs | 6 +- 3 files changed, 64 insertions(+), 126 deletions(-) diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index b03571a4505..908d8ebb37f 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -3,7 +3,6 @@ use fnv::FnvHasher; -use std::collections::HashMap; use std::ops::Deref; use std::sync::Arc; use std::{fmt, hash}; @@ -14,36 +13,39 @@ use handles::Handle; pub type FNV = hash::BuildHasherDefault; /// -/// Variants represent a string->string map. For hashability purposes, they're stored -/// as sorted string tuples. +/// Params represent a TypeId->Key map. +/// +/// For efficiency and hashability, they're stored as sorted Keys (with distinct TypeIds), and +/// wrapped in an `Arc` that allows us to copy-on-write for param contents. /// #[repr(C)] #[derive(Clone, Debug, Default, Eq, Hash, PartialEq)] -pub struct Variants(pub Vec<(String, String)>); +pub struct Params(Arc>); + +impl Params { + pub fn new(mut params: Vec) -> Params { + params.sort_by(|l, r| l.type_id().cmp(r.type_id())); + params.dedup_by(|l, r| l.type_id() == r.type_id()); + Params(Arc::new(params)) + } -impl Variants { - /// - /// Merges right over self (by key, and then sorted by key). /// - /// TODO: Unused: see https://github.com/pantsbuild/pants/issues/4020 + /// TODO: This is a compatibility API to assist in the transition from "every Node has exactly + /// one Subject" to "every Node has zero or more Params". See: + /// https://github.com/pantsbuild/pants/issues/5788 /// - #[allow(dead_code)] - pub fn merge(&self, right: Variants) -> Variants { - // Merge. - let mut left: HashMap<_, _, FNV> = self.0.iter().cloned().collect(); - left.extend(right.0); - // Convert back to a vector and sort. - let mut result: Vec<(String, String)> = left.into_iter().collect(); - result.sort(); - Variants(result) + pub fn expect_single(&self) -> &Key { + if self.0.len() != 1 { + panic!( + "Expect Params to contain exactly one value... contained: {:?}", + self.0 + ); + } + &self.0[0] } - pub fn find(&self, key: &str) -> Option<&str> { - self - .0 - .iter() - .find(|&&(ref k, _)| k == key) - .map(|&(_, ref v)| v.as_str()) + pub fn find(&self, type_id: TypeId) -> Option<&Key> { + self.0.iter().find(|k| k.type_id() == &type_id) } } @@ -52,7 +54,7 @@ pub type Id = u64; // The type of a python object (which itself has a type, but which is not represented // by a Key, because that would result in a infinitely recursive structure.) #[repr(C)] -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] pub struct TypeId(pub Id); // On the python side, the 0th type id is used as an anonymous id diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 22877d411dc..29db866a39b 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -10,7 +10,7 @@ use futures::future::{self, Future}; use boxfuture::{BoxFuture, Boxable}; use context::{Context, Core}; -use core::{throw, Failure, Key, Noop, TypeConstraint, Value, Variants}; +use core::{throw, Failure, Key, Noop, Params, TypeConstraint, Value}; use externs; use fs::{ self, Dir, DirectoryListing, File, FileContent, GlobExpansionConjunction, GlobMatching, Link, @@ -89,68 +89,57 @@ pub trait WrappedNode: Into { } /// -/// A Node that selects a product for a subject. +/// A Node that selects a product for some Params. /// /// A Select can be satisfied by multiple sources, but fails if multiple sources produce a value. -/// The 'variants' field represents variant configuration that is propagated to dependencies. When -/// a task needs to consume a product as configured by the variants map, it can pass variant_key, -/// which matches a 'variant' value to restrict the names of values selected by a SelectNode. +/// The 'params' represent a series of type-keyed parameters that will be used by Nodes in the +/// subgraph below this Select. /// #[derive(Clone, Debug, Eq, Hash, PartialEq)] pub struct Select { - pub subject: Key, - pub variants: Variants, + pub params: Params, pub selector: selectors::Select, entries: rule_graph::Entries, } impl Select { - pub fn new( - product: TypeConstraint, - subject: Key, - variants: Variants, - edges: &rule_graph::RuleEdges, - ) -> Select { + pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select { let selector = selectors::Select::without_variant(product); let select_key = rule_graph::SelectKey::JustSelect(selector.clone()); Select { selector: selector, - subject: subject, - variants: variants, + params: params, entries: edges.entries_for(&select_key), } } pub fn new_with_entries( product: TypeConstraint, - subject: Key, - variants: Variants, + params: Params, entries: rule_graph::Entries, ) -> Select { let selector = selectors::Select::without_variant(product); Select { selector: selector, - subject: subject, - variants: variants, + params: params, entries: entries, } } pub fn new_with_selector( selector: selectors::Select, - subject: Key, - variants: Variants, + params: Params, edges: &rule_graph::RuleEdges, ) -> Select { + let subject_type_id = params.expect_single().type_id().clone(); let select_key = rule_graph::SelectKey::JustSelect(selector.clone()); Select { selector: selector, - subject: subject, - variants: variants, + params: params, entries: edges .entries_for(&select_key) .into_iter() - .filter(|e| e.matches_subject_type(subject.type_id().clone())) + .filter(|e| e.matches_subject_type(subject_type_id)) .collect(), } } @@ -159,45 +148,22 @@ impl Select { &self.selector.product } - fn select_literal_single<'a>( - &self, - candidate: &'a Value, - variant_value: &Option, - ) -> bool { - if !externs::satisfied_by(&self.selector.product, candidate) { - return false; - } - match variant_value { - &Some(ref vv) if externs::project_str(candidate, "name") != *vv => - // There is a variant value, and it doesn't match. - false, - _ => - true, - } - } - /// /// Looks for has-a or is-a relationships between the given value and the requested product. /// /// Returns the resulting product value, or None if no match was made. /// - fn select_literal( - &self, - context: &Context, - candidate: Value, - variant_value: &Option, - ) -> Option { + fn select_literal(&self, context: &Context, candidate: Value) -> Option { // Check whether the subject is-a instance of the product. - if self.select_literal_single(&candidate, variant_value) { + if externs::satisfied_by(&self.selector.product, &candidate) { return Some(candidate); } // Else, check whether it has-a instance of the product. - // TODO: returning only the first literal configuration of a given type/variant. Need to - // define mergeability for products. + // TODO: returning only the first item of a particular type. if externs::satisfied_by(&context.core.types.has_products, &candidate) { for child in externs::project_multi(&candidate, "products") { - if self.select_literal_single(&child, variant_value) { + if externs::satisfied_by(&self.selector.product, &child) { return Some(child); } } @@ -212,14 +178,13 @@ impl Select { &self, context: &Context, results: Vec>, - variant_value: &Option, ) -> Result { let mut matches = Vec::new(); let mut max_noop = Noop::NoTask; for result in results { match result { Ok(value) => { - if let Some(v) = self.select_literal(&context, value, variant_value) { + if let Some(v) = self.select_literal(&context, value) { matches.push(v); } } @@ -268,12 +233,8 @@ impl Select { .expect("Expected edges to exist for Snapshot intrinsic."); // Compute PathGlobs for the subject. let context = context.clone(); - Select::new( - context.core.types.path_globs, - self.subject, - self.variants.clone(), - &edges, - ).run(context.clone()) + Select::new(context.core.types.path_globs, self.params.clone(), &edges) + .run(context.clone()) .and_then(move |path_globs_val| context.get(Snapshot(externs::key_for(path_globs_val)))) .to_boxed() } @@ -292,8 +253,7 @@ impl Select { let context = context.clone(); Select::new( context.core.types.process_request, - self.subject, - self.variants.clone(), + self.params.clone(), edges, ).run(context.clone()) .and_then(|process_request_val| { @@ -306,7 +266,7 @@ impl Select { /// /// Return Futures for each Task/Node that might be able to compute the given product for the - /// given subject and variants. + /// given Params. /// fn gen_nodes(&self, context: &Context) -> Vec> { if let Some(&(_, ref value)) = context.core.tasks.gen_singleton(self.product()) { @@ -319,9 +279,8 @@ impl Select { .map( |entry| match context.core.rule_graph.rule_for_inner(entry) { &rule_graph::Rule::Task(ref task) => context.get(Task { - subject: self.subject, + params: self.params.clone(), product: *self.product(), - variants: self.variants.clone(), task: task.clone(), entry: Arc::new(entry.clone()), }), @@ -347,8 +306,7 @@ impl Select { let context = context.clone(); Select::new( context.core.types.directory_digest, - self.subject, - self.variants.clone(), + self.params.clone(), edges, ).run(context.clone()) .and_then(|directory_digest_val| { @@ -407,25 +365,11 @@ impl WrappedNode for Select { type Item = Value; fn run(self, context: Context) -> NodeFuture { - // TODO add back support for variants https://github.com/pantsbuild/pants/issues/4020 - - // If there is a variant_key, see whether it has been configured; if not, no match. - let variant_value: Option = match self.selector.variant_key { - Some(ref variant_key) => { - let variant_value = self.variants.find(variant_key); - if variant_value.is_none() { - return err(Failure::Noop(Noop::NoVariant)); - } - variant_value.map(|v| v.to_string()) - } - None => None, - }; - // If the Subject "is a" or "has a" Product, then we're done. - if let Some(literal_value) = - self.select_literal(&context, externs::val_for(&self.subject), &variant_value) + if let Some(value) = + self.select_literal(&context, externs::val_for(self.params.expect_single())) { - return ok(literal_value); + return ok(value); } // Else, attempt to use the configured tasks to compute the value. @@ -440,11 +384,8 @@ impl WrappedNode for Select { .collect::>(), ); - let variant_value = variant_value.map(|s| s.to_string()); deps_future - .and_then(move |dep_results| { - future::result(self.choose_task_result(&context, dep_results, &variant_value)) - }) + .and_then(move |dep_results| self.choose_task_result(&context, dep_results)) .to_boxed() } } @@ -790,9 +731,8 @@ impl From for NodeKey { #[derive(Clone, Debug, Eq, Hash, PartialEq)] pub struct Task { - subject: Key, + params: Params, product: TypeConstraint, - variants: Variants, task: tasks::Task, entry: Arc, } @@ -805,8 +745,7 @@ impl Task { ) -> NodeFuture> { let get_futures = gets .into_iter() - .map(|get| { - let externs::Get(product, subject) = get; + .map(|externs::Get(product, subject)| { let entries = context .core .rule_graph @@ -816,7 +755,7 @@ impl Task { product: product, subject: *subject.type_id(), })); - Select::new_with_entries(product, subject, Variants::default(), entries) + Select::new_with_entries(product, Params::new(vec![subject]), entries) .run(context.clone()) .map_err(was_required) }) @@ -861,16 +800,13 @@ impl WrappedNode for Task { .rule_graph .edges_for_inner(&self.entry) .expect("edges for task exist."); - let subject = self.subject; - let variants = self.variants; + let params = self.params; future::join_all( self .task .clause .into_iter() - .map(|s| { - Select::new_with_selector(s, subject, variants.clone(), edges).run(context.clone()) - }) + .map(|s| Select::new_with_selector(s, params.clone(), edges).run(context.clone())) .collect::>(), ) }; @@ -1046,13 +982,13 @@ impl Node for NodeKey { &NodeKey::Scandir(ref s) => format!("Scandir({:?})", s.0), &NodeKey::Select(ref s) => format!( "Select({}, {})", - keystr(&s.subject), + keystr(&s.params.expect_single()), typstr(&s.selector.product) ), &NodeKey::Task(ref s) => format!( "Task({}, {}, {})", externs::project_str(&externs::val_for(&s.task.func.0), "__name__"), - keystr(&s.subject), + keystr(&s.params.expect_single()), typstr(&s.product) ), &NodeKey::Snapshot(ref s) => format!("Snapshot({})", keystr(&s.0)), diff --git a/src/rust/engine/src/scheduler.rs b/src/rust/engine/src/scheduler.rs index ca0827ac8a0..e84c8d0c998 100644 --- a/src/rust/engine/src/scheduler.rs +++ b/src/rust/engine/src/scheduler.rs @@ -11,7 +11,7 @@ use futures::sync::oneshot; use boxfuture::{BoxFuture, Boxable}; use context::{Context, Core}; -use core::{Failure, Key, TypeConstraint, TypeId, Value, Variants}; +use core::{Failure, Key, Params, TypeConstraint, TypeId, Value}; use fs::{self, GlobMatching, PosixFS}; use graph::{EntryId, Graph, Node, NodeContext}; use nodes::{NodeKey, Select, Tracer, TryInto, Visualizer}; @@ -111,7 +111,7 @@ impl Scheduler { )?; request .roots - .push(Select::new(product, subject, Variants::default(), &edges)); + .push(Select::new(product, Params::new(vec![subject]), &edges)); Ok(()) } @@ -258,7 +258,7 @@ impl Scheduler { .roots .iter() .zip(results.into_iter()) - .map(|(s, r)| (&s.subject, &s.selector.product, r)) + .map(|(s, r)| (s.params.expect_single(), &s.selector.product, r)) .collect() } From 532f82dd40db646ca9fbef91c32b6fa3df147488 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 24 May 2018 18:31:56 -0700 Subject: [PATCH 02/13] Rename SubjectIsProduct to `Param` in the RuleGraph, and replace single-subject fields with multi-Param fields. --- src/rust/engine/src/core.rs | 9 +- src/rust/engine/src/lib.rs | 2 +- src/rust/engine/src/nodes.rs | 29 +- src/rust/engine/src/rule_graph.rs | 292 +++++++++++-------- src/rust/engine/src/tasks.rs | 8 + tests/python/pants_test/engine/test_rules.py | 41 ++- 6 files changed, 217 insertions(+), 164 deletions(-) diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index 908d8ebb37f..4c7d7fc4f65 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -44,8 +44,15 @@ impl Params { &self.0[0] } + /// + /// Returns the given TypeId if it is represented in this set of Params. + /// pub fn find(&self, type_id: TypeId) -> Option<&Key> { - self.0.iter().find(|k| k.type_id() == &type_id) + self + .0 + .binary_search_by(|probe| probe.type_id().cmp(&type_id)) + .ok() + .map(|idx| &self.0[idx]) } } diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index fb22c978470..136b455d484 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -752,7 +752,7 @@ fn graph_sub( product_type: TypeConstraint, ) -> RuleGraph { let graph_maker = GraphMaker::new(&scheduler.core.tasks, vec![subject_type]); - graph_maker.sub_graph(subject_type, &product_type) + graph_maker.sub_graph(&subject_type, &product_type) } fn write_to_file(path: &Path, graph: &RuleGraph) -> io::Result<()> { diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 29db866a39b..34a48c5b858 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -106,10 +106,11 @@ impl Select { pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select { let selector = selectors::Select::without_variant(product); let select_key = rule_graph::SelectKey::JustSelect(selector.clone()); + let entries = edges.entries_for(&select_key, ¶ms); Select { selector: selector, params: params, - entries: edges.entries_for(&select_key), + entries: entries, } } @@ -131,16 +132,14 @@ impl Select { params: Params, edges: &rule_graph::RuleEdges, ) -> Select { - let subject_type_id = params.expect_single().type_id().clone(); let select_key = rule_graph::SelectKey::JustSelect(selector.clone()); + // TODO: We should know statically which entries are valid, right? It's not clear why this + // method needs to filter... can maybe remove SelectKey. + let entries = edges.entries_for(&select_key, ¶ms); Select { selector: selector, params: params, - entries: edges - .entries_for(&select_key) - .into_iter() - .filter(|e| e.matches_subject_type(subject_type_id)) - .collect(), + entries: entries, } } @@ -746,16 +745,22 @@ impl Task { let get_futures = gets .into_iter() .map(|externs::Get(product, subject)| { + // TODO: The subject of the get is a new parameter, but params from the context should be + // included as well. + let params = Params::new(vec![subject]); let entries = context .core .rule_graph .edges_for_inner(entry) .expect("edges for task exist.") - .entries_for(&rule_graph::SelectKey::JustGet(selectors::Get { - product: product, - subject: *subject.type_id(), - })); - Select::new_with_entries(product, Params::new(vec![subject]), entries) + .entries_for( + &rule_graph::SelectKey::JustGet(selectors::Get { + product: product, + subject: *subject.type_id(), + }), + ¶ms, + ); + Select::new_with_entries(product, params, entries) .run(context.clone()) .map_err(was_required) }) diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index 46ae9c86def..a4e6315b37d 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -1,14 +1,16 @@ // Copyright 2017 Pants project contributors (see CONTRIBUTORS.md). // Licensed under the Apache License, Version 2.0 (see LICENSE). -use std::collections::{hash_map, HashMap, HashSet}; +use std::collections::{hash_map, BTreeSet, HashMap, HashSet}; use std::io; -use core::{Function, Key, TypeConstraint, TypeId, Value, ANY_TYPE}; +use core::{Function, Key, Params, TypeConstraint, TypeId, Value, ANY_TYPE}; use externs; use selectors::{Get, Select}; use tasks::{Intrinsic, Task, Tasks}; +type ParamTypes = BTreeSet; + #[derive(Eq, Hash, PartialEq, Clone, Debug)] pub struct UnreachableError { task_rule: Task, @@ -20,7 +22,7 @@ impl UnreachableError { UnreachableError { task_rule: task_rule, diagnostic: Diagnostic { - subject_type: ANY_TYPE, + params: Default::default(), reason: "Unreachable".to_string(), }, } @@ -34,10 +36,10 @@ pub enum EntryWithDeps { } impl EntryWithDeps { - fn subject_type(&self) -> TypeId { + fn params(&self) -> &ParamTypes { match self { - &EntryWithDeps::Root(ref re) => re.subject_type, - &EntryWithDeps::Inner(ref ie) => ie.subject_type, + &EntryWithDeps::Root(ref re) => &re.params, + &EntryWithDeps::Inner(ref ie) => &ie.params, } } @@ -83,26 +85,14 @@ impl EntryWithDeps { #[derive(Eq, Hash, PartialEq, Clone, Debug)] pub enum Entry { - SubjectIsProduct { subject_type: TypeId }, - + Param(TypeId), WithDeps(EntryWithDeps), - - Singleton { value: Key, product: TypeConstraint }, -} - -impl Entry { - pub fn matches_subject_type(&self, actual_subject_type: TypeId) -> bool { - match self { - &Entry::SubjectIsProduct { ref subject_type } => *subject_type == actual_subject_type, - &Entry::WithDeps(ref r) => r.subject_type() == actual_subject_type, - &Entry::Singleton { .. } => true, - } - } + Singleton(Key, TypeConstraint), } #[derive(Eq, Hash, PartialEq, Clone, Debug)] pub struct RootEntry { - subject_type: TypeId, + params: ParamTypes, // TODO: A RootEntry can only have one declared `Select`, and no declared `Get`s, but these // are shaped as Vecs to temporarily minimize the re-shuffling in `_construct_graph`. Remove in // a future commit. @@ -120,7 +110,7 @@ pub enum Rule { #[derive(Eq, Hash, PartialEq, Clone, Debug)] pub struct InnerEntry { - subject_type: TypeId, + params: ParamTypes, rule: Rule, } @@ -130,21 +120,6 @@ impl InnerEntry { } } -impl Entry { - fn new_subject_is_product(subject_type: TypeId) -> Entry { - Entry::SubjectIsProduct { - subject_type: subject_type, - } - } - - fn new_singleton(value: Key, product: TypeConstraint) -> Entry { - Entry::Singleton { - value: value, - product: product, - } - } -} - /// /// A key for the Selects used from a rule. Rules are only picked up by Select selectors. These keys /// uniquely identify the selects used by a particular entry in the rule graph so that they can be @@ -165,7 +140,7 @@ type UnfulfillableRuleMap = HashMap; #[derive(Eq, Hash, PartialEq, Clone, Debug)] pub struct Diagnostic { - subject_type: TypeId, + params: ParamTypes, reason: String, } @@ -173,19 +148,23 @@ pub struct Diagnostic { // to be found statically rather than dynamically. pub struct GraphMaker<'t> { tasks: &'t Tasks, - root_subject_types: Vec, + root_param_types: ParamTypes, } impl<'t> GraphMaker<'t> { pub fn new(tasks: &'t Tasks, root_subject_types: Vec) -> GraphMaker<'t> { + let root_param_types = root_subject_types.into_iter().collect(); GraphMaker { - tasks: tasks, - root_subject_types: root_subject_types, + tasks, + root_param_types, } } - pub fn sub_graph(&self, subject_type: TypeId, product_type: &TypeConstraint) -> RuleGraph { - if let Some(beginning_root) = self.gen_root_entry(subject_type, product_type) { + pub fn sub_graph(&self, subject_type: &TypeId, product_type: &TypeConstraint) -> RuleGraph { + // TODO: Update to support rendering a subgraph given a set of ParamTypes. + let param_types = vec![*subject_type].into_iter().collect(); + + if let Some(beginning_root) = self.gen_root_entry(¶m_types, product_type) { self._construct_graph(vec![beginning_root]) } else { RuleGraph::default() @@ -198,11 +177,13 @@ impl<'t> GraphMaker<'t> { pub fn _construct_graph(&self, roots: Vec) -> RuleGraph { let mut dependency_edges: RuleDependencyEdges = HashMap::new(); + let mut entry_equivalences = HashMap::new(); let mut unfulfillable_rules: UnfulfillableRuleMap = HashMap::new(); for beginning_root in roots { self._construct_graph_helper( &mut dependency_edges, + &mut entry_equivalences, &mut unfulfillable_rules, EntryWithDeps::Root(beginning_root), ); @@ -211,7 +192,7 @@ impl<'t> GraphMaker<'t> { let unreachable_rules = self.unreachable_rules(&dependency_edges, &unfulfillable_rules); RuleGraph { - root_subject_types: self.root_subject_types.clone(), + root_param_types: self.root_param_types.clone(), rule_dependency_edges: dependency_edges, unfulfillable_rules: unfulfillable_rules, unreachable_rules: unreachable_rules, @@ -249,57 +230,86 @@ impl<'t> GraphMaker<'t> { } /// - /// Computes (and memoizes) whether any rules can compute the given `product_type` for the given - /// `subject_type`. + /// Computes whether the given candidate entry is satisfiable, and if it is, returns a copy + /// of the entry with its parameters pruned to what is actually used. Once computed, the + /// "equivalence" of the input entry to the output entry is memoized in entry_equivalences. /// - /// When a rule cannot be fulfilled, it is added to `unfulfillable_rules` rather than to - /// `rule_dependency_edges`. + /// When a rule can be fulfilled it will end up stored in both the rule_dependency_edges and + /// the equivalances. If it can't be fulfilled, it is added to `unfulfillable_rules`. /// fn _construct_graph_helper( &self, rule_dependency_edges: &mut RuleDependencyEdges, + entry_equivalences: &mut HashMap, unfulfillable_rules: &mut UnfulfillableRuleMap, entry: EntryWithDeps, - ) -> bool { - // If the entry has not been visited before, store a placeholder in the unfulfillable rules map - // and then visit its children. Otherwise, we're done. + ) -> Option { + if let Some(equivalent) = entry_equivalences.get(&entry) { + // A simplified equivalent entry has already been computed, return it. + return Some(equivalent.clone()); + } else if let Some(_) = unfulfillable_rules.get(&entry) { + // The rule is unfulfillable. + return None; + } + + // Otherwise, store a placeholder in the rule_dependency_edges map and then visit its + // children. // // This prevents infinite recursion by shortcircuiting when an entry recursively depends on // itself. It's totally fine for rules to be recursive: the recursive path just never // contributes to whether the rule is satisfiable. - match (unfulfillable_rules.entry(entry.clone()), rule_dependency_edges.entry(entry.clone())) { - (hash_map::Entry::Vacant(_), hash_map::Entry::Vacant(re)) => { + match rule_dependency_edges.entry(entry.clone()) { + hash_map::Entry::Vacant(re) => { // When a rule has not been visited before, we visit it by storing a placeholder in the // rule dependencies map (to prevent infinite recursion). re.insert(RuleEdges::default()); }, - (hash_map::Entry::Vacant(_), hash_map::Entry::Occupied(_)) => - // Rule has been visited before and been found to be valid, or is currently being - // recursively visited and has a placeholder. - return true, - (hash_map::Entry::Occupied(_), _) => - // Rule has either been visited before and found unfulfillable. - return false, + hash_map::Entry::Occupied(o) => + // We're currently recursively under this rule, but its simplified equivalence has not yet + // been computed. This entry will be rewritten with its equivalency when the parent + // completes. + return Some(o.key().clone()), }; // For each dependency of the rule, recurse for each potential match and collect RuleEdges. let mut edges = RuleEdges::new(); let mut fulfillable = true; + let mut used_params = BTreeSet::new(); for select_key in entry.dependency_keys() { - let (subject, product) = match &select_key { - &SelectKey::JustSelect(ref s) => (entry.subject_type(), s.product), - &SelectKey::JustGet(ref g) => (g.subject, g.product), + let (params, product) = match &select_key { + &SelectKey::JustSelect(ref s) => (entry.params().clone(), s.product.clone()), + &SelectKey::JustGet(ref g) => { + let get_params = { + let mut p = entry.params().clone(); + p.insert(g.subject.clone()); + p + }; + (get_params, g.product.clone()) + } }; // Confirm that at least one candidate is fulfillable. - let fulfillable_candidates = rhs(&self.tasks, subject, &product) + let fulfillable_candidates = rhs(&self.tasks, ¶ms, &product) .into_iter() - .filter(|candidate| match candidate { - &Entry::WithDeps(ref c) => { - self._construct_graph_helper(rule_dependency_edges, unfulfillable_rules, c.clone()) + .filter_map(|candidate| match candidate { + Entry::WithDeps(c) => { + if let Some(equivalence) = self._construct_graph_helper( + rule_dependency_edges, + entry_equivalences, + unfulfillable_rules, + c, + ) { + used_params.extend(equivalence.params().iter().cloned()); + Some(Entry::WithDeps(equivalence)) + } else { + None + } } - &Entry::SubjectIsProduct { .. } => true, - &Entry::Singleton { .. } => true, + Entry::Param(type_id) => { + used_params.insert(type_id); + Some(Entry::Param(type_id)) + } + s @ Entry::Singleton { .. } => Some(s), }) .collect::>(); @@ -309,11 +319,12 @@ impl<'t> GraphMaker<'t> { .entry(entry.clone()) .or_insert_with(Vec::new) .push(Diagnostic { - subject_type: subject, + params: params.clone(), reason: format!( - "no rule was available to compute {} for subject type {}", - type_constraint_str(product), - type_str(subject) + "no rule was available to compute {} with parameter type{} {}", + type_constraint_str(product.clone()), + if params.len() > 1 { "s" } else { "" }, + params_str(¶ms), ), }); fulfillable = false; @@ -324,40 +335,39 @@ impl<'t> GraphMaker<'t> { } if fulfillable { - // All depedendencies were fulfillable: replace the placeholder with the computed RuleEdges. - rule_dependency_edges.insert(entry, edges); - true + // All dependencies were fulfillable: store the equivalence, and replace the placeholder. + // TODO: Compute used parameters above and store them here. + // TODO2: We also need to rewrite all edges with the new equivalence, because in cases where + // we recursed on ourself, nodes will have dependencies on us. + entry_equivalences.insert(entry.clone(), entry.clone()); + rule_dependency_edges.insert(entry.clone(), edges); + Some(entry) } else { // Was not fulfillable. Remove the placeholder: the unfulfillable entries we stored will // prevent us from attempting to expand this node again. rule_dependency_edges.remove(&entry); - false + None } } fn gen_root_entries(&self, product_types: &HashSet) -> Vec { - let mut result: Vec = Vec::new(); - for subj_type in &self.root_subject_types { - for pt in product_types { - if let Some(entry) = self.gen_root_entry(*subj_type, pt) { - result.push(entry); - } - } - } - result + product_types + .iter() + .filter_map(|product_type| self.gen_root_entry(&self.root_param_types, product_type)) + .collect() } fn gen_root_entry( &self, - subject_type: TypeId, + param_types: &ParamTypes, product_type: &TypeConstraint, ) -> Option { - let candidates = rhs(&self.tasks, subject_type, product_type); + let candidates = rhs(&self.tasks, param_types, product_type); if candidates.is_empty() { None } else { Some(RootEntry { - subject_type: subject_type, + params: param_types.clone(), clause: vec![Select { product: *product_type, variant_key: None, @@ -376,7 +386,7 @@ impl<'t> GraphMaker<'t> { /// the subject of the graph. /// /// -/// `root_subject_types` the root subject types this graph was generated with. +/// `root_param_types` the root parameter types that this graph was generated with. /// `root_dependencies` A map from root rules, ie rules representing the expected selector / subject /// types for requests, to the rules that can fulfill them. /// `rule_dependency_edges` A map from rule entries to the rule entries they depend on. @@ -385,7 +395,7 @@ impl<'t> GraphMaker<'t> { /// containing the reasons why they were eliminated from the graph. #[derive(Debug, Default)] pub struct RuleGraph { - root_subject_types: Vec, + root_param_types: ParamTypes, rule_dependency_edges: RuleDependencyEdges, unfulfillable_rules: UnfulfillableRuleMap, unreachable_rules: Vec, @@ -419,6 +429,14 @@ pub fn type_str(type_id: TypeId) -> String { } } +pub fn params_str(params: &ParamTypes) -> String { + params + .iter() + .map(|type_id| type_str(type_id.clone())) + .collect::>() + .join("+") +} + fn val_name(val: &Value) -> String { externs::project_str(val, "__name__") } @@ -438,12 +456,10 @@ fn get_str(get: &Get) -> String { fn entry_str(entry: &Entry) -> String { match entry { &Entry::WithDeps(ref e) => entry_with_deps_str(e), - &Entry::SubjectIsProduct { subject_type } => { - format!("SubjectIsProduct({})", type_str(subject_type)) - } - &Entry::Singleton { ref value, product } => format!( + &Entry::Param(type_id) => format!("Param({})", type_str(type_id)), + &Entry::Singleton(value, product) => format!( "Singleton({}, {})", - externs::key_to_str(value), + externs::key_to_str(&value), type_constraint_str(product) ), } @@ -453,17 +469,17 @@ fn entry_with_deps_str(entry: &EntryWithDeps) -> String { match entry { &EntryWithDeps::Inner(InnerEntry { rule: Rule::Task(ref task_rule), - subject_type, - }) => format!("{} of {}", task_display(task_rule), type_str(subject_type)), + ref params, + }) => format!("{} of {}", task_display(task_rule), params_str(params)), &EntryWithDeps::Inner(InnerEntry { rule: Rule::Intrinsic(ref intrinsic), - subject_type, + ref params, }) => format!( "({}, ({},), {:?}) for {}", type_constraint_str(intrinsic.product), type_constraint_str(intrinsic.input), intrinsic.kind, - type_str(subject_type) + params_str(params) ), &EntryWithDeps::Root(ref root) => format!( "{} for {}", @@ -473,7 +489,7 @@ fn entry_with_deps_str(entry: &EntryWithDeps) -> String { .map(|s| select_str(s)) .collect::>() .join(", "), - type_str(root.subject_type) + params_str(&root.params) ), } } @@ -515,9 +531,9 @@ impl RuleGraph { } pub fn find_root_edges(&self, subject_type: TypeId, select: Select) -> Option { - // TODO return Result instead + // TODO: Support more than one root parameter... needs some API work. let root = RootEntry { - subject_type: subject_type, + params: vec![subject_type].into_iter().collect(), clause: vec![select], gets: vec![], }; @@ -620,7 +636,7 @@ impl RuleGraph { } let mut root_subject_type_strs = self - .root_subject_types + .root_param_types .iter() .map(|&t| type_str(t)) .collect::>(); @@ -694,11 +710,24 @@ impl RuleEdges { } } - pub fn entries_for(&self, select_key: &SelectKey) -> Entries { + pub fn entries_for(&self, select_key: &SelectKey, param_values: &Params) -> Entries { self .dependencies_by_select_key .get(select_key) - .cloned() + .map(|entries| { + entries + .into_iter() + .filter(|&entry| match entry { + &Entry::WithDeps(EntryWithDeps::Root(RootEntry { ref params, .. })) + | &Entry::WithDeps(EntryWithDeps::Inner(InnerEntry { ref params, .. })) => params + .iter() + .all(|type_id| param_values.find(*type_id).is_some()), + &Entry::Param(type_id) => param_values.find(type_id).is_some(), + &Entry::Singleton { .. } => true, + }) + .cloned() + .collect() + }) .unwrap_or_else(Vec::new) } @@ -722,28 +751,33 @@ impl RuleEdges { } } -fn rhs(tasks: &Tasks, subject_type: TypeId, product_type: &TypeConstraint) -> Entries { - if externs::satisfied_by_type(product_type, subject_type) { - // NB a matching subject is always picked first - vec![Entry::new_subject_is_product(subject_type)] - } else if let Some(&(ref key, _)) = tasks.gen_singleton(product_type) { - vec![Entry::new_singleton(*key, *product_type)] - } else { - let mut entries = Vec::new(); - if let Some(matching_intrinsic) = tasks.gen_intrinsic(product_type) { - entries.push(Entry::WithDeps(EntryWithDeps::Inner(InnerEntry { - subject_type: subject_type, - rule: Rule::Intrinsic(*matching_intrinsic), - }))); - } - if let Some(matching_tasks) = tasks.gen_tasks(product_type) { - entries.extend(matching_tasks.iter().map(|task_rule| { - Entry::WithDeps(EntryWithDeps::Inner(InnerEntry { - subject_type: subject_type, - rule: Rule::Task(task_rule.clone()), - })) - })); - } - entries +fn rhs(tasks: &Tasks, params: &ParamTypes, product_type: &TypeConstraint) -> Entries { + if let Some(&(ref key, _)) = tasks.gen_singleton(product_type) { + return vec![Entry::Singleton(*key, *product_type)]; + } + + let mut entries = Vec::new(); + if let Some(type_id) = params + .iter() + .find(|&&type_id| externs::satisfied_by_type(product_type, type_id)) + { + // TODO: We only match the first param type here that satisfies the constraint although it's + // possible that multiple parameters could. Would be nice to be able to remove TypeConstraint. + entries.push(Entry::Param(*type_id)); + } + if let Some(matching_intrinsic) = tasks.gen_intrinsic(product_type) { + entries.push(Entry::WithDeps(EntryWithDeps::Inner(InnerEntry { + params: params.clone(), + rule: Rule::Intrinsic(*matching_intrinsic), + }))); + } + if let Some(matching_tasks) = tasks.gen_tasks(product_type) { + entries.extend(matching_tasks.iter().map(|task_rule| { + Entry::WithDeps(EntryWithDeps::Inner(InnerEntry { + params: params.clone(), + rule: Rule::Task(task_rule.clone()), + })) + })); } + entries } diff --git a/src/rust/engine/src/tasks.rs b/src/rust/engine/src/tasks.rs index 282ae373dde..3ee1e374c63 100644 --- a/src/rust/engine/src/tasks.rs +++ b/src/rust/engine/src/tasks.rs @@ -67,6 +67,14 @@ impl Tasks { self.tasks.values().flat_map(|tasks| tasks).collect() } + pub fn singleton_types(&self) -> Vec { + self + .singletons + .values() + .map(|&(k, _)| *k.type_id()) + .collect() + } + pub fn gen_singleton(&self, product: &TypeConstraint) -> Option<&(Key, Value)> { self.singletons.get(product) } diff --git a/tests/python/pants_test/engine/test_rules.py b/tests/python/pants_test/engine/test_rules.py index e2809d84ef4..026dde5420f 100644 --- a/tests/python/pants_test/engine/test_rules.py +++ b/tests/python/pants_test/engine/test_rules.py @@ -82,7 +82,7 @@ def test_ruleset_with_missing_product_type(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 1 (A, (Select(B),), noop): - no rule was available to compute B for subject type SubA + no rule was available to compute B with parameter type SubA """).strip(), str(cm.exception)) @@ -94,8 +94,8 @@ def test_ruleset_with_rule_with_two_missing_selects(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 1 (A, (Select(B), Select(C)), noop): - no rule was available to compute B for subject type SubA - no rule was available to compute C for subject type SubA + no rule was available to compute B with parameter type SubA + no rule was available to compute C with parameter type SubA """).strip(), str(cm.exception)) @@ -115,9 +115,9 @@ def test_ruleset_with_superclass_of_selected_type_produced_fails(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 2 (A, (Select(B),), noop): - no rule was available to compute B for subject type C + no rule was available to compute B with parameter type C (B, (Select(SubA),), noop): - no rule was available to compute SubA for subject type C + no rule was available to compute SubA with parameter type C """).strip(), str(cm.exception)) @@ -142,7 +142,7 @@ def test_ruleset_with_failure_due_to_incompatible_subject_for_singleton(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 1 (D, (Select(C),), noop): - no rule was available to compute C for subject type A + no rule was available to compute C with parameter type A """).strip(), str(cm.exception)) @@ -161,9 +161,9 @@ def test_not_fulfillable_duplicated_dependency(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 2 (B, (Select(D),), noop): - no rule was available to compute D for subject type SubA + no rule was available to compute D with parameter type SubA (D, (Select(A), Select(SubA)), [Get(A, C)], noop): - no rule was available to compute A for subject type C + no rule was available to compute A with parameter type C """).strip(), str(cm.exception)) @@ -188,7 +188,7 @@ def test_smallest_full_test(self): "Select(A) for SubA" [color=blue] "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"SubjectIsProduct(SubA)"} + "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} }""").strip(), fullgraph) def test_full_graph_for_planner_example(self): @@ -237,7 +237,7 @@ def test_smallest_full_test_multiple_root_subject_types(self): // root subject types: A, SubA // root entries "Select(A) for A" [color=blue] - "Select(A) for A" -> {"SubjectIsProduct(A)"} + "Select(A) for A" -> {"Param(A)"} "Select(A) for SubA" [color=blue] "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} "Select(B) for A" [color=blue] @@ -245,8 +245,8 @@ def test_smallest_full_test_multiple_root_subject_types(self): "Select(B) for SubA" [color=blue] "Select(B) for SubA" -> {"(B, (Select(A),), noop) of SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"SubjectIsProduct(SubA)"} - "(B, (Select(A),), noop) of A" -> {"SubjectIsProduct(A)"} + "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} + "(B, (Select(A),), noop) of A" -> {"Param(A)"} "(B, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} }""").strip(), fullgraph) @@ -265,7 +265,7 @@ def test_single_rule_depending_on_subject_selection(self): "Select(A) for SubA" [color=blue] "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"SubjectIsProduct(SubA)"} + "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} }""").strip(), subgraph) @@ -284,7 +284,7 @@ def test_multiple_selects(self): "Select(A) for SubA" [color=blue] "Select(A) for SubA" -> {"(A, (Select(SubA), Select(B)), noop) of SubA"} // internal entries - "(A, (Select(SubA), Select(B)), noop) of SubA" -> {"SubjectIsProduct(SubA)" "(B, (,), noop) of SubA"} + "(A, (Select(SubA), Select(B)), noop) of SubA" -> {"Param(SubA)" "(B, (,), noop) of SubA"} "(B, (,), noop) of SubA" -> {} }""").strip(), subgraph) @@ -305,7 +305,7 @@ def test_one_level_of_recursion(self): "Select(A) for SubA" -> {"(A, (Select(B),), noop) of SubA"} // internal entries "(A, (Select(B),), noop) of SubA" -> {"(B, (Select(SubA),), noop) of SubA"} - "(B, (Select(SubA),), noop) of SubA" -> {"SubjectIsProduct(SubA)"} + "(B, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} }""").strip(), subgraph) @@ -365,7 +365,7 @@ def test_root_tuple_removed_when_no_matches(self): "Select(A) for C" [color=blue] "Select(A) for C" -> {"(A, (Select(C),), noop) of C"} // internal entries - "(A, (Select(C),), noop) of C" -> {"SubjectIsProduct(C)"} + "(A, (Select(C),), noop) of C" -> {"Param(C)"} }""").strip(), fullgraph) @@ -398,7 +398,6 @@ def test_get_with_matching_singleton(self): subgraph = self.create_subgraph(A, rules, SubA()) - #TODO perhaps singletons should be marked in the dot format somehow self.assert_equal_with_printing(dedent(""" digraph { // root subject types: SubA @@ -406,7 +405,7 @@ def test_get_with_matching_singleton(self): "Select(A) for SubA" [color=blue] "Select(A) for SubA" -> {"(A, (Select(SubA),), [Get(B, C)], noop) of SubA"} // internal entries - "(A, (Select(SubA),), [Get(B, C)], noop) of SubA" -> {"SubjectIsProduct(SubA)" "Singleton(B(), B)"} + "(A, (Select(SubA),), [Get(B, C)], noop) of SubA" -> {"Singleton(B(), B)" "Param(SubA)"} }""").strip(), subgraph) @@ -426,7 +425,7 @@ def test_depends_on_multiple_one_noop(self): "Select(B) for SubA" [color=blue] "Select(B) for SubA" -> {"(B, (Select(A),), noop) of SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"SubjectIsProduct(SubA)"} + "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} "(B, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} }""").strip(), subgraph) @@ -451,7 +450,7 @@ def test_multiple_depend_on_same_rule(self): "Select(C) for SubA" [color=blue] "Select(C) for SubA" -> {"(C, (Select(A),), noop) of SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"SubjectIsProduct(SubA)"} + "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} "(B, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} "(C, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} }""").strip(), @@ -473,7 +472,7 @@ def test_get_simple(self): "Select(A) for SubA" -> {"(A, (,), [Get(B, D)], noop) of SubA"} // internal entries "(A, (,), [Get(B, D)], noop) of SubA" -> {"(B, (Select(D),), noop) of D"} - "(B, (Select(D),), noop) of D" -> {"SubjectIsProduct(D)"} + "(B, (Select(D),), noop) of D" -> {"Param(D)"} }""").strip(), subgraph) From 5538b37de6a319638f8f6c189b52f71106903f3b Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Sun, 15 Jul 2018 18:04:13 -0700 Subject: [PATCH 03/13] Fix rendering of an empty RuleGraph. --- src/rust/engine/src/rule_graph.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index a4e6315b37d..a214eaa7e14 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -629,12 +629,6 @@ impl RuleGraph { } pub fn visualize(&self, f: &mut io::Write) -> io::Result<()> { - if self.rule_dependency_edges.is_empty() { - writeln!(f, "digraph {{")?; - writeln!(f, " // empty graph")?; - writeln!(f, "}}")?; - } - let mut root_subject_type_strs = self .root_param_types .iter() From f0ae015f32d8524879bf5eeaa08f04ffef75e8c0 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Sun, 15 Jul 2018 12:39:14 -0700 Subject: [PATCH 04/13] Compute used parameters to monomorphize the graph so that there is exactly one candidate per RuleEdge(s), and remove Noop. --- src/python/pants/engine/rules.py | 11 - src/python/pants/engine/scheduler.py | 6 +- src/rust/engine/Cargo.lock | 3 +- src/rust/engine/Cargo.toml | 3 +- src/rust/engine/src/core.rs | 23 +- src/rust/engine/src/lib.rs | 8 +- src/rust/engine/src/nodes.rs | 203 +++----- src/rust/engine/src/rule_graph.rs | 660 +++++++++++++++++++++------ src/rust/engine/src/tasks.rs | 6 +- 9 files changed, 601 insertions(+), 322 deletions(-) diff --git a/src/python/pants/engine/rules.py b/src/python/pants/engine/rules.py index c5fd274ca86..e6a42643055 100644 --- a/src/python/pants/engine/rules.py +++ b/src/python/pants/engine/rules.py @@ -131,10 +131,6 @@ class Rule(AbstractClass): def output_constraint(self): """An output Constraint type for the rule.""" - @abstractproperty - def input_selectors(self): - """Collection of input selectors.""" - class TaskRule(datatype(['output_constraint', 'input_selectors', 'input_gets', 'func']), Rule): """A Rule that runs a task function when all of its input selectors are satisfied. @@ -191,10 +187,6 @@ def __new__(cls, output_type, value): # Create. return super(SingletonRule, cls).__new__(cls, constraint, value) - @property - def input_selectors(self): - return tuple() - def __repr__(self): return '{}({}, {})'.format(type(self).__name__, type_or_constraint_repr(self.output_constraint), self.value) @@ -207,9 +199,6 @@ class RootRule(datatype(['output_constraint']), Rule): of an execution. """ - def input_selectors(self): - return [] - class RuleIndex(datatype(['rules', 'roots'])): """Holds an index of Tasks and Singletons used to instantiate Nodes.""" diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index 30f8952bd9c..5028333b746 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -230,8 +230,8 @@ def _register_singleton(self, output_constraint, rule): def _register_task(self, output_constraint, rule): """Register the given TaskRule with the native scheduler.""" - func = rule.func - self._native.lib.tasks_task_begin(self._tasks, Function(self._to_key(func)), output_constraint) + func = Function(self._to_key(rule.func)) + self._native.lib.tasks_task_begin(self._tasks, func, output_constraint) for selector in rule.input_selectors: selector_type = type(selector) product_constraint = self._to_constraint(selector.product) @@ -325,7 +325,7 @@ def _run_and_return_roots(self, session, execution_request): for raw_root in self._native.unpack(raw_roots.nodes_ptr, raw_roots.nodes_len): if raw_root.state_tag is 1: state = Return(self._from_value(raw_root.state_value)) - elif raw_root.state_tag in (2, 3, 4): + elif raw_root.state_tag in (2, 3): state = Throw(self._from_value(raw_root.state_value)) else: raise ValueError( diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index aa90019539c..207251bc17e 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -257,7 +257,8 @@ dependencies = [ "futures 0.1.23 (registry+https://github.com/rust-lang/crates.io-index)", "graph 0.0.1", "hashing 0.0.1", - "lazy_static 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "itertools 0.7.8 (registry+https://github.com/rust-lang/crates.io-index)", + "lazy_static 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", "process_execution 0.0.1", "resettable 0.0.1", diff --git a/src/rust/engine/Cargo.toml b/src/rust/engine/Cargo.toml index 190a6a0611f..2aa72e9daf4 100644 --- a/src/rust/engine/Cargo.toml +++ b/src/rust/engine/Cargo.toml @@ -77,7 +77,8 @@ fs = { path = "fs" } futures = "^0.1.16" graph = { path = "graph" } hashing = { path = "hashing" } -lazy_static = "1" +itertools = "0.7.2" +lazy_static = "0.2.2" log = "0.4" process_execution = { path = "process_execution" } resettable = { path = "resettable" } diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index 4c7d7fc4f65..da4d0c1dd79 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -165,37 +165,16 @@ pub enum Failure { /// A Node failed because a filesystem change invalidated it or its inputs. /// A root requestor should usually immediately retry their request. Invalidated, - /// There was no valid combination of rules to satisfy a request. - Noop(Noop), /// A rule raised an exception. Throw(Value, String), } -// NB: enum members are listed in ascending priority order based on how likely they are -// to be useful to users. -#[derive(Clone, Copy, Eq, PartialEq, Ord, PartialOrd)] -pub enum Noop { - NoTask, - NoVariant, - Cycle, -} - -impl fmt::Debug for Noop { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(match self { - &Noop::Cycle => "Dep graph contained a cycle.", - &Noop::NoTask => "No task was available to compute the value.", - &Noop::NoVariant => "A matching variant key was not configured in variants.", - }) - } -} - pub fn throw(msg: &str) -> Failure { Failure::Throw( externs::create_exception(msg), format!( "Traceback (no traceback):\n \nException: {}", msg - ).to_string(), + ), ) } diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 136b455d484..47cc1abee84 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -44,6 +44,7 @@ extern crate fs; extern crate futures; extern crate graph; extern crate hashing; +extern crate itertools; #[macro_use] extern crate lazy_static; #[macro_use] @@ -82,8 +83,7 @@ use types::Types; enum RawStateTag { Return = 1, Throw = 2, - Noop = 3, - Invalidated = 4, + Invalidated = 3, } #[repr(C)] @@ -100,10 +100,6 @@ impl RawNode { let (state_tag, state_value) = match state { Ok(v) => (RawStateTag::Return as u8, v), Err(Failure::Throw(exc, _)) => (RawStateTag::Throw as u8, exc), - Err(Failure::Noop(noop)) => ( - RawStateTag::Noop as u8, - externs::create_exception(&format!("{:?}", noop)), - ), Err(Failure::Invalidated) => ( RawStateTag::Invalidated as u8, externs::create_exception("Exhausted retries due to changed files."), diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 34a48c5b858..c19f9ad5718 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -10,7 +10,7 @@ use futures::future::{self, Future}; use boxfuture::{BoxFuture, Boxable}; use context::{Context, Core}; -use core::{throw, Failure, Key, Noop, Params, TypeConstraint, Value}; +use core::{throw, Failure, Key, Params, TypeConstraint, Value}; use externs; use fs::{ self, Dir, DirectoryListing, File, FileContent, GlobExpansionConjunction, GlobMatching, Link, @@ -34,17 +34,6 @@ fn err(failure: Failure) -> NodeFuture { future::err(failure).to_boxed() } -/// -/// A helper to indicate that the value represented by the Failure was required, and thus -/// fatal if not present. -/// -fn was_required(failure: Failure) -> Failure { - match failure { - Failure::Noop(noop) => throw(&format!("No source of required dependency: {:?}", noop)), - f => f, - } -} - impl VFS for Context { fn read_link(&self, link: &Link) -> NodeFuture { self.get(ReadLink(link.clone())).map(|res| res.0).to_boxed() @@ -99,31 +88,24 @@ pub trait WrappedNode: Into { pub struct Select { pub params: Params, pub selector: selectors::Select, - entries: rule_graph::Entries, + entry: rule_graph::Entry, } impl Select { pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select { - let selector = selectors::Select::without_variant(product); - let select_key = rule_graph::SelectKey::JustSelect(selector.clone()); - let entries = edges.entries_for(&select_key, ¶ms); - Select { - selector: selector, - params: params, - entries: entries, - } + Self::new_with_selector(selectors::Select::without_variant(product), params, edges) } pub fn new_with_entries( product: TypeConstraint, params: Params, - entries: rule_graph::Entries, + entry: rule_graph::Entry, ) -> Select { let selector = selectors::Select::without_variant(product); Select { - selector: selector, - params: params, - entries: entries, + selector, + params, + entry, } } @@ -133,13 +115,16 @@ impl Select { edges: &rule_graph::RuleEdges, ) -> Select { let select_key = rule_graph::SelectKey::JustSelect(selector.clone()); - // TODO: We should know statically which entries are valid, right? It's not clear why this - // method needs to filter... can maybe remove SelectKey. - let entries = edges.entries_for(&select_key, ¶ms); + // TODO: Is it worth propagating an error here? + // TODO: Need to filter the parameters to what is actually used by this Entry. + let entry = edges + .entry_for(&select_key) + .unwrap_or_else(|| panic!("{:?} did not declare a dependency on {:?}", edges, selector)) + .clone(); Select { - selector: selector, - params: params, - entries: entries, + selector, + params, + entry, } } @@ -150,12 +135,12 @@ impl Select { /// /// Looks for has-a or is-a relationships between the given value and the requested product. /// - /// Returns the resulting product value, or None if no match was made. + /// Returns the resulting product Value for success, or the original Value for failure. /// - fn select_literal(&self, context: &Context, candidate: Value) -> Option { + fn select_literal(&self, context: &Context, candidate: Value) -> Result { // Check whether the subject is-a instance of the product. if externs::satisfied_by(&self.selector.product, &candidate) { - return Some(candidate); + return Ok(candidate); } // Else, check whether it has-a instance of the product. @@ -163,61 +148,11 @@ impl Select { if externs::satisfied_by(&context.core.types.has_products, &candidate) { for child in externs::project_multi(&candidate, "products") { if externs::satisfied_by(&self.selector.product, &child) { - return Some(child); - } - } - } - None - } - - /// - /// Given the results of configured Task nodes, select a single successful value, or fail. - /// - fn choose_task_result( - &self, - context: &Context, - results: Vec>, - ) -> Result { - let mut matches = Vec::new(); - let mut max_noop = Noop::NoTask; - for result in results { - match result { - Ok(value) => { - if let Some(v) = self.select_literal(&context, value) { - matches.push(v); - } - } - Err(err) => { - match err { - Failure::Noop(noop) => { - // Record the highest priority Noop value. - if noop > max_noop { - max_noop = noop; - } - continue; - } - i @ Failure::Invalidated => return Err(i), - f @ Failure::Throw(..) => return Err(f), - } + return Ok(child); } } } - - if matches.len() > 1 { - // TODO: Multiple successful tasks are not currently supported. We could allow for this - // by adding support for "mergeable" products. see: - // https://github.com/pantsbuild/pants/issues/2526 - return Err(throw("Conflicting values produced for subject and type.")); - } - - match matches.pop() { - Some(matched) => - // Exactly one value was available. - Ok(matched), - None => - // Propagate the highest priority Noop value. - Err(Failure::Noop(max_noop)), - } + Err(candidate) } fn snapshot( @@ -264,24 +199,24 @@ impl Select { } /// - /// Return Futures for each Task/Node that might be able to compute the given product for the + /// Return the Future for the Task that should compute the given product for the /// given Params. /// - fn gen_nodes(&self, context: &Context) -> Vec> { + /// TODO: This could take `self` by value and avoid cloning. + /// + fn gen_node(&self, context: &Context) -> NodeFuture { if let Some(&(_, ref value)) = context.core.tasks.gen_singleton(self.product()) { - return vec![future::ok(value.clone()).to_boxed()]; + return future::ok(value.clone()).to_boxed(); } - self - .entries - .iter() - .map( - |entry| match context.core.rule_graph.rule_for_inner(entry) { + match &self.entry { + &rule_graph::Entry::WithDeps(rule_graph::EntryWithDeps::Inner(ref inner)) => { + match inner.rule() { &rule_graph::Rule::Task(ref task) => context.get(Task { params: self.params.clone(), product: *self.product(), task: task.clone(), - entry: Arc::new(entry.clone()), + entry: Arc::new(self.entry.clone()), }), &rule_graph::Rule::Intrinsic(Intrinsic { kind: IntrinsicKind::Snapshot, @@ -289,7 +224,7 @@ impl Select { }) => { let context = context.clone(); self - .snapshot(&context, &entry) + .snapshot(&context, &self.entry) .map(move |snapshot| Snapshot::store_snapshot(&context.core, &snapshot)) .to_boxed() } @@ -300,7 +235,7 @@ impl Select { let edges = &context .core .rule_graph - .edges_for_inner(entry) + .edges_for_inner(&self.entry) .expect("Expected edges to exist for FilesContent intrinsic."); let context = context.clone(); Select::new( @@ -338,7 +273,7 @@ impl Select { }) => { let context = context.clone(); self - .execute_process(&context, &entry) + .execute_process(&context, &self.entry) .map(move |result| { externs::unsafe_call( &context.core.types.construct_process_result, @@ -352,9 +287,16 @@ impl Select { }) .to_boxed() } - }, - ) - .collect::>>() + } + } + &rule_graph::Entry::WithDeps(rule_graph::EntryWithDeps::Root(_)) + | &rule_graph::Entry::Param(_) + | &rule_graph::Entry::Singleton { .. } => { + // TODO: gen_node should be inlined, and should use these Entry types to skip + // any runtime checks of python objects. + panic!("Not a runtime-executable entry! {:?}", self.entry) + } + } } } @@ -365,26 +307,23 @@ impl WrappedNode for Select { fn run(self, context: Context) -> NodeFuture { // If the Subject "is a" or "has a" Product, then we're done. - if let Some(value) = - self.select_literal(&context, externs::val_for(self.params.expect_single())) + if let Ok(value) = self.select_literal(&context, externs::val_for(self.params.expect_single())) { return ok(value); } - // Else, attempt to use the configured tasks to compute the value. - let deps_future = future::join_all( - self - .gen_nodes(&context) - .into_iter() - .map(|node_future| { - // Don't fail the join if one fails. - node_future.then(future::ok) + // Attempt to use the configured Task to compute the value. + self + .gen_node(&context) + .and_then(move |value| { + self.select_literal(&context, value).map_err(|value| { + throw(&format!( + "{} returned a result value that did not satisfy its constraints: {:?}", + rule_graph::entry_str(&self.entry), + value + )) }) - .collect::>(), - ); - - deps_future - .and_then(move |dep_results| self.choose_task_result(&context, dep_results)) + }) .to_boxed() } } @@ -747,22 +686,26 @@ impl Task { .map(|externs::Get(product, subject)| { // TODO: The subject of the get is a new parameter, but params from the context should be // included as well. + // TODO: Parameters should be filtered to what is used by the Entry. let params = Params::new(vec![subject]); - let entries = context + let select_key = rule_graph::SelectKey::JustGet(selectors::Get { + product: product, + subject: *subject.type_id(), + }); + let entry = context .core .rule_graph .edges_for_inner(entry) .expect("edges for task exist.") - .entries_for( - &rule_graph::SelectKey::JustGet(selectors::Get { - product: product, - subject: *subject.type_id(), - }), - ¶ms, - ); - Select::new_with_entries(product, params, entries) - .run(context.clone()) - .map_err(was_required) + .entry_for(&select_key) + .unwrap_or_else(|| { + panic!( + "{:?} did not declare a dependency on {:?}", + entry, select_key + ) + }) + .clone(); + Select::new_with_entries(product, params, entry).run(context.clone()) }) .collect::>(); future::join_all(get_futures).to_boxed() @@ -856,7 +799,7 @@ impl NodeVisualizer for Visualizer { fn color(&mut self, entry: &Entry) -> String { let max_colors = 12; match entry.peek() { - None | Some(Err(Failure::Noop(_))) => "white".to_string(), + None => "white".to_string(), Some(Err(Failure::Throw(..))) => "4".to_string(), Some(Err(Failure::Invalidated)) => "12".to_string(), Some(Ok(_)) => { @@ -877,7 +820,6 @@ impl NodeTracer for Tracer { fn is_bottom(result: Option>) -> bool { match result { Some(Err(Failure::Invalidated)) => false, - Some(Err(Failure::Noop(..))) => true, Some(Err(Failure::Throw(..))) => false, Some(Ok(_)) => true, None => { @@ -902,7 +844,6 @@ impl NodeTracer for Tracer { .collect::>() .join("\n") ), - Some(Err(Failure::Noop(ref x))) => format!("Noop({:?})", x), Some(Err(Failure::Invalidated)) => "Invalidated".to_string(), } } @@ -1018,7 +959,7 @@ impl NodeError for Failure { } fn cyclic() -> Failure { - Failure::Noop(Noop::Cycle) + throw("Dep graph contained a cycle.") } } diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index a214eaa7e14..94ab4a636d6 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -4,7 +4,9 @@ use std::collections::{hash_map, BTreeSet, HashMap, HashSet}; use std::io; -use core::{Function, Key, Params, TypeConstraint, TypeId, Value, ANY_TYPE}; +use itertools::Itertools; + +use core::{Function, Key, TypeConstraint, TypeId, Value, ANY_TYPE}; use externs; use selectors::{Get, Select}; use tasks::{Intrinsic, Task, Tasks}; @@ -24,6 +26,7 @@ impl UnreachableError { diagnostic: Diagnostic { params: Default::default(), reason: "Unreachable".to_string(), + details: vec![], }, } } @@ -38,8 +41,8 @@ pub enum EntryWithDeps { impl EntryWithDeps { fn params(&self) -> &ParamTypes { match self { - &EntryWithDeps::Root(ref re) => &re.params, &EntryWithDeps::Inner(ref ie) => &ie.params, + &EntryWithDeps::Root(ref re) => &re.params, } } @@ -81,6 +84,32 @@ impl EntryWithDeps { }) => vec![SelectKey::JustSelect(Select::without_variant(*input))], } } + + /// + /// Given a set of used parameters (which must be a subset of the parameters available here), + /// return a clone of this entry with its parameter set reduced to the used parameters. + /// + fn simplified(&self, used_params: ParamTypes) -> EntryWithDeps { + let mut simplified = self.clone(); + { + let simplified_params = match &mut simplified { + &mut EntryWithDeps::Inner(ref mut ie) => &mut ie.params, + &mut EntryWithDeps::Root(ref mut re) => &mut re.params, + }; + + let unavailable_params: ParamTypes = + used_params.difference(simplified_params).cloned().collect(); + assert!( + unavailable_params.is_empty(), + "Entry {} used parameters that were not available: {}", + entry_with_deps_str(self), + params_str(&unavailable_params), + ); + + *simplified_params = used_params; + } + simplified + } } #[derive(Eq, Hash, PartialEq, Clone, Debug)] @@ -133,15 +162,53 @@ pub enum SelectKey { JustSelect(Select), } -pub type Entries = Vec; type RuleDependencyEdges = HashMap; -type RuleDiagnostics = Vec; -type UnfulfillableRuleMap = HashMap; +type UnfulfillableRuleMap = HashMap>; #[derive(Eq, Hash, PartialEq, Clone, Debug)] pub struct Diagnostic { params: ParamTypes, reason: String, + details: Vec, +} + +impl Diagnostic { + fn ambiguous(available_params: &ParamTypes, key: &SelectKey, entries: Vec<&Entry>) -> Diagnostic { + let params_clause = match available_params.len() { + 0 => "", + 1 => " with parameter type ", + _ => " with parameter types ", + }; + Diagnostic { + params: available_params.clone(), + reason: format!( + "ambiguous rules for {}{}{}", + select_key_str(&key), + params_clause, + params_str(&available_params), + ), + details: entries.into_iter().map(entry_str).collect(), + } + } +} + +enum ConstructGraphResult { + // The Entry was satisfiable without waiting for any additional nodes to be satisfied. The result + // contains copies of the input Entry for each set subset of the parameters that satisfy it. + Fulfilled(Vec), + // The Entry was not satifiable with installed rules. + Unfulfillable, + // The dependencies of an Entry might be satisfiable, but is currently blocked waiting for the + // results of the given entries. + // + // Holds partially-fulfilled Entries which do not yet contain their full set of used parameters. + // These entries are only consumed the case when a caller is the source of a cycle, and in that + // case they represent everything except the caller's own parameters (which provides enough + // information for the caller to complete). + CycledOn { + cyclic_deps: HashSet, + partial_simplified_entries: Vec, + }, } // Given the task index and the root subjects, it produces a rule graph that allows dependency nodes @@ -165,25 +232,25 @@ impl<'t> GraphMaker<'t> { let param_types = vec![*subject_type].into_iter().collect(); if let Some(beginning_root) = self.gen_root_entry(¶m_types, product_type) { - self._construct_graph(vec![beginning_root]) + self.construct_graph(vec![beginning_root]) } else { RuleGraph::default() } } pub fn full_graph(&self) -> RuleGraph { - self._construct_graph(self.gen_root_entries(&self.tasks.all_product_types())) + self.construct_graph(self.gen_root_entries(&self.tasks.all_product_types())) } - pub fn _construct_graph(&self, roots: Vec) -> RuleGraph { + pub fn construct_graph(&self, roots: Vec) -> RuleGraph { let mut dependency_edges: RuleDependencyEdges = HashMap::new(); - let mut entry_equivalences = HashMap::new(); + let mut simplified_entries = HashMap::new(); let mut unfulfillable_rules: UnfulfillableRuleMap = HashMap::new(); for beginning_root in roots { - self._construct_graph_helper( + self.construct_graph_helper( &mut dependency_edges, - &mut entry_equivalences, + &mut simplified_entries, &mut unfulfillable_rules, EntryWithDeps::Root(beginning_root), ); @@ -230,26 +297,26 @@ impl<'t> GraphMaker<'t> { } /// - /// Computes whether the given candidate entry is satisfiable, and if it is, returns a copy - /// of the entry with its parameters pruned to what is actually used. Once computed, the - /// "equivalence" of the input entry to the output entry is memoized in entry_equivalences. + /// Computes whether the given candidate Entry is satisfiable, and if it is, returns a copy + /// of the Entry for each set of input parameters that will satisfy it. Once computed, the + /// simplified versions are memoized in all_simplified_entries. /// /// When a rule can be fulfilled it will end up stored in both the rule_dependency_edges and - /// the equivalances. If it can't be fulfilled, it is added to `unfulfillable_rules`. + /// all_simplified_entries. If it can't be fulfilled, it is added to `unfulfillable_rules`. /// - fn _construct_graph_helper( + fn construct_graph_helper( &self, rule_dependency_edges: &mut RuleDependencyEdges, - entry_equivalences: &mut HashMap, + all_simplified_entries: &mut HashMap>, unfulfillable_rules: &mut UnfulfillableRuleMap, entry: EntryWithDeps, - ) -> Option { - if let Some(equivalent) = entry_equivalences.get(&entry) { + ) -> ConstructGraphResult { + if let Some(simplified) = all_simplified_entries.get(&entry) { // A simplified equivalent entry has already been computed, return it. - return Some(equivalent.clone()); + return ConstructGraphResult::Fulfilled(simplified.clone()); } else if let Some(_) = unfulfillable_rules.get(&entry) { // The rule is unfulfillable. - return None; + return ConstructGraphResult::Unfulfillable; } // Otherwise, store a placeholder in the rule_dependency_edges map and then visit its @@ -260,22 +327,62 @@ impl<'t> GraphMaker<'t> { // contributes to whether the rule is satisfiable. match rule_dependency_edges.entry(entry.clone()) { hash_map::Entry::Vacant(re) => { - // When a rule has not been visited before, we visit it by storing a placeholder in the - // rule dependencies map (to prevent infinite recursion). + // When a rule has not been visited before, we start the visit by storing a placeholder in + // the rule dependencies map in order to detect rule cycles. re.insert(RuleEdges::default()); - }, - hash_map::Entry::Occupied(o) => + } + hash_map::Entry::Occupied(_) => { // We're currently recursively under this rule, but its simplified equivalence has not yet - // been computed. This entry will be rewritten with its equivalency when the parent - // completes. - return Some(o.key().clone()), + // been computed (or we would have returned it above). The cyclic parent(s) will complete + // before recursing to compute this node again. + let mut cyclic_deps = HashSet::new(); + cyclic_deps.insert(entry.clone()); + return ConstructGraphResult::CycledOn { + cyclic_deps, + partial_simplified_entries: vec![entry.simplified(BTreeSet::new())], + }; + } }; - // For each dependency of the rule, recurse for each potential match and collect RuleEdges. - let mut edges = RuleEdges::new(); - let mut fulfillable = true; - let mut used_params = BTreeSet::new(); - for select_key in entry.dependency_keys() { + // For each dependency of the rule, recurse for each potential match and collect RuleEdges and + // used parameters. + // + // This is a `loop` because if we discover that this entry needs to complete in order to break + // a cycle on itself, it will re-compute dependencies after having partially-completed. + loop { + if let Ok(res) = self.construct_dependencies( + rule_dependency_edges, + all_simplified_entries, + unfulfillable_rules, + entry.clone(), + ) { + break res; + } + } + } + + /// + /// For each dependency of the rule, recurse for each potential match and collect RuleEdges and + /// used parameters. + /// + /// This is called in a `loop` until it succeeds, because if we discover that this entry needs + /// to complete in order to break a cycle on itself, it will re-compute dependencies after having + /// partially-completed. + /// + fn construct_dependencies( + &self, + rule_dependency_edges: &mut RuleDependencyEdges, + all_simplified_entries: &mut HashMap>, + unfulfillable_rules: &mut UnfulfillableRuleMap, + entry: EntryWithDeps, + ) -> Result { + let mut fulfillable_candidates_by_key = HashMap::new(); + let mut cycled_on = HashSet::new(); + let mut unfulfillable_diagnostics = Vec::new(); + + let dependency_keys = entry.dependency_keys(); + + for select_key in dependency_keys { let (params, product) = match &select_key { &SelectKey::JustSelect(ref s) => (entry.params().clone(), s.product.clone()), &SelectKey::JustGet(ref g) => { @@ -288,68 +395,344 @@ impl<'t> GraphMaker<'t> { } }; - // Confirm that at least one candidate is fulfillable. - let fulfillable_candidates = rhs(&self.tasks, ¶ms, &product) - .into_iter() - .filter_map(|candidate| match candidate { - Entry::WithDeps(c) => { - if let Some(equivalence) = self._construct_graph_helper( - rule_dependency_edges, - entry_equivalences, - unfulfillable_rules, - c, - ) { - used_params.extend(equivalence.params().iter().cloned()); - Some(Entry::WithDeps(equivalence)) - } else { - None + // Collect fulfillable candidates, used parameters, and cyclic deps. + let mut cycled = false; + let mut fulfillable_candidates = fulfillable_candidates_by_key + .entry(select_key.clone()) + .or_insert_with(Vec::new); + for candidate in rhs(&self.tasks, ¶ms, &product) { + match candidate { + Entry::WithDeps(c) => match self.construct_graph_helper( + rule_dependency_edges, + all_simplified_entries, + unfulfillable_rules, + c.clone(), + ) { + ConstructGraphResult::Unfulfillable => {} + ConstructGraphResult::Fulfilled(simplified_entries) => { + fulfillable_candidates.push( + simplified_entries + .into_iter() + .map(Entry::WithDeps) + .collect::>(), + ); + } + ConstructGraphResult::CycledOn { + cyclic_deps, + partial_simplified_entries, + } => { + cycled = true; + cycled_on.extend(cyclic_deps); + fulfillable_candidates.push( + partial_simplified_entries + .into_iter() + .map(Entry::WithDeps) + .collect::>(), + ); } + }, + p @ Entry::Param(_) => { + fulfillable_candidates.push(vec![p]); } - Entry::Param(type_id) => { - used_params.insert(type_id); - Some(Entry::Param(type_id)) + s @ Entry::Singleton { .. } => { + fulfillable_candidates.push(vec![s]); } - s @ Entry::Singleton { .. } => Some(s), - }) - .collect::>(); + }; + } - if fulfillable_candidates.is_empty() { + if cycled { + // If any candidate triggered a cycle on a rule that has not yet completed, then we are not + // yet fulfillable, and should finish gathering any other cyclic rule dependencies. + continue; + } else if fulfillable_candidates.is_empty() { // If no candidates were fulfillable, this rule is not fulfillable. - unfulfillable_rules - .entry(entry.clone()) - .or_insert_with(Vec::new) - .push(Diagnostic { - params: params.clone(), - reason: format!( - "no rule was available to compute {} with parameter type{} {}", - type_constraint_str(product.clone()), - if params.len() > 1 { "s" } else { "" }, - params_str(¶ms), - ), - }); - fulfillable = false; - } else { - // Extend the RuleEdges for this SelectKey. - edges.add_edges_via(select_key, fulfillable_candidates); + unfulfillable_diagnostics.push(Diagnostic { + params: params.clone(), + reason: format!( + "no rule was available to compute {} with parameter type{} {}", + type_constraint_str(product.clone()), + if params.len() > 1 { "s" } else { "" }, + params_str(¶ms), + ), + details: vec![], + }); } } - if fulfillable { - // All dependencies were fulfillable: store the equivalence, and replace the placeholder. - // TODO: Compute used parameters above and store them here. - // TODO2: We also need to rewrite all edges with the new equivalence, because in cases where - // we recursed on ourself, nodes will have dependencies on us. - entry_equivalences.insert(entry.clone(), entry.clone()); - rule_dependency_edges.insert(entry.clone(), edges); - Some(entry) - } else { + // If any dependencies were completely unfulfillable, then whether or not there were cyclic + // dependencies isn't relevant. + if !unfulfillable_diagnostics.is_empty() { // Was not fulfillable. Remove the placeholder: the unfulfillable entries we stored will // prevent us from attempting to expand this node again. + unfulfillable_rules + .entry(entry.clone()) + .or_insert_with(Vec::new) + .extend(unfulfillable_diagnostics); rule_dependency_edges.remove(&entry); - None + return Ok(ConstructGraphResult::Unfulfillable); + } + + // No dependencies were completely unfulfillable (although some may have been cyclic). + // + // If this is an Aggregration, flatten the candidates by duplicating the SelectKey to treat + // each concrete rule as a group of candidates. Otherwise, flatten each group of candidates. + let flattened_fulfillable_candidates_by_key = fulfillable_candidates_by_key + .into_iter() + .map(|(k, candidate_group)| (k, Itertools::flatten(candidate_group.into_iter()).collect())) + .collect(); + + // Generate one Entry per legal combination of parameters. + let simplified_entries = + match Self::monomorphize(&entry, flattened_fulfillable_candidates_by_key) { + Ok(se) => se, + Err(ambiguous_diagnostics) => { + // At least one combination of the dependencies was ambiguous. + unfulfillable_rules + .entry(entry.clone()) + .or_insert_with(Vec::new) + .extend(ambiguous_diagnostics); + rule_dependency_edges.remove(&entry); + return Ok(ConstructGraphResult::Unfulfillable); + } + }; + let simplified_entries_only = simplified_entries + .iter() + .map(|(entry, _)| entry.clone()) + .collect(); + + if !cycled_on.is_empty() { + // The set of cycled dependencies can only contain call stack "parents" of the dependency: we + // remove this entry from the set (if we're in it), until the top-most cyclic parent + // (represented by an empty set) is the one that re-starts recursion. + cycled_on.remove(&entry); + if cycled_on.is_empty() { + // If we were the only member of the set of cyclic dependencies, then we are the top-most + // cyclic parent in the call stack, and we should complete. This represents the case where + // a rule recursively depends on itself, and thus "cannot complete without completing". + // + // Store our simplified equivalence and then re-execute our dependency discovery. In this + // second attempt our cyclic dependencies will use the simplified representation(s) to succeed. + all_simplified_entries.insert(entry.clone(), simplified_entries_only); + return Err(()); + } else { + // This rule may be fulfillable, but we can't compute its complete set of dependencies until + // parent rule entries complete. Remove our placeholder edges before returning. + rule_dependency_edges.remove(&entry); + return Ok(ConstructGraphResult::CycledOn { + cyclic_deps: cycled_on, + partial_simplified_entries: simplified_entries_only, + }); + } + } else { + // All dependencies were fulfillable and none were blocked on cycles. Remove the + // placeholder and store the simplified entries. + rule_dependency_edges.remove(&entry); + rule_dependency_edges.extend(simplified_entries); + + all_simplified_entries.insert(entry.clone(), simplified_entries_only.clone()); + return Ok(ConstructGraphResult::Fulfilled(simplified_entries_only)); } } + /// + /// Given an Entry and a mapping of all legal sources of each of its dependencies, generates a + /// simplified Entry for each legal combination of parameters. + /// + /// Computes the union of all parameters used by the dependencies, and then uses the powerset of + /// used parameters to filter the possible combinations of dependencies. If multiple choices of + /// dependencies are possible for any set of parameters, then the graph is ambiguous. + /// + fn monomorphize( + entry: &EntryWithDeps, + deps: Vec<(SelectKey, Vec)>, + ) -> Result, Vec> { + // Collect the powerset of the union of used parameters, ordered by set size. + let params_powerset: Vec> = { + let mut all_used_params = BTreeSet::new(); + for (key, inputs) in &deps { + for input in inputs { + all_used_params.extend(Self::used_params(key, input)); + } + } + // Compute the powerset ordered by ascending set size. + let all_used_params = all_used_params.into_iter().collect::>(); + let mut param_sets = Self::powerset(&all_used_params).collect::>(); + param_sets.sort_by(|l, r| l.len().cmp(&r.len())); + param_sets + }; + + // Then, for the powerset of used parameters, determine which dependency combinations are + // satisfiable. + let mut combinations: HashMap = HashMap::new(); + let mut diagnostics = Vec::new(); + for available_params in params_powerset { + let available_params = available_params.into_iter().collect(); + // If a subset of these parameters is already satisfied, skip. This has the effect of + // selecting the smallest sets of parameters that will satisfy a rule. + // NB: This scan over satisfied sets is linear, but should have a small N. + if combinations + .keys() + .any(|satisfied_entry| satisfied_entry.params().is_subset(&available_params)) + { + continue; + } + + match Self::choose_dependencies(&available_params, &deps) { + Ok(Some(inputs)) => { + let mut rule_edges = RuleEdges::default(); + for (key, input) in inputs { + rule_edges.add_edge(key.clone(), input.clone()); + } + combinations.insert(entry.simplified(available_params), rule_edges); + } + Ok(None) => {} + Err(diagnostic) => diagnostics.push(diagnostic), + } + } + + // If none of the combinations was satisfiable, return the generated diagnostics. + if !combinations.is_empty() { + Ok(combinations) + } else { + Err(diagnostics) + } + } + + /// + /// Given a set of available Params, choose one combination of satisfiable Entry dependencies if + /// it exists (it may not, because we're searching for sets of legal parameters in the powerset + /// of all used params). + /// + /// If an ambiguity is detected in rule dependencies (ie, if multiple rules are satisfiable for + /// a single dependency key), fail with a Diagnostic. + /// + fn choose_dependencies<'a>( + available_params: &ParamTypes, + deps: &'a Vec<(SelectKey, Vec)>, + ) -> Result>, Diagnostic> { + let mut combination = Vec::new(); + for (key, input_entries) in deps { + let satisfiable_entries = input_entries + .iter() + .filter(|input_entry| { + Self::used_params(key, input_entry) + .iter() + .all(|p| available_params.contains(p)) + }) + .collect::>(); + + let chosen_entries = Self::choose_dependency(satisfiable_entries); + match chosen_entries.len() { + 0 => { + return Ok(None); + } + 1 => { + combination.push((key, *chosen_entries.last().unwrap())); + } + _ => { + return Err(Diagnostic::ambiguous(available_params, key, chosen_entries)); + } + } + } + + Ok(Some(combination)) + } + + fn choose_dependency<'a>(satisfiable_entries: Vec<&'a Entry>) -> Vec<&'a Entry> { + if satisfiable_entries.is_empty() { + // No source of this dependency was satisfiable with these Params. + return vec![]; + } + + // Prefer a Singleton, then a Param, then the non-ambiguous rule with the smallest set of + // input Params. + // TODO: We should likely prefer Rules to Params. + if satisfiable_entries.len() == 1 { + satisfiable_entries + } else if let Some(singleton) = satisfiable_entries.iter().find(|e| match e { + &Entry::Singleton { .. } => true, + _ => false, + }) { + vec![*singleton] + } else if let Some(param) = satisfiable_entries.iter().find(|e| match e { + &Entry::Param(_) => true, + _ => false, + }) { + vec![*param] + } else { + // Group by the simplified version of each rule: if exactly one, we're finished. We prefer + // the non-ambiguous rule with the smallest set of Params, as that minimizes Node identities + // in the graph and biases toward receiving values from dependencies (which do not affect our + // identity) rather than dependents. + // + // TODO: There has to be a more efficient way to group by Entry. + let mut rules_by_kind: HashMap = HashMap::new(); + for satisfiable_entry in satisfiable_entries.iter() { + match satisfiable_entry { + &Entry::WithDeps(ref wd) => { + rules_by_kind + .entry(wd.simplified(BTreeSet::new())) + .and_modify(|e| { + // TODO: Param set size isn't sufficient to fully differentiate alternatives: would + // prefer a larger param set to a smaller one. + // TODO2: This might be redundant now that we are matching subsets in monomorphize. + if e.0 > wd.params().len() { + *e = (wd.params().len(), satisfiable_entry); + } + }) + .or_insert((wd.params().len(), satisfiable_entry)); + } + _ => {} + } + } + + rules_by_kind + .into_iter() + .map(|(_, (_, rule))| rule) + .collect::>() + } + } + + /// + /// Computes the parameters used by the given SelectKey and Entry. + /// + fn used_params(key: &SelectKey, entry: &Entry) -> Vec { + // `Get`s introduce new Params to a subgraph, so using a Param provided by a Get does not + // count toward an Entry's used params. + let provided_param = match &key { + &SelectKey::JustSelect(_) => None, + &SelectKey::JustGet(ref g) => Some(&g.subject), + }; + + match &entry { + &Entry::WithDeps(ref e) => e.params() + .iter() + .filter(|&type_id| Some(type_id) != provided_param) + .cloned() + .collect(), + &Entry::Param(ref type_id) if Some(type_id) != provided_param => vec![*type_id], + &Entry::Singleton { .. } | &Entry::Param(_) => vec![], + } + } + + fn powerset<'a, T: Clone>(slice: &'a [T]) -> impl Iterator> + 'a { + (0..(1 << slice.len())).into_iter().map(move |mask| { + let mut ss = Vec::new(); + let mut bitset = mask; + while bitset > 0 { + // isolate the rightmost bit to select one item + let rightmost: u64 = bitset & !(bitset - 1); + // turn the isolated bit into an array index + let idx = rightmost.trailing_zeros(); + let item = &slice[idx as usize]; + ss.push(item.clone()); + // zero the trailing bit + bitset &= bitset - 1; + } + ss + }) + } + fn gen_root_entries(&self, product_types: &HashSet) -> Vec { product_types .iter() @@ -441,6 +824,13 @@ fn val_name(val: &Value) -> String { externs::project_str(val, "__name__") } +pub fn select_key_str(select_key: &SelectKey) -> String { + match select_key { + &SelectKey::JustSelect(ref s) => select_str(s), + &SelectKey::JustGet(ref g) => get_str(g), + } +} + pub fn select_str(select: &Select) -> String { format!("Select({})", type_constraint_str(select.product)).to_string() // TODO variant key } @@ -453,7 +843,10 @@ fn get_str(get: &Get) -> String { ) } -fn entry_str(entry: &Entry) -> String { +/// +/// TODO: Move all of these methods to Display impls. +/// +pub fn entry_str(entry: &Entry) -> String { match entry { &Entry::WithDeps(ref e) => entry_with_deps_str(e), &Entry::Param(type_id) => format!("Param({})", type_str(type_id)), @@ -470,7 +863,10 @@ fn entry_with_deps_str(entry: &EntryWithDeps) -> String { &EntryWithDeps::Inner(InnerEntry { rule: Rule::Task(ref task_rule), ref params, - }) => format!("{} of {}", task_display(task_rule), params_str(params)), + }) => { + // TODO: `for`, not `of` + format!("{} of {}", task_display(task_rule), params_str(params)) + } &EntryWithDeps::Inner(InnerEntry { rule: Rule::Intrinsic(ref intrinsic), ref params, @@ -547,17 +943,6 @@ impl RuleGraph { /// TODO: It's not clear what is preventing `Node` implementations from ending up with non-Inner /// entries, but it would be good to make it typesafe instead. /// - pub fn rule_for_inner<'a>(&self, entry: &'a Entry) -> &'a Rule { - if let &Entry::WithDeps(EntryWithDeps::Inner(ref inner)) = entry { - &inner.rule - } else { - panic!("not an inner entry! {:?}", entry) - } - } - - /// - /// TODO: See rule_for_inner. - /// pub fn edges_for_inner(&self, entry: &Entry) -> Option { if let &Entry::WithDeps(ref e) = entry { self.rule_dependency_edges.get(e).cloned() @@ -567,7 +952,7 @@ impl RuleGraph { } pub fn validate(&self) -> Result<(), String> { - let mut collated_errors: HashMap> = HashMap::new(); + let mut collated_errors: HashMap> = HashMap::new(); let used_rules: HashSet<_> = self .rule_dependency_edges @@ -608,7 +993,7 @@ impl RuleGraph { collated_errors .entry(task_rule.clone()) .or_insert_with(Vec::new) - .push(d.reason.clone()); + .push(d); } } @@ -618,9 +1003,21 @@ impl RuleGraph { let mut msgs: Vec = collated_errors .into_iter() - .map(|(rule, mut errors)| { - errors.sort(); - format!("{}:\n {}", task_display(&rule), errors.join("\n ")) + .map(|(rule, mut diagnostics)| { + diagnostics.sort_by(|l, r| l.reason.cmp(&r.reason)); + diagnostics.dedup_by(|l, r| l.reason == r.reason); + let errors = diagnostics + .into_iter() + .map(|d| { + if d.details.is_empty() { + format!("{}", d.reason) + } else { + format!("{}:\n {}", d.reason, d.details.join("\n ")) + } + }) + .collect::>() + .join("\n "); + format!("{}:\n {}", task_display(&rule), errors) }) .collect(); msgs.sort(); @@ -653,8 +1050,7 @@ impl RuleGraph { root_str, root_str, deps - .dependencies - .iter() + .all_dependencies() .map(|d| format!("\"{}\"", entry_str(d))) .collect::>() .join(" ") @@ -675,8 +1071,7 @@ impl RuleGraph { " \"{}\" -> {{{}}}", entry_with_deps_str(k), deps - .dependencies - .iter() + .all_dependencies() .map(|d| format!("\"{}\"", entry_str(d))) .collect::>() .join(" ") @@ -690,62 +1085,39 @@ impl RuleGraph { } } +/// +/// Records the dependency rules for a rule. +/// #[derive(Eq, PartialEq, Clone, Debug, Default)] pub struct RuleEdges { - dependencies: Entries, - dependencies_by_select_key: HashMap, + dependencies: HashMap>, } impl RuleEdges { - pub fn new() -> RuleEdges { - RuleEdges { - dependencies: vec![], - dependencies_by_select_key: HashMap::new(), - } - } - - pub fn entries_for(&self, select_key: &SelectKey, param_values: &Params) -> Entries { + pub fn entry_for(&self, select_key: &SelectKey) -> Option<&Entry> { self - .dependencies_by_select_key + .dependencies .get(select_key) - .map(|entries| { - entries - .into_iter() - .filter(|&entry| match entry { - &Entry::WithDeps(EntryWithDeps::Root(RootEntry { ref params, .. })) - | &Entry::WithDeps(EntryWithDeps::Inner(InnerEntry { ref params, .. })) => params - .iter() - .all(|type_id| param_values.find(*type_id).is_some()), - &Entry::Param(type_id) => param_values.find(type_id).is_some(), - &Entry::Singleton { .. } => true, - }) - .cloned() - .collect() - }) - .unwrap_or_else(Vec::new) + .and_then(|entries| entries.first()) } - pub fn is_empty(&self) -> bool { - self.dependencies.is_empty() + pub fn all_dependencies(&self) -> impl Iterator { + Itertools::flatten(self.dependencies.values()) } - fn add_edges_via(&mut self, select_key: SelectKey, new_dependencies: Entries) { - let deps_for_selector = self - .dependencies_by_select_key + fn add_edge(&mut self, select_key: SelectKey, new_dependency: Entry) { + self + .dependencies .entry(select_key) - .or_insert_with(Vec::new); - for d in new_dependencies { - if !deps_for_selector.contains(&d) { - deps_for_selector.push(d.clone()); - } - if !self.dependencies.contains(&d) { - self.dependencies.push(d); - } - } + .or_insert_with(Vec::new) + .push(new_dependency); } } -fn rhs(tasks: &Tasks, params: &ParamTypes, product_type: &TypeConstraint) -> Entries { +/// +/// Select Entries that can provide the given product type with the given parameters. +/// +fn rhs(tasks: &Tasks, params: &ParamTypes, product_type: &TypeConstraint) -> Vec { if let Some(&(ref key, _)) = tasks.gen_singleton(product_type) { return vec![Entry::Singleton(*key, *product_type)]; } diff --git a/src/rust/engine/src/tasks.rs b/src/rust/engine/src/tasks.rs index 3ee1e374c63..69f9fc61111 100644 --- a/src/rust/engine/src/tasks.rs +++ b/src/rust/engine/src/tasks.rs @@ -38,8 +38,8 @@ pub struct Tasks { /// 2. add_*() - zero or more times per task to add input clauses /// 3. task_end() - once per task /// -/// Also has a one-shot method for adding a singleton (which has no Selects): -/// 1. singleton_add() +/// Also has a one-shot method for adding Singletons (which have no Selects): +/// * singleton_add() /// /// (This protocol was original defined in a Builder, but that complicated the C lifecycle.) /// @@ -112,7 +112,7 @@ impl Tasks { pub fn singleton_add(&mut self, value: Value, product: TypeConstraint) { if let Some(&(_, ref existing_value)) = self.singletons.get(&product) { panic!( - "More than one singleton rule was installed for the product {:?}: {:?} vs {:?}", + "More than one Singleton rule was installed for the product {:?}: {:?} vs {:?}", product, existing_value, value, ); } From a6ff9eb8d21a1f4a6f2679156c14e778e57469d6 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 18 Jul 2018 14:56:27 -0700 Subject: [PATCH 05/13] Drop some unnecessary RootRules. --- .../pants/backend/native/subsystems/binaries/binutils.py | 3 +-- src/python/pants/backend/native/subsystems/binaries/gcc.py | 3 +-- src/python/pants/backend/native/subsystems/xcode_cli_tools.py | 3 +-- src/python/pants/engine/fs.py | 1 - 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/python/pants/backend/native/subsystems/binaries/binutils.py b/src/python/pants/backend/native/subsystems/binaries/binutils.py index 63c554261b7..e3fa9b2214b 100644 --- a/src/python/pants/backend/native/subsystems/binaries/binutils.py +++ b/src/python/pants/backend/native/subsystems/binaries/binutils.py @@ -8,7 +8,7 @@ from pants.backend.native.config.environment import Assembler, Linker from pants.binaries.binary_tool import NativeTool -from pants.engine.rules import RootRule, rule +from pants.engine.rules import rule from pants.engine.selectors import Select @@ -49,5 +49,4 @@ def create_binutils_rules(): return [ get_as, get_ld, - RootRule(Binutils), ] diff --git a/src/python/pants/backend/native/subsystems/binaries/gcc.py b/src/python/pants/backend/native/subsystems/binaries/gcc.py index 0d2b253f82a..3f7781ab701 100644 --- a/src/python/pants/backend/native/subsystems/binaries/gcc.py +++ b/src/python/pants/backend/native/subsystems/binaries/gcc.py @@ -9,7 +9,7 @@ from pants.backend.native.config.environment import CCompiler, CppCompiler, Platform from pants.backend.native.subsystems.utils.archive_file_mapper import ArchiveFileMapper from pants.binaries.binary_tool import NativeTool -from pants.engine.rules import RootRule, rule +from pants.engine.rules import rule from pants.engine.selectors import Select from pants.util.memo import memoized_method, memoized_property @@ -108,5 +108,4 @@ def create_gcc_rules(): return [ get_gcc, get_gplusplus, - RootRule(GCC), ] diff --git a/src/python/pants/backend/native/subsystems/xcode_cli_tools.py b/src/python/pants/backend/native/subsystems/xcode_cli_tools.py index bdba39c75d5..91d380ce2b1 100644 --- a/src/python/pants/backend/native/subsystems/xcode_cli_tools.py +++ b/src/python/pants/backend/native/subsystems/xcode_cli_tools.py @@ -7,7 +7,7 @@ import os from pants.backend.native.config.environment import Assembler, CCompiler, CppCompiler, Linker -from pants.engine.rules import RootRule, rule +from pants.engine.rules import rule from pants.engine.selectors import Select from pants.subsystem.subsystem import Subsystem from pants.util.dirutil import is_readable_dir @@ -196,5 +196,4 @@ def create_xcode_cli_tools_rules(): get_ld, get_clang, get_clang_plusplus, - RootRule(XCodeCLITools), ] diff --git a/src/python/pants/engine/fs.py b/src/python/pants/engine/fs.py index 01398318fe5..1109f7bd6e2 100644 --- a/src/python/pants/engine/fs.py +++ b/src/python/pants/engine/fs.py @@ -144,5 +144,4 @@ def create_fs_rules(): return [ RootRule(DirectoryDigest), RootRule(PathGlobs), - RootRule(Snapshot), ] From 4d1c58fca2e3ec17e0ae7d071fdc7d34a1ccf445 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 17 Jul 2018 16:16:38 -0700 Subject: [PATCH 06/13] Remove planners.py, which is too reliant on ambiguous rules to continue to support. --- src/python/pants/engine/scheduler.py | 3 + src/rust/engine/src/rule_graph.rs | 3 +- tests/python/pants_test/engine/BUILD | 15 - tests/python/pants_test/engine/examples/BUILD | 53 -- .../pants_test/engine/examples/planners.py | 480 ------------------ .../pants_test/engine/examples/visualizer.py | 83 --- tests/python/pants_test/engine/test_engine.py | 37 +- tests/python/pants_test/engine/test_graph.py | 161 ------ tests/python/pants_test/engine/test_rules.py | 8 - .../pants_test/engine/test_scheduler.py | 232 +-------- 10 files changed, 9 insertions(+), 1066 deletions(-) delete mode 100644 tests/python/pants_test/engine/examples/planners.py delete mode 100644 tests/python/pants_test/engine/examples/visualizer.py delete mode 100644 tests/python/pants_test/engine/test_graph.py diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index 5028333b746..8e1c6a69782 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -413,6 +413,9 @@ def __init__(self, scheduler, session): self._session = session self._run_count = 0 + def assert_ruleset_valid(self): + self._scheduler._assert_ruleset_valid() + def graph_len(self): return self._scheduler.graph_len() diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index 94ab4a636d6..881c35aea1b 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -705,7 +705,8 @@ impl<'t> GraphMaker<'t> { }; match &entry { - &Entry::WithDeps(ref e) => e.params() + &Entry::WithDeps(ref e) => e + .params() .iter() .filter(|&type_id| Some(type_id) != provided_param) .cloned() diff --git a/tests/python/pants_test/engine/BUILD b/tests/python/pants_test/engine/BUILD index 0e9a26d3447..09b1f3d11a9 100644 --- a/tests/python/pants_test/engine/BUILD +++ b/tests/python/pants_test/engine/BUILD @@ -76,18 +76,6 @@ python_tests( ] ) -python_tests( - name='graph', - sources=['test_graph.py'], - coverage=['pants.engine.graph'], - dependencies=[ - '3rdparty/python:future', - 'src/python/pants/build_graph', - 'src/python/pants/engine:scheduler', - 'tests/python/pants_test/engine/examples:planners', - ] -) - python_tests( name='isolated_process', sources=['test_isolated_process.py'], @@ -135,7 +123,6 @@ python_tests( 'src/python/pants/engine:nodes', 'src/python/pants/engine:scheduler', 'src/python/pants/util:contextutil', - 'tests/python/pants_test/engine/examples:planners', 'tests/python/pants_test/engine/examples:scheduler_inputs', 'tests/python/pants_test/engine:util', ] @@ -201,7 +188,6 @@ python_tests( 'src/python/pants/engine:selectors', 'src/python/pants/util:objects', 'tests/python/pants_test/engine/examples:parsers', - 'tests/python/pants_test/engine/examples:planners', 'tests/python/pants_test/subsystem:subsystem_utils', ] ) @@ -216,7 +202,6 @@ python_tests( 'src/python/pants/base:cmd_line_spec_parser', 'src/python/pants/build_graph', 'src/python/pants/engine:scheduler', - 'tests/python/pants_test/engine/examples:planners', 'tests/python/pants_test/engine/examples:scheduler_inputs', ] ) diff --git a/tests/python/pants_test/engine/examples/BUILD b/tests/python/pants_test/engine/examples/BUILD index 9f764a1840f..353de5ece9a 100644 --- a/tests/python/pants_test/engine/examples/BUILD +++ b/tests/python/pants_test/engine/examples/BUILD @@ -19,27 +19,6 @@ python_library( ] ) -python_library( - name='planners', - sources=['planners.py'], - dependencies=[ - '3rdparty/python:future', - ':parsers', - ':sources', - 'src/python/pants/base:exceptions', - 'src/python/pants/base:project_tree', - 'src/python/pants/build_graph', - 'src/python/pants/engine:build_files', - 'src/python/pants/engine:fs', - 'src/python/pants/engine:mapper', - 'src/python/pants/engine:nodes', - 'src/python/pants/engine:parser', - 'src/python/pants/engine:scheduler', - 'src/python/pants/engine:selectors', - 'src/python/pants/engine:struct', - ] -) - python_library( name='sources', sources=['sources.py'], @@ -52,38 +31,6 @@ python_library( ] ) -python_library( - name='visualizer', - sources=['visualizer.py'], - dependencies=[ - ':planners', - 'src/python/pants/base:cmd_line_spec_parser', - 'src/python/pants/binaries', - 'src/python/pants/build_graph', - 'src/python/pants/engine:scheduler', - 'src/python/pants/util:contextutil', - 'src/python/pants/util:desktop', - 'src/python/pants/util:process_handler', - 'tests/python/pants_test/engine:util', - ] -) - -python_binary( - name='viz', - entry_point='pants_test.engine.examples.visualizer:main_addresses', - dependencies=[ - ':visualizer' - ] -) - -python_binary( - name='viz-fs', - entry_point='pants_test.engine.examples.visualizer:main_filespecs', - dependencies=[ - ':visualizer' - ] -) - resources( name='fs_test', # Note that this test data dir is bundled into a tarfile, to preserve symlink structure diff --git a/tests/python/pants_test/engine/examples/planners.py b/tests/python/pants_test/engine/examples/planners.py deleted file mode 100644 index 2aed7185976..00000000000 --- a/tests/python/pants_test/engine/examples/planners.py +++ /dev/null @@ -1,480 +0,0 @@ -# coding=utf-8 -# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). -# Licensed under the Apache License, Version 2.0 (see LICENSE). - -from __future__ import absolute_import, division, print_function, unicode_literals - -import functools -import re -from abc import abstractmethod -from builtins import str -from os import sep as os_sep -from os.path import join as os_path_join - -from pants.base.exceptions import TaskError -from pants.base.file_system_project_tree import FileSystemProjectTree -from pants.base.project_tree import Dir -from pants.build_graph.address import Address -from pants.engine.addressable import addressable_list -from pants.engine.build_files import create_graph_rules -from pants.engine.fs import DirectoryDigest, FilesContent, PathGlobs, Snapshot, create_fs_rules -from pants.engine.mapper import AddressFamily, AddressMapper -from pants.engine.parser import SymbolTable -from pants.engine.rules import SingletonRule, TaskRule, rule -from pants.engine.scheduler import Scheduler -from pants.engine.selectors import Get, Select, SelectVariant -from pants.engine.struct import HasProducts, Struct, StructWithDeps, Variants -from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS -from pants.util.meta import AbstractClass -from pants.util.objects import SubclassesOf, datatype -from pants_test.engine.examples.parsers import JsonParser -from pants_test.engine.examples.sources import Sources - - -def printing_func(func): - @functools.wraps(func) - def wrapper(*inputs): - product = func(*inputs) - return_val = product if product else '<<>>'.format(func.__name__) - print('{} executed for {}, returned: {}'.format(func.__name__, inputs, return_val)) - return return_val - return wrapper - - -class Target(Struct, HasProducts): - """A placeholder for the most-numerous Struct subclass. - - This particular implementation holds a collection of other Structs in a `configurations` field. - """ - - def __init__(self, name=None, configurations=None, **kwargs): - """ - :param string name: The name of this target which forms its address in its namespace. - :param list configurations: The configurations that apply to this target in various contexts. - """ - super(Target, self).__init__(name=name, **kwargs) - - self.configurations = configurations - - @property - def products(self): - return self.configurations - - @addressable_list(SubclassesOf(Struct)) - def configurations(self): - """The configurations that apply to this target in various contexts. - - :rtype list of :class:`pants.engine.configuration.Struct` - """ - - -class JavaSources(Sources, StructWithDeps): - extensions = ('.java',) - - -class ScalaSources(Sources, StructWithDeps): - extensions = ('.scala',) - - -class PythonSources(Sources, StructWithDeps): - extensions = ('.py',) - - -class ThriftSources(Sources, StructWithDeps): - extensions = ('.thrift',) - - -class ResourceSources(Sources): - extensions = tuple() - - -class ScalaInferredDepsSources(Sources): - """A Sources subclass which can be converted to ScalaSources via dep inference.""" - extensions = ('.scala',) - - -class JVMPackageName(datatype(['name'])): - """A typedef to represent a fully qualified JVM package name.""" - pass - - -class SourceRoots(datatype(['srcroots'])): - """Placeholder for the SourceRoot subsystem.""" - - -@printing_func -@rule(Address, [Select(JVMPackageName), Select(Snapshot)]) -def select_package_address(jvm_package_name, snapshot): - """Return the Address from the given AddressFamilies which provides the given package.""" - address_families = yield [Get(AddressFamily, Dir, ds) for ds in snapshot.dir_stats] - addresses = [address for address_family in address_families - for address in address_family.addressables.keys()] - if len(addresses) == 0: - raise ValueError('No targets existed in {} to provide {}'.format( - address_families, jvm_package_name)) - elif len(addresses) > 1: - raise ValueError('Multiple targets might be able to provide {}:\n {}'.format( - jvm_package_name, '\n '.join(str(a) for a in addresses))) - yield addresses[0].to_address() - - -@printing_func -@rule(PathGlobs, [Select(JVMPackageName), Select(SourceRoots)]) -def calculate_package_search_path(jvm_package_name, source_roots): - """Return PathGlobs to match directories where the given JVMPackageName might exist.""" - rel_package_dir = jvm_package_name.name.replace('.', os_sep) - specs = [os_path_join(srcroot, rel_package_dir) for srcroot in source_roots.srcroots] - return PathGlobs(include=specs) - - -@printing_func -@rule(ScalaSources, [Select(ScalaInferredDepsSources)]) -def reify_scala_sources(sources): - """Given a ScalaInferredDepsSources object, create ScalaSources.""" - snapshot = yield Get(Snapshot, PathGlobs, sources.path_globs) - source_files_content = yield Get(FilesContent, DirectoryDigest, snapshot.directory_digest) - packages = set() - import_re = re.compile(r'^import ([^;]*);?$') - for filecontent in source_files_content.dependencies: - for line in filecontent.content.decode('utf-8').splitlines(): - match = import_re.search(line) - if match: - packages.add(match.group(1).rsplit('.', 1)[0]) - - dependency_addresses = yield [Get(Address, JVMPackageName(p)) for p in packages] - - kwargs = sources._asdict() - kwargs['dependencies'] = list(set(dependency_addresses)) - yield ScalaSources(**kwargs) - - -class Requirement(Struct): - """A setuptools requirement.""" - - def __init__(self, req, repo=None, **kwargs): - """ - :param string req: A setuptools compatible requirement specifier; eg: `pantsbuild.pants>0.0.42`. - :param string repo: An optional custom find-links repo URL. - """ - super(Requirement, self).__init__(req=req, repo=repo, **kwargs) - - -class Classpath(Struct): - """Placeholder product.""" - - def __init__(self, creator, **kwargs): - super(Classpath, self).__init__(creator=creator, **kwargs) - - -class ManagedResolve(Struct): - """A frozen ivy resolve that when combined with a ManagedJar can produce a Jar.""" - - def __init__(self, revs, **kwargs): - """ - :param dict revs: A dict of artifact org#name to version. - """ - super(ManagedResolve, self).__init__(revs=revs, **kwargs) - - def __repr__(self): - return "ManagedResolve({})".format(self.revs) - - -class Jar(Struct): - """A java jar.""" - - def __init__(self, org=None, name=None, rev=None, **kwargs): - """ - :param string org: The Maven ``groupId`` of this dependency. - :param string name: The Maven ``artifactId`` of this dependency; also serves as the name portion - of the address of this jar if defined at the top level of a BUILD file. - :param string rev: The Maven ``version`` of this dependency. - """ - super(Jar, self).__init__(org=org, name=name, rev=rev, **kwargs) - - -class ManagedJar(Struct): - """A java jar template, which can be merged with a ManagedResolve to determine a concrete version.""" - - def __init__(self, org, name, **kwargs): - """ - :param string org: The Maven ``groupId`` of this dependency. - :param string name: The Maven ``artifactId`` of this dependency; also serves as the name portion - of the address of this jar if defined at the top level of a BUILD file. - """ - super(ManagedJar, self).__init__(org=org, name=name, **kwargs) - - -@printing_func -@rule(Jar, [Select(ManagedJar), SelectVariant(ManagedResolve, 'resolve')]) -def select_rev(managed_jar, managed_resolve): - (org, name) = (managed_jar.org, managed_jar.name) - rev = managed_resolve.revs.get('{}#{}'.format(org, name), None) - if not rev: - raise TaskError('{} does not have a managed version in {}.'.format(managed_jar, managed_resolve)) - return Jar(org=managed_jar.org, name=managed_jar.name, rev=rev) - - -@printing_func -@rule(Classpath, [Select(Jar)]) -def ivy_resolve(jars): - return Classpath(creator='ivy_resolve') - - -@printing_func -@rule(Classpath, [Select(ResourceSources)]) -def isolate_resources(resources): - """Copies resources into a private directory, and provides them as a Classpath entry.""" - return Classpath(creator='isolate_resources') - - -class ThriftConfiguration(StructWithDeps): - pass - - -class ApacheThriftConfiguration(ThriftConfiguration): - def __init__(self, rev=None, strict=True, **kwargs): - """ - :param string rev: The version of the apache thrift compiler to use. - :param bool strict: `False` to turn strict compiler warnings off (not recommended). - """ - super(ApacheThriftConfiguration, self).__init__(rev=rev, strict=strict, **kwargs) - - -class ApacheThriftJavaConfiguration(ApacheThriftConfiguration): - pass - - -class ApacheThriftPythonConfiguration(ApacheThriftConfiguration): - pass - - -class ApacheThriftError(TaskError): - pass - - -@rule(JavaSources, [Select(ThriftSources), SelectVariant(ApacheThriftJavaConfiguration, 'thrift')]) -def gen_apache_java_thrift(sources, config): - return gen_apache_thrift(sources, config) - - -@rule(PythonSources, [Select(ThriftSources), SelectVariant(ApacheThriftPythonConfiguration, 'thrift')]) -def gen_apache_python_thrift(sources, config): - return gen_apache_thrift(sources, config) - - -@printing_func -def gen_apache_thrift(sources, config): - if config.rev == 'fail': - raise ApacheThriftError('Failed to generate via apache thrift for ' - 'sources: {}, config: {}'.format(sources, config)) - if isinstance(config, ApacheThriftJavaConfiguration): - return JavaSources(files=['Fake.java'], dependencies=config.dependencies) - elif isinstance(config, ApacheThriftPythonConfiguration): - return PythonSources(files=['fake.py'], dependencies=config.dependencies) - - -class BuildPropertiesConfiguration(Struct): - pass - - -@printing_func -@rule(Classpath, [Select(BuildPropertiesConfiguration)]) -def write_name_file(name): - """Write a file containing the name of this target in the CWD.""" - return Classpath(creator='write_name_file') - - -class Scrooge(datatype(['tool_address'])): - """Placeholder for a Scrooge subsystem.""" - - -class ScroogeConfiguration(ThriftConfiguration): - def __init__(self, rev=None, strict=True, **kwargs): - """ - :param string rev: The version of the scrooge compiler to use. - :param bool strict: `False` to turn strict compiler warnings off (not recommended). - """ - super(ScroogeConfiguration, self).__init__(rev=rev, strict=strict, **kwargs) - - -class ScroogeScalaConfiguration(ScroogeConfiguration): - pass - - -class ScroogeJavaConfiguration(ScroogeConfiguration): - pass - - -@rule(ScalaSources, - [Select(ThriftSources), - SelectVariant(ScroogeScalaConfiguration, 'thrift')]) -def gen_scrooge_scala_thrift(sources, config): - scrooge_classpath = yield Get(Classpath, Address, Scrooge.tool_address) - yield gen_scrooge_thrift(sources, config, scrooge_classpath) - - -@rule(JavaSources, - [Select(ThriftSources), - SelectVariant(ScroogeJavaConfiguration, 'thrift')]) -def gen_scrooge_java_thrift(sources, config): - scrooge_classpath = yield Get(Classpath, Address, Scrooge.tool_address) - yield gen_scrooge_thrift(sources, config, scrooge_classpath) - - -@printing_func -def gen_scrooge_thrift(sources, config, scrooge_classpath): - if isinstance(config, ScroogeJavaConfiguration): - return JavaSources(files=['Fake.java'], dependencies=config.dependencies) - elif isinstance(config, ScroogeScalaConfiguration): - return ScalaSources(files=['Fake.scala'], dependencies=config.dependencies) - - -@printing_func -@rule(Classpath, [Select(JavaSources)]) -def javac(sources): - classpath = yield [(Get(Classpath, Address, d) if type(d) is Address else Get(Classpath, Jar, d)) - for d in sources.dependencies] - print('compiling {} with {}'.format(sources, classpath)) - yield Classpath(creator='javac') - - -@printing_func -@rule(Classpath, [Select(ScalaSources)]) -def scalac(sources): - classpath = yield [(Get(Classpath, Address, d) if type(d) is Address else Get(Classpath, Jar, d)) - for d in sources.dependencies] - print('compiling {} with {}'.format(sources, classpath)) - yield Classpath(creator='scalac') - - -class Goal(AbstractClass): - """A synthetic aggregate product produced by a goal, which is its own task.""" - - def __init__(self, *args): - if all(arg is None for arg in args): - msg = '\n '.join(p.__name__ for p in self.products()) - raise TaskError('Unable to produce any of the products for goal `{}`:\n {}'.format( - self.name(), msg)) - - @classmethod - @abstractmethod - def name(cls): - """Returns the name of the Goal.""" - - @classmethod - def rule(cls): - """Returns a Rule for this Goal, used to install the Goal. - - A Goal is it's own synthetic output product, and its constructor acts as its task function. It - selects each of its products as optional, but fails synchronously if none of them are available. - """ - return TaskRule(cls, [Select(p, optional=True) for p in cls.products()], cls) - - @classmethod - @abstractmethod - def products(cls): - """Returns the products that this Goal requests.""" - - def __eq__(self, other): - return type(self) == type(other) - - def __ne__(self, other): - return not (self == other) - - def __hash__(self): - return hash(type(self)) - - def __str__(self): - return '{}()'.format(type(self).__name__) - - def __repr__(self): - return str(self) - - -class GenGoal(Goal): - """A goal that requests all known types of sources.""" - - @classmethod - def name(cls): - return 'gen' - - @classmethod - def products(cls): - return [JavaSources, PythonSources, ResourceSources, ScalaSources] - - -class ExampleTable(SymbolTable): - @classmethod - def table(cls): - return {'apache_thrift_java_configuration': ApacheThriftJavaConfiguration, - 'apache_thrift_python_configuration': ApacheThriftPythonConfiguration, - 'jar': Jar, - 'managed_jar': ManagedJar, - 'managed_resolve': ManagedResolve, - 'requirement': Requirement, - 'scrooge_java_configuration': ScroogeJavaConfiguration, - 'scrooge_scala_configuration': ScroogeScalaConfiguration, - 'java': JavaSources, - 'python': PythonSources, - 'resources': ResourceSources, - 'scala': ScalaSources, - 'thrift': ThriftSources, - 'target': Target, - 'variants': Variants, - 'build_properties': BuildPropertiesConfiguration, - 'inferred_scala': ScalaInferredDepsSources} - - -def setup_json_scheduler(build_root, native): - """Return a build graph and scheduler configured for BLD.json files under the given build root. - - :rtype :class:`pants.engine.scheduler.SchedulerSession` - """ - - symbol_table = ExampleTable() - - # Register "literal" subjects required for these rules. - address_mapper = AddressMapper(build_patterns=('BLD.json',), - parser=JsonParser(symbol_table)) - - work_dir = os_path_join(build_root, '.pants.d') - project_tree = FileSystemProjectTree(build_root) - - rules = [ - # Codegen - GenGoal.rule(), - gen_apache_java_thrift, - gen_apache_python_thrift, - gen_scrooge_scala_thrift, - gen_scrooge_java_thrift, - SingletonRule(Scrooge, Scrooge(Address.parse('src/scala/scrooge'))) - ] + [ - # scala dependency inference - reify_scala_sources, - select_package_address, - calculate_package_search_path, - SingletonRule(SourceRoots, SourceRoots(('src/java','src/scala'))), - ] + [ - # Remote dependency resolution - ivy_resolve, - select_rev, - ] + [ - # Compilers - isolate_resources, - write_name_file, - javac, - scalac, - ] + ( - create_graph_rules(address_mapper, symbol_table) - ) + ( - create_fs_rules() - ) - - scheduler = Scheduler(native, - project_tree, - work_dir, - rules, - DEFAULT_EXECUTION_OPTIONS, - None, - None) - return scheduler.new_session() diff --git a/tests/python/pants_test/engine/examples/visualizer.py b/tests/python/pants_test/engine/examples/visualizer.py deleted file mode 100644 index 3d271f6334d..00000000000 --- a/tests/python/pants_test/engine/examples/visualizer.py +++ /dev/null @@ -1,83 +0,0 @@ -# coding=utf-8 -# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). -# Licensed under the Apache License, Version 2.0 (see LICENSE). - -from __future__ import absolute_import, division, print_function, unicode_literals - -import os -import sys -from textwrap import dedent - -from pants.base.cmd_line_spec_parser import CmdLineSpecParser -from pants.engine.fs import PathGlobs -from pants.util import desktop -from pants.util.contextutil import temporary_file_path -from pants.util.process_handler import subprocess -from pants_test.engine.examples.planners import setup_json_scheduler -from pants_test.engine.util import init_native - - -# TODO: These aren't tests themselves, so they should be under examples/ or testprojects/? -def visualize_execution_graph(scheduler): - with temporary_file_path(cleanup=False, suffix='.dot') as dot_file: - scheduler.visualize_graph_to_file(dot_file) - print('dot file saved to: {}'.format(dot_file)) - - with temporary_file_path(cleanup=False, suffix='.svg') as image_file: - subprocess.check_call('dot -Tsvg -o{} {}'.format(image_file, dot_file), shell=True) - print('svg file saved to: {}'.format(image_file)) - desktop.ui_open(image_file) - - -def visualize_build_request(build_root, goals, subjects): - native = init_native() - scheduler = setup_json_scheduler(build_root, native) - - execution_request = scheduler.build_request(goals, subjects) - # NB: Calls `schedule` independently of `execute`, in order to render a graph before validating it. - scheduler.schedule(execution_request) - visualize_execution_graph(scheduler) - - -def pop_build_root_and_goals(description, args): - def usage(error_message): - print(error_message, file=sys.stderr) - print(dedent(""" - {} - """.format(sys.argv[0])), file=sys.stderr) - sys.exit(1) - - if len(args) < 2: - usage('Must supply at least the build root path and one goal: {}'.format(description)) - - build_root = args.pop(0) - - if not os.path.isdir(build_root): - usage('First argument must be a valid build root, {} is not a directory.'.format(build_root)) - build_root = os.path.realpath(build_root) - - def is_goal(arg): return os.path.sep not in arg - - goals = [arg for arg in args if is_goal(arg)] - if not goals: - usage('Must supply at least one goal.') - - return build_root, goals, [arg for arg in args if not is_goal(arg)] - - -def main_addresses(): - build_root, goals, args = pop_build_root_and_goals( - '[build root path] [goal]+ [address spec]*', sys.argv[1:]) - - cmd_line_spec_parser = CmdLineSpecParser(build_root) - spec_roots = [cmd_line_spec_parser.parse_spec(spec) for spec in args] - visualize_build_request(build_root, goals, spec_roots) - - -def main_filespecs(): - build_root, goals, args = pop_build_root_and_goals( - '[build root path] [filespecs]*', sys.argv[1:]) - - # Create PathGlobs for each arg relative to the buildroot. - path_globs = PathGlobs(include=args) - visualize_build_request(build_root, goals, path_globs) diff --git a/tests/python/pants_test/engine/test_engine.py b/tests/python/pants_test/engine/test_engine.py index dd6ba78365a..c08feb4d63d 100644 --- a/tests/python/pants_test/engine/test_engine.py +++ b/tests/python/pants_test/engine/test_engine.py @@ -4,51 +4,16 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import os import unittest from builtins import object, str from textwrap import dedent -from pants.build_graph.address import Address -from pants.engine.nodes import Return from pants.engine.rules import RootRule, TaskRule, rule from pants.engine.scheduler import ExecutionError from pants.engine.selectors import Get, Select -from pants.util.contextutil import temporary_dir from pants.util.objects import datatype -from pants_test.engine.examples.planners import Classpath, setup_json_scheduler from pants_test.engine.scheduler_test_base import SchedulerTestBase -from pants_test.engine.util import (assert_equal_with_printing, init_native, - remove_locations_from_traceback) - - -class EngineExamplesTest(unittest.TestCase): - - _native = init_native() - - def setUp(self): - build_root = os.path.join(os.path.dirname(__file__), 'examples', 'scheduler_inputs') - self.scheduler = setup_json_scheduler(build_root, self._native) - - self.java = Address.parse('src/java/simple') - - def request(self, products, *addresses): - return self.scheduler.execution_request(products, addresses) - - def test_serial_execution_simple(self): - request = self.request([Classpath], self.java) - result = self.scheduler.execute(request) - with temporary_dir() as tempdir: - self.scheduler.visualize_graph_to_file(os.path.join(tempdir, 'run.0.dot')) - self.assertEqual(Return(Classpath(creator='javac')), result.root_products[0][1]) - self.assertIsNone(result.error) - - def test_product_request_return(self): - count = 0 - for computed_product in self.scheduler.product_request(Classpath, [self.java]): - self.assertIsInstance(computed_product, Classpath) - count += 1 - self.assertGreater(count, 0) +from pants_test.engine.util import assert_equal_with_printing, remove_locations_from_traceback class A(object): diff --git a/tests/python/pants_test/engine/test_graph.py b/tests/python/pants_test/engine/test_graph.py deleted file mode 100644 index 27a3b09b3e3..00000000000 --- a/tests/python/pants_test/engine/test_graph.py +++ /dev/null @@ -1,161 +0,0 @@ -# coding=utf-8 -# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). -# Licensed under the Apache License, Version 2.0 (see LICENSE). - -from __future__ import absolute_import, division, print_function, unicode_literals - -import functools -import unittest -from builtins import range - -from pants.engine.nodes import Return - - -_WAITING = 'TODO: Waiting' - - -@unittest.skip('Skipped to expedite landing #3821; see: #4027.') -class GraphTest(unittest.TestCase): - - def setUp(self): - super(GraphTest, self).setUp() - self.pg = 'TODO: These tests need to be ported to native tests.' - - @classmethod - def _mk_chain(cls, graph, sequence, states=[_WAITING, Return]): - """Create a chain of dependencies (e.g. 'A'->'B'->'C'->'D') in the graph from a sequence.""" - for state in states: - dest = None - for src in reversed(sequence): - if state is _WAITING: - graph.add_dependencies(src, [dest] if dest else []) - else: - graph.complete_node(src, state([dest])) - dest = src - return sequence - - def test_disallow_completed_state_change(self): - self.pg.complete_node('A', Return('done!')) - with self.assertRaises('TODO: CompletedNodeException: These tests should be ported to native tests.'): - self.pg.add_dependencies('A', ['B']) - - def test_disallow_completing_with_incomplete_deps(self): - self.pg.add_dependencies('A', ['B']) - self.pg.add_dependencies('B', ['C']) - with self.assertRaises('TODO: IncompleteDependencyException: These tests should be ported to native tests.'): - self.pg.complete_node('A', Return('done!')) - - def test_dependency_edges(self): - self.pg.add_dependencies('A', ['B', 'C']) - self.assertEqual({'B', 'C'}, set(self.pg.dependencies_of('A'))) - self.assertEqual({'A'}, set(self.pg.dependents_of('B'))) - self.assertEqual({'A'}, set(self.pg.dependents_of('C'))) - - def test_cycle_simple(self): - self.pg.add_dependencies('A', ['B']) - self.pg.add_dependencies('B', ['A']) - # NB: Order matters: the second insertion is the one tracked as a cycle. - self.assertEqual({'B'}, set(self.pg.dependencies_of('A'))) - self.assertEqual(set(), set(self.pg.dependencies_of('B'))) - self.assertEqual(set(), set(self.pg.cyclic_dependencies_of('A'))) - self.assertEqual({'A'}, set(self.pg.cyclic_dependencies_of('B'))) - - def test_cycle_indirect(self): - self.pg.add_dependencies('A', ['B']) - self.pg.add_dependencies('B', ['C']) - self.pg.add_dependencies('C', ['A']) - - self.assertEqual({'B'}, set(self.pg.dependencies_of('A'))) - self.assertEqual({'C'}, set(self.pg.dependencies_of('B'))) - self.assertEqual(set(), set(self.pg.dependencies_of('C'))) - self.assertEqual(set(), set(self.pg.cyclic_dependencies_of('A'))) - self.assertEqual(set(), set(self.pg.cyclic_dependencies_of('B'))) - self.assertEqual({'A'}, set(self.pg.cyclic_dependencies_of('C'))) - - def test_cycle_long(self): - # Creating a long chain is allowed. - nodes = list(range(0, 100)) - self._mk_chain(self.pg, nodes, states=(_WAITING,)) - walked_nodes = [node for node, _ in self.pg.walk([nodes[0]])] - self.assertEqual(nodes, walked_nodes) - - # Closing the chain is not. - begin, end = nodes[0], nodes[-1] - self.pg.add_dependencies(end, [begin]) - self.assertEqual(set(), set(self.pg.dependencies_of(end))) - self.assertEqual({begin}, set(self.pg.cyclic_dependencies_of(end))) - - def test_walk(self): - nodes = list('ABCDEF') - self._mk_chain(self.pg, nodes) - walked_nodes = list((node for node, _ in self.pg.walk(nodes[0]))) - self.assertEqual(nodes, walked_nodes) - - def test_invalidate_all(self): - chain_list = list('ABCDEFGHIJKLMNOPQRSTUVWXYZ') - invalidators = ( - self.pg.invalidate, - functools.partial(self.pg.invalidate, lambda node, _: node == 'Z') - ) - - for invalidator in invalidators: - self._mk_chain(self.pg, chain_list) - - self.assertTrue(self.pg.completed_nodes()) - self.assertTrue(self.pg.dependents()) - self.assertTrue(self.pg.dependencies()) - self.assertTrue(self.pg.cyclic_dependencies()) - - invalidator() - - self.assertFalse(self.pg._nodes) - - def test_invalidate_partial(self): - comparison_pg = 'TODO: These tests need to be ported to native tests.' - chain_a = list('ABCDEF') - chain_b = list('GHIJKL') - - # Add two dependency chains to the primary graph. - self._mk_chain(self.pg, chain_a) - self._mk_chain(self.pg, chain_b) - - # Add only the dependency chain we won't invalidate to the comparison graph. - self._mk_chain(comparison_pg, chain_b) - - # Invalidate one of the chains in the primary graph from the right-most node. - self.pg.invalidate(lambda node, _: node == chain_a[-1]) - - # Ensure the final structure of the primary graph matches the comparison graph. - pg_structure = {n: e.structure() for n, e in self.pg._nodes.items()} - comparison_structure = {n: e.structure() for n, e in comparison_pg._nodes.items()} - self.assertEqual(pg_structure, comparison_structure) - - def test_invalidate_count(self): - self._mk_chain(self.pg, list('ABCDEFGHIJKLMNOPQRSTUVWXYZ')) - invalidated_count = self.pg.invalidate(lambda node, _: node == 'I') - self.assertEqual(invalidated_count, 9) - - def test_invalidate_partial_identity_check(self): - # Create a graph with a chain from A..Z. - chain = self._mk_chain(self.pg, list('ABCDEFGHIJKLMNOPQRSTUVWXYZ')) - self.assertTrue(list(self.pg.completed_nodes())) - - # Track the pre-invaliation nodes (from A..Q). - index_of_q = chain.index('Q') - before_nodes = [node for node, _ in self.pg.completed_nodes() if node in chain[:index_of_q + 1]] - self.assertTrue(before_nodes) - - # Invalidate all nodes under Q. - self.pg.invalidate(lambda node, _: node == chain[index_of_q]) - self.assertTrue(list(self.pg.completed_nodes())) - - # Check that the root node and all fs nodes were removed via a identity checks. - for node, entry in self.pg._nodes.items(): - self.assertFalse(node in before_nodes, 'node:\n{}\nwasnt properly removed'.format(node)) - - for associated in (entry.dependencies, entry.dependents, entry.cyclic_dependencies): - for associated_entry in associated: - self.assertFalse( - associated_entry.node in before_nodes, - 'node:\n{}\nis still associated with:\n{}\nin {}'.format(node, associated_entry.node, entry) - ) diff --git a/tests/python/pants_test/engine/test_rules.py b/tests/python/pants_test/engine/test_rules.py index 026dde5420f..2c4e4507355 100644 --- a/tests/python/pants_test/engine/test_rules.py +++ b/tests/python/pants_test/engine/test_rules.py @@ -15,17 +15,9 @@ from pants.engine.selectors import Get, Select from pants.util.objects import Exactly from pants_test.engine.examples.parsers import JsonParser -from pants_test.engine.examples.planners import Goal from pants_test.engine.util import TargetTable, assert_equal_with_printing, create_scheduler -class AGoal(Goal): - - @classmethod - def products(cls): - return [A] - - class A(object): def __repr__(self): diff --git a/tests/python/pants_test/engine/test_scheduler.py b/tests/python/pants_test/engine/test_scheduler.py index 5b9e07d4cc0..8b29666bfab 100644 --- a/tests/python/pants_test/engine/test_scheduler.py +++ b/tests/python/pants_test/engine/test_scheduler.py @@ -4,242 +4,16 @@ from __future__ import absolute_import, division, print_function, unicode_literals -import os import unittest -from builtins import object, open, str +from builtins import object from textwrap import dedent -from pants.base.cmd_line_spec_parser import CmdLineSpecParser -from pants.base.specs import Specs -from pants.build_graph.address import Address -from pants.engine.addressable import BuildFileAddresses -from pants.engine.nodes import Return, Throw from pants.engine.rules import RootRule, TaskRule -from pants.engine.selectors import Select, SelectVariant -from pants.util.contextutil import temporary_dir -from pants_test.engine.examples.planners import (ApacheThriftJavaConfiguration, Classpath, GenGoal, - Jar, ThriftSources, setup_json_scheduler) -from pants_test.engine.util import (assert_equal_with_printing, create_scheduler, init_native, +from pants.engine.selectors import Select +from pants_test.engine.util import (assert_equal_with_printing, create_scheduler, remove_locations_from_traceback) -walk = "TODO: Should port tests that attempt to inspect graph internals to the native code." - - -class SchedulerTest(unittest.TestCase): - - _native = init_native() - - def setUp(self): - build_root = os.path.join(os.path.dirname(__file__), 'examples', 'scheduler_inputs') - self.spec_parser = CmdLineSpecParser(build_root) - self.scheduler = setup_json_scheduler(build_root, self._native) - - self.guava = Address.parse('3rdparty/jvm:guava') - self.thrift = Address.parse('src/thrift/codegen/simple') - self.java = Address.parse('src/java/codegen/simple') - self.java_simple = Address.parse('src/java/simple') - self.java_multi = Address.parse('src/java/multiple_classpath_entries') - self.no_variant_thrift = Address.parse('src/java/codegen/selector:conflict') - self.unconfigured_thrift = Address.parse('src/thrift/codegen/unconfigured') - self.resources = Address.parse('src/resources/simple') - self.consumes_resources = Address.parse('src/java/consumes_resources') - self.consumes_managed_thirdparty = Address.parse('src/java/managed_thirdparty') - self.managed_guava = Address.parse('3rdparty/jvm/managed:guava') - self.managed_hadoop = Address.parse('3rdparty/jvm/managed:hadoop-common') - self.managed_resolve_latest = Address.parse('3rdparty/jvm/managed:latest-hadoop') - self.inferred_deps = Address.parse('src/scala/inferred_deps') - - def tearDown(self): - super(SchedulerTest, self).tearDown() - # Without eagerly dropping this reference, each instance created for a test method - # will live until all tests in this class have completed: can confirm by editing - # the `scheduler_destroy` call in `src/python/pants/engine/native.py`. - self.scheduler = None - - def parse_specs(self, *specs): - return Specs(tuple(self.spec_parser.parse_spec(spec) for spec in specs)) - - def assert_select_for_subjects(self, walk, selector, subjects, variants=None): - raise ValueError(walk) - - def build(self, execution_request): - """Execute the given request and return roots as a list of ((subject, product), value) tuples.""" - result = self.scheduler.execute(execution_request) - self.assertIsNone(result.error) - return result.root_products - - def request(self, products, *subjects): - return self.scheduler.execution_request(products, subjects) - - def assert_root(self, root, subject, return_value): - """Asserts that the given root has the given result.""" - self.assertEqual(subject, root[0][0]) - self.assertEqual(Return(return_value), root[1]) - - def assert_root_failed(self, root, subject, msg_str): - """Asserts that the root was a Throw result containing the given msg string.""" - self.assertEqual(subject, root[0][0]) - self.assertEqual(Throw, type(root[1])) - self.assertIn(msg_str, str(root[1].exc)) - - def test_compile_only_3rdparty(self): - build_request = self.request([Classpath], self.guava) - root, = self.build(build_request) - self.assert_root(root, self.guava, Classpath(creator='ivy_resolve')) - - @unittest.skip('Skipped to expedite landing #3821; see: #4027.') - def test_compile_only_3rdparty_internal(self): - build_request = self.request([Classpath], '3rdparty/jvm:guava') - root, = self.build(build_request) - - # Expect a SelectNode for each of the Jar/Classpath. - self.assert_select_for_subjects(walk, Select(Jar), [self.guava]) - self.assert_select_for_subjects(walk, Select(Classpath), [self.guava]) - - @unittest.skip('Skipped to expedite landing #3821; see: #4020.') - def test_gen(self): - build_request = self.request([GenGoal], self.thrift) - root, = self.build(build_request) - - # Root: expect the synthetic GenGoal product. - self.assert_root(root, self.thrift, GenGoal("non-empty input to satisfy the Goal constructor")) - - variants = {'thrift': 'apache_java'} - # Expect ThriftSources to have been selected. - self.assert_select_for_subjects(walk, Select(ThriftSources), [self.thrift], variants=variants) - # Expect an ApacheThriftJavaConfiguration to have been used via the default Variants. - self.assert_select_for_subjects(walk, SelectVariant(ApacheThriftJavaConfiguration, - variant_key='thrift'), - [self.thrift], - variants=variants) - - @unittest.skip('Skipped to expedite landing #3821; see: #4020.') - def test_codegen_simple(self): - build_request = self.request([Classpath], self.java) - root, = self.build(build_request) - - # The subgraph below 'src/thrift/codegen/simple' will be affected by its default variants. - subjects = [self.guava, self.java, self.thrift] - variant_subjects = [ - Jar(org='org.apache.thrift', name='libthrift', rev='0.9.2', type_alias='jar'), - Jar(org='commons-lang', name='commons-lang', rev='2.5', type_alias='jar'), - Address.parse('src/thrift:slf4j-api')] - - # Root: expect a DependenciesNode depending on a SelectNode with compilation via javac. - self.assert_root(root, self.java, Classpath(creator='javac')) - - # Confirm that exactly the expected subjects got Classpaths. - self.assert_select_for_subjects(walk, Select(Classpath), subjects) - self.assert_select_for_subjects(walk, Select(Classpath), variant_subjects, - variants={'thrift': 'apache_java'}) - - def test_consumes_resources(self): - build_request = self.request([Classpath], self.consumes_resources) - root, = self.build(build_request) - self.assert_root(root, self.consumes_resources, Classpath(creator='javac')) - - @unittest.skip('Skipped to expedite landing #3821; see: #4027.') - def test_consumes_resources_internal(self): - build_request = self.request([Classpath], self.consumes_resources) - root, = self.build(build_request) - - # Confirm a classpath for the resources target and other subjects. We know that they are - # reachable from the root (since it was involved in this walk). - subjects = [self.resources, - self.consumes_resources, - self.guava] - self.assert_select_for_subjects(walk, Select(Classpath), subjects) - - @unittest.skip('Skipped to expedite landing #3821; see: #4020.') - def test_managed_resolve(self): - """A managed resolve should consume a ManagedResolve and ManagedJars to produce Jars.""" - build_request = self.request([Classpath], self.consumes_managed_thirdparty) - root, = self.build(build_request) - - # Validate the root. - self.assert_root(root, self.consumes_managed_thirdparty, Classpath(creator='javac')) - - # Confirm that we produced classpaths for the managed jars. - managed_jars = [self.managed_guava, self.managed_hadoop] - self.assert_select_for_subjects(walk, Select(Classpath), [self.consumes_managed_thirdparty]) - self.assert_select_for_subjects(walk, Select(Classpath), managed_jars, - variants={'resolve': 'latest-hadoop'}) - - # Confirm that the produced jars had the appropriate versions. - self.assertEqual({Jar('org.apache.hadoop', 'hadoop-common', '2.7.0'), - Jar('com.google.guava', 'guava', '18.0')}, - {ret.value for node, ret in walk - if node.product == Jar}) - - def test_dependency_inference(self): - """Scala dependency inference introduces dependencies that do not exist in BUILD files.""" - build_request = self.request([Classpath], self.inferred_deps) - root, = self.build(build_request) - self.assert_root(root, self.inferred_deps, Classpath(creator='scalac')) - - @unittest.skip('Skipped to expedite landing #3821; see: #4027.') - def test_dependency_inference_internal(self): - """Scala dependency inference introduces dependencies that do not exist in BUILD files.""" - build_request = self.request([Classpath], self.inferred_deps) - root, = self.build(build_request) - - # Confirm that we requested a classpath for the root and inferred targets. - self.assert_select_for_subjects(walk, Select(Classpath), [self.inferred_deps, self.java_simple]) - - def test_multiple_classpath_entries(self): - """Multiple Classpath products for a single subject currently cause a failure.""" - build_request = self.request([Classpath], self.java_multi) - root, = self.build(build_request) - - # Validate that the root failed. - self.assert_root_failed(root, self.java_multi, "Conflicting values produced for") - - def test_descendant_specs(self): - """Test that Addresses are produced via recursive globs of the 3rdparty/jvm directory.""" - specs = self.parse_specs('3rdparty/jvm::') - build_request = self.scheduler.execution_request([BuildFileAddresses], [specs]) - ((subject, _), root), = self.build(build_request) - - # Validate the root. - self.assertEqual(specs, subject) - self.assertEqual(BuildFileAddresses, type(root.value)) - - # Confirm that a few expected addresses are in the list. - self.assertIn(self.guava, root.value.dependencies) - self.assertIn(self.managed_guava, root.value.dependencies) - self.assertIn(self.managed_resolve_latest, root.value.dependencies) - - def test_sibling_specs(self): - """Test that sibling Addresses are parsed in the 3rdparty/jvm directory.""" - specs = self.parse_specs('3rdparty/jvm:') - build_request = self.scheduler.execution_request([BuildFileAddresses], [specs]) - ((subject, _), root), = self.build(build_request) - - # Validate the root. - self.assertEqual(specs, subject) - self.assertEqual(BuildFileAddresses, type(root.value)) - - # Confirm that an expected address is in the list. - self.assertIn(self.guava, root.value.dependencies) - # And that a subdirectory address is not. - self.assertNotIn(self.managed_guava, root.value.dependencies) - - def test_scheduler_visualize(self): - specs = self.parse_specs('3rdparty/jvm::') - build_request = self.request([BuildFileAddresses], specs) - self.build(build_request) - - with temporary_dir() as td: - output_path = os.path.join(td, 'output.dot') - self.scheduler.visualize_graph_to_file(output_path) - with open(output_path, 'r') as fh: - graphviz_output = fh.read().strip() - - self.assertIn('digraph', graphviz_output) - self.assertIn(' -> ', graphviz_output) - - class A(object): pass From 12546e538e83cfb0667b06c57c165e3b0616f2bf Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Sun, 9 Sep 2018 17:15:04 -0700 Subject: [PATCH 07/13] Review feedback (and remove SelectVariant, which was missed the first time round). --- src/python/pants/engine/native.py | 1 - src/python/pants/engine/scheduler.py | 7 +---- src/python/pants/engine/selectors.py | 22 ------------- src/rust/engine/src/lib.rs | 16 +--------- src/rust/engine/src/nodes.rs | 8 ++--- src/rust/engine/src/rule_graph.rs | 31 +++++++------------ src/rust/engine/src/scheduler.rs | 2 +- src/rust/engine/src/selectors.rs | 8 ++--- src/rust/engine/src/tasks.rs | 7 ++--- .../pants_test/engine/test_selectors.py | 12 +------ 10 files changed, 23 insertions(+), 91 deletions(-) diff --git a/src/python/pants/engine/native.py b/src/python/pants/engine/native.py index b6f6fe247f4..9b4d2be469f 100644 --- a/src/python/pants/engine/native.py +++ b/src/python/pants/engine/native.py @@ -171,7 +171,6 @@ void tasks_task_begin(Tasks*, Function, TypeConstraint); void tasks_add_get(Tasks*, TypeConstraint, TypeId); void tasks_add_select(Tasks*, TypeConstraint); -void tasks_add_select_variant(Tasks*, TypeConstraint, Buffer); void tasks_task_end(Tasks*); void tasks_singleton_add(Tasks*, Handle, TypeConstraint); void tasks_destroy(Tasks*); diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index 8e1c6a69782..8f452f10b91 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -20,7 +20,7 @@ from pants.engine.native import Function, TypeConstraint, TypeId from pants.engine.nodes import Return, State, Throw from pants.engine.rules import RuleIndex, SingletonRule, TaskRule -from pants.engine.selectors import Select, SelectVariant, constraint_for +from pants.engine.selectors import Select, constraint_for from pants.engine.struct import HasProducts, Variants from pants.util.contextutil import temporary_file_path from pants.util.dirutil import check_no_overlapping_paths @@ -237,11 +237,6 @@ def _register_task(self, output_constraint, rule): product_constraint = self._to_constraint(selector.product) if selector_type is Select: self._native.lib.tasks_add_select(self._tasks, product_constraint) - elif selector_type is SelectVariant: - key_buf = self._to_utf8_buf(selector.variant_key) - self._native.lib.tasks_add_select_variant(self._tasks, - product_constraint, - key_buf) else: raise ValueError('Unrecognized Selector type: {}'.format(selector)) for get in rule.input_gets: diff --git a/src/python/pants/engine/selectors.py b/src/python/pants/engine/selectors.py index d46e4b88a1a..037120ec350 100644 --- a/src/python/pants/engine/selectors.py +++ b/src/python/pants/engine/selectors.py @@ -8,8 +8,6 @@ from abc import abstractproperty from builtins import str -import six - from pants.util.meta import AbstractClass from pants.util.objects import Exactly, datatype @@ -110,23 +108,3 @@ def __repr__(self): return '{}({}{})'.format(type(self).__name__, type_or_constraint_repr(self.product), ', optional=True' if self.optional else '') - - -class SelectVariant(datatype(['product', 'variant_key']), Selector): - """Selects the matching Product and variant name for the Subject provided to the constructor. - - For example: a SelectVariant with a variant_key of "thrift" and a product of type ApacheThrift - will only match when a consumer passes a variant value for "thrift" that matches the name of an - ApacheThrift value. - """ - optional = False - - def __new__(cls, product, variant_key): - if not isinstance(variant_key, six.string_types): - raise ValueError('Expected variant_key to be a string, but was {!r}'.format(variant_key)) - return super(SelectVariant, cls).__new__(cls, product, variant_key) - - def __repr__(self): - return '{}({}, {})'.format(type(self).__name__, - type_or_constraint_repr(self.product), - repr(self.variant_key)) diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 47cc1abee84..397d04d9382 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -432,21 +432,7 @@ pub extern "C" fn tasks_add_get(tasks_ptr: *mut Tasks, product: TypeConstraint, #[no_mangle] pub extern "C" fn tasks_add_select(tasks_ptr: *mut Tasks, product: TypeConstraint) { with_tasks(tasks_ptr, |tasks| { - tasks.add_select(product, None); - }) -} - -#[no_mangle] -pub extern "C" fn tasks_add_select_variant( - tasks_ptr: *mut Tasks, - product: TypeConstraint, - variant_key_buf: Buffer, -) { - let variant_key = variant_key_buf - .to_string() - .expect("Failed to decode key for select_variant"); - with_tasks(tasks_ptr, |tasks| { - tasks.add_select(product, Some(variant_key)); + tasks.add_select(product); }) } diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index c19f9ad5718..56d19b6ae31 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -93,7 +93,7 @@ pub struct Select { impl Select { pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select { - Self::new_with_selector(selectors::Select::without_variant(product), params, edges) + Self::new_with_selector(selectors::Select::new(product), params, edges) } pub fn new_with_entries( @@ -101,7 +101,7 @@ impl Select { params: Params, entry: rule_graph::Entry, ) -> Select { - let selector = selectors::Select::without_variant(product); + let selector = selectors::Select::new(product); Select { selector, params, @@ -685,8 +685,8 @@ impl Task { .into_iter() .map(|externs::Get(product, subject)| { // TODO: The subject of the get is a new parameter, but params from the context should be - // included as well. - // TODO: Parameters should be filtered to what is used by the Entry. + // included as well. Additionally, params should be filtered to what is used by the Entry. + // see https://github.com/pantsbuild/pants/issues/6478 let params = Params::new(vec![subject]); let select_key = rule_graph::SelectKey::JustGet(selectors::Get { product: product, diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index 881c35aea1b..bf04fb127b0 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -24,7 +24,7 @@ impl UnreachableError { UnreachableError { task_rule: task_rule, diagnostic: Diagnostic { - params: Default::default(), + params: ParamTypes::default(), reason: "Unreachable".to_string(), details: vec![], }, @@ -81,7 +81,7 @@ impl EntryWithDeps { &EntryWithDeps::Inner(InnerEntry { rule: Rule::Intrinsic(Intrinsic { ref input, .. }), .. - }) => vec![SelectKey::JustSelect(Select::without_variant(*input))], + }) => vec![SelectKey::JustSelect(Select::new(*input))], } } @@ -196,7 +196,7 @@ enum ConstructGraphResult { // The Entry was satisfiable without waiting for any additional nodes to be satisfied. The result // contains copies of the input Entry for each set subset of the parameters that satisfy it. Fulfilled(Vec), - // The Entry was not satifiable with installed rules. + // The Entry was not satisfiable with installed rules. Unfulfillable, // The dependencies of an Entry might be satisfiable, but is currently blocked waiting for the // results of the given entries. @@ -386,6 +386,7 @@ impl<'t> GraphMaker<'t> { let (params, product) = match &select_key { &SelectKey::JustSelect(ref s) => (entry.params().clone(), s.product.clone()), &SelectKey::JustGet(ref g) => { + // Unlike Selects, Gets introduce new parameter values into a subgraph. let get_params = { let mut p = entry.params().clone(); p.insert(g.subject.clone()); @@ -495,10 +496,7 @@ impl<'t> GraphMaker<'t> { return Ok(ConstructGraphResult::Unfulfillable); } }; - let simplified_entries_only = simplified_entries - .iter() - .map(|(entry, _)| entry.clone()) - .collect(); + let simplified_entries_only = simplified_entries.keys().cloned().collect(); if !cycled_on.is_empty() { // The set of cycled dependencies can only contain call stack "parents" of the dependency: we @@ -664,8 +662,6 @@ impl<'t> GraphMaker<'t> { // the non-ambiguous rule with the smallest set of Params, as that minimizes Node identities // in the graph and biases toward receiving values from dependencies (which do not affect our // identity) rather than dependents. - // - // TODO: There has to be a more efficient way to group by Entry. let mut rules_by_kind: HashMap = HashMap::new(); for satisfiable_entry in satisfiable_entries.iter() { match satisfiable_entry { @@ -673,9 +669,6 @@ impl<'t> GraphMaker<'t> { rules_by_kind .entry(wd.simplified(BTreeSet::new())) .and_modify(|e| { - // TODO: Param set size isn't sufficient to fully differentiate alternatives: would - // prefer a larger param set to a smaller one. - // TODO2: This might be redundant now that we are matching subsets in monomorphize. if e.0 > wd.params().len() { *e = (wd.params().len(), satisfiable_entry); } @@ -752,10 +745,7 @@ impl<'t> GraphMaker<'t> { } else { Some(RootEntry { params: param_types.clone(), - clause: vec![Select { - product: *product_type, - variant_key: None, - }], + clause: vec![Select::new(*product_type)], gets: vec![], }) } @@ -923,14 +913,15 @@ fn task_display(task: &Task) -> String { } impl RuleGraph { - pub fn new(tasks: &Tasks, root_subject_types: Vec) -> RuleGraph { - GraphMaker::new(tasks, root_subject_types).full_graph() + pub fn new(tasks: &Tasks, root_param_types: Vec) -> RuleGraph { + GraphMaker::new(tasks, root_param_types).full_graph() } - pub fn find_root_edges(&self, subject_type: TypeId, select: Select) -> Option { + pub fn find_root_edges(&self, param_type: TypeId, select: Select) -> Option { // TODO: Support more than one root parameter... needs some API work. + // see https://github.com/pantsbuild/pants/issues/6478 let root = RootEntry { - params: vec![subject_type].into_iter().collect(), + params: vec![param_type].into_iter().collect(), clause: vec![select], gets: vec![], }; diff --git a/src/rust/engine/src/scheduler.rs b/src/rust/engine/src/scheduler.rs index e84c8d0c998..88f1ed78c3f 100644 --- a/src/rust/engine/src/scheduler.rs +++ b/src/rust/engine/src/scheduler.rs @@ -107,7 +107,7 @@ impl Scheduler { ) -> Result<(), String> { let edges = self.find_root_edges_or_update_rule_graph( subject.type_id().clone(), - &selectors::Select::without_variant(product), + &selectors::Select::new(product), )?; request .roots diff --git a/src/rust/engine/src/selectors.rs b/src/rust/engine/src/selectors.rs index 2d765acd715..bd9e4aca3d9 100644 --- a/src/rust/engine/src/selectors.rs +++ b/src/rust/engine/src/selectors.rs @@ -12,14 +12,10 @@ pub struct Get { #[derive(Clone, Debug, Eq, Hash, PartialEq)] pub struct Select { pub product: TypeConstraint, - pub variant_key: Option, } impl Select { - pub fn without_variant(product: TypeConstraint) -> Select { - Select { - product: product, - variant_key: None, - } + pub fn new(product: TypeConstraint) -> Select { + Select { product: product } } } diff --git a/src/rust/engine/src/tasks.rs b/src/rust/engine/src/tasks.rs index 69f9fc61111..f08bf629e86 100644 --- a/src/rust/engine/src/tasks.rs +++ b/src/rust/engine/src/tasks.rs @@ -151,16 +151,13 @@ impl Tasks { }); } - pub fn add_select(&mut self, product: TypeConstraint, variant_key: Option) { + pub fn add_select(&mut self, product: TypeConstraint) { self .preparing .as_mut() .expect("Must `begin()` a task creation before adding clauses!") .clause - .push(Select { - product: product, - variant_key: variant_key, - }); + .push(Select::new(product)); } pub fn task_end(&mut self) { diff --git a/tests/python/pants_test/engine/test_selectors.py b/tests/python/pants_test/engine/test_selectors.py index d2115079bf6..b24dd4c96cd 100644 --- a/tests/python/pants_test/engine/test_selectors.py +++ b/tests/python/pants_test/engine/test_selectors.py @@ -7,9 +7,7 @@ import unittest from builtins import object -from future.utils import PY3 - -from pants.engine.selectors import Select, SelectVariant +from pants.engine.selectors import Select class AClass(object): @@ -21,13 +19,5 @@ def test_select_repr(self): self.assert_repr("Select(AClass)", Select(AClass)) self.assert_repr("Select(AClass, optional=True)", Select(AClass, optional=True)) - def test_variant_repr(self): - self.assert_repr("SelectVariant(AClass, {unicode_literal}'field')".format(unicode_literal='' if PY3 else 'u'), - SelectVariant(AClass, 'field')) - def assert_repr(self, expected, selector): self.assertEqual(expected, repr(selector)) - - def test_select_variant_requires_string_key(self): - with self.assertRaises(ValueError): - SelectVariant(AClass, None) From 69143fecf7804d2783c3be42705bb2db182e4607 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 12 Sep 2018 20:05:38 -0700 Subject: [PATCH 08/13] Additional review feedback. --- src/rust/engine/src/core.rs | 10 +++++----- src/rust/engine/src/nodes.rs | 2 +- src/rust/engine/src/rule_graph.rs | 10 +++++----- src/rust/engine/src/scheduler.rs | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index da4d0c1dd79..3d77424fd85 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -18,21 +18,21 @@ pub type FNV = hash::BuildHasherDefault; /// For efficiency and hashability, they're stored as sorted Keys (with distinct TypeIds), and /// wrapped in an `Arc` that allows us to copy-on-write for param contents. /// +/// TODO: Consider replacing the Arc> with a [smallvec](https://crates.io/crates/smallvec). +/// #[repr(C)] #[derive(Clone, Debug, Default, Eq, Hash, PartialEq)] pub struct Params(Arc>); impl Params { - pub fn new(mut params: Vec) -> Params { - params.sort_by(|l, r| l.type_id().cmp(r.type_id())); - params.dedup_by(|l, r| l.type_id() == r.type_id()); - Params(Arc::new(params)) + pub fn new_single(param: Key) -> Params { + Params(Arc::new(vec![param])) } /// /// TODO: This is a compatibility API to assist in the transition from "every Node has exactly /// one Subject" to "every Node has zero or more Params". See: - /// https://github.com/pantsbuild/pants/issues/5788 + /// https://github.com/pantsbuild/pants/issues/6478 /// pub fn expect_single(&self) -> &Key { if self.0.len() != 1 { diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 56d19b6ae31..16ab7fc8ee6 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -687,7 +687,7 @@ impl Task { // TODO: The subject of the get is a new parameter, but params from the context should be // included as well. Additionally, params should be filtered to what is used by the Entry. // see https://github.com/pantsbuild/pants/issues/6478 - let params = Params::new(vec![subject]); + let params = Params::new_single(subject); let select_key = rule_graph::SelectKey::JustGet(selectors::Get { product: product, subject: *subject.type_id(), diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index bf04fb127b0..507b7f1c50b 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -219,17 +219,17 @@ pub struct GraphMaker<'t> { } impl<'t> GraphMaker<'t> { - pub fn new(tasks: &'t Tasks, root_subject_types: Vec) -> GraphMaker<'t> { - let root_param_types = root_subject_types.into_iter().collect(); + pub fn new(tasks: &'t Tasks, root_param_types: Vec) -> GraphMaker<'t> { + let root_param_types = root_param_types.into_iter().collect(); GraphMaker { tasks, root_param_types, } } - pub fn sub_graph(&self, subject_type: &TypeId, product_type: &TypeConstraint) -> RuleGraph { + pub fn sub_graph(&self, param_type: &TypeId, product_type: &TypeConstraint) -> RuleGraph { // TODO: Update to support rendering a subgraph given a set of ParamTypes. - let param_types = vec![*subject_type].into_iter().collect(); + let param_types = vec![*param_type].into_iter().collect(); if let Some(beginning_root) = self.gen_root_entry(¶m_types, product_type) { self.construct_graph(vec![beginning_root]) @@ -625,7 +625,7 @@ impl<'t> GraphMaker<'t> { return Ok(None); } 1 => { - combination.push((key, *chosen_entries.last().unwrap())); + combination.push((key, chosen_entries[0])); } _ => { return Err(Diagnostic::ambiguous(available_params, key, chosen_entries)); diff --git a/src/rust/engine/src/scheduler.rs b/src/rust/engine/src/scheduler.rs index 88f1ed78c3f..c174680ee35 100644 --- a/src/rust/engine/src/scheduler.rs +++ b/src/rust/engine/src/scheduler.rs @@ -111,7 +111,7 @@ impl Scheduler { )?; request .roots - .push(Select::new(product, Params::new(vec![subject]), &edges)); + .push(Select::new(product, Params::new_single(subject), &edges)); Ok(()) } From 7d8a7fe4b3beb8a96d17102284fa07191580fee5 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Fri, 14 Sep 2018 15:04:00 -0700 Subject: [PATCH 09/13] Fix clippy lints --- src/rust/engine/src/lib.rs | 2 +- src/rust/engine/src/rule_graph.rs | 98 +++++++++++++++---------------- 2 files changed, 50 insertions(+), 50 deletions(-) diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 397d04d9382..5209646bf60 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -734,7 +734,7 @@ fn graph_sub( product_type: TypeConstraint, ) -> RuleGraph { let graph_maker = GraphMaker::new(&scheduler.core.tasks, vec![subject_type]); - graph_maker.sub_graph(&subject_type, &product_type) + graph_maker.sub_graph(subject_type, &product_type) } fn write_to_file(path: &Path, graph: &RuleGraph) -> io::Result<()> { diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index 507b7f1c50b..a46fecae4c4 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -227,9 +227,9 @@ impl<'t> GraphMaker<'t> { } } - pub fn sub_graph(&self, param_type: &TypeId, product_type: &TypeConstraint) -> RuleGraph { + pub fn sub_graph(&self, param_type: TypeId, product_type: &TypeConstraint) -> RuleGraph { // TODO: Update to support rendering a subgraph given a set of ParamTypes. - let param_types = vec![*param_type].into_iter().collect(); + let param_types = vec![param_type].into_iter().collect(); if let Some(beginning_root) = self.gen_root_entry(¶m_types, product_type) { self.construct_graph(vec![beginning_root]) @@ -314,7 +314,7 @@ impl<'t> GraphMaker<'t> { if let Some(simplified) = all_simplified_entries.get(&entry) { // A simplified equivalent entry has already been computed, return it. return ConstructGraphResult::Fulfilled(simplified.clone()); - } else if let Some(_) = unfulfillable_rules.get(&entry) { + } else if unfulfillable_rules.get(&entry).is_some() { // The rule is unfulfillable. return ConstructGraphResult::Unfulfillable; } @@ -336,10 +336,11 @@ impl<'t> GraphMaker<'t> { // been computed (or we would have returned it above). The cyclic parent(s) will complete // before recursing to compute this node again. let mut cyclic_deps = HashSet::new(); - cyclic_deps.insert(entry.clone()); + let simplified = entry.simplified(BTreeSet::new()); + cyclic_deps.insert(entry); return ConstructGraphResult::CycledOn { cyclic_deps, - partial_simplified_entries: vec![entry.simplified(BTreeSet::new())], + partial_simplified_entries: vec![simplified], }; } }; @@ -384,15 +385,15 @@ impl<'t> GraphMaker<'t> { for select_key in dependency_keys { let (params, product) = match &select_key { - &SelectKey::JustSelect(ref s) => (entry.params().clone(), s.product.clone()), + &SelectKey::JustSelect(ref s) => (entry.params().clone(), s.product), &SelectKey::JustGet(ref g) => { // Unlike Selects, Gets introduce new parameter values into a subgraph. let get_params = { let mut p = entry.params().clone(); - p.insert(g.subject.clone()); + p.insert(g.subject); p }; - (get_params, g.product.clone()) + (get_params, g.product) } }; @@ -407,7 +408,7 @@ impl<'t> GraphMaker<'t> { rule_dependency_edges, all_simplified_entries, unfulfillable_rules, - c.clone(), + c, ) { ConstructGraphResult::Unfulfillable => {} ConstructGraphResult::Fulfilled(simplified_entries) => { @@ -445,13 +446,15 @@ impl<'t> GraphMaker<'t> { // If any candidate triggered a cycle on a rule that has not yet completed, then we are not // yet fulfillable, and should finish gathering any other cyclic rule dependencies. continue; - } else if fulfillable_candidates.is_empty() { + } + + if fulfillable_candidates.is_empty() { // If no candidates were fulfillable, this rule is not fulfillable. unfulfillable_diagnostics.push(Diagnostic { params: params.clone(), reason: format!( "no rule was available to compute {} with parameter type{} {}", - type_constraint_str(product.clone()), + type_constraint_str(product), if params.len() > 1 { "s" } else { "" }, params_str(¶ms), ), @@ -480,11 +483,11 @@ impl<'t> GraphMaker<'t> { let flattened_fulfillable_candidates_by_key = fulfillable_candidates_by_key .into_iter() .map(|(k, candidate_group)| (k, Itertools::flatten(candidate_group.into_iter()).collect())) - .collect(); + .collect::>(); // Generate one Entry per legal combination of parameters. let simplified_entries = - match Self::monomorphize(&entry, flattened_fulfillable_candidates_by_key) { + match Self::monomorphize(&entry, &flattened_fulfillable_candidates_by_key) { Ok(se) => se, Err(ambiguous_diagnostics) => { // At least one combination of the dependencies was ambiguous. @@ -496,9 +499,17 @@ impl<'t> GraphMaker<'t> { return Ok(ConstructGraphResult::Unfulfillable); } }; - let simplified_entries_only = simplified_entries.keys().cloned().collect(); + let simplified_entries_only: Vec<_> = simplified_entries.keys().cloned().collect(); - if !cycled_on.is_empty() { + if cycled_on.is_empty() { + // All dependencies were fulfillable and none were blocked on cycles. Remove the + // placeholder and store the simplified entries. + rule_dependency_edges.remove(&entry); + rule_dependency_edges.extend(simplified_entries); + + all_simplified_entries.insert(entry, simplified_entries_only.clone()); + Ok(ConstructGraphResult::Fulfilled(simplified_entries_only)) + } else { // The set of cycled dependencies can only contain call stack "parents" of the dependency: we // remove this entry from the set (if we're in it), until the top-most cyclic parent // (represented by an empty set) is the one that re-starts recursion. @@ -510,25 +521,17 @@ impl<'t> GraphMaker<'t> { // // Store our simplified equivalence and then re-execute our dependency discovery. In this // second attempt our cyclic dependencies will use the simplified representation(s) to succeed. - all_simplified_entries.insert(entry.clone(), simplified_entries_only); - return Err(()); + all_simplified_entries.insert(entry, simplified_entries_only); + Err(()) } else { // This rule may be fulfillable, but we can't compute its complete set of dependencies until // parent rule entries complete. Remove our placeholder edges before returning. rule_dependency_edges.remove(&entry); - return Ok(ConstructGraphResult::CycledOn { + Ok(ConstructGraphResult::CycledOn { cyclic_deps: cycled_on, partial_simplified_entries: simplified_entries_only, - }); + }) } - } else { - // All dependencies were fulfillable and none were blocked on cycles. Remove the - // placeholder and store the simplified entries. - rule_dependency_edges.remove(&entry); - rule_dependency_edges.extend(simplified_entries); - - all_simplified_entries.insert(entry.clone(), simplified_entries_only.clone()); - return Ok(ConstructGraphResult::Fulfilled(simplified_entries_only)); } } @@ -542,12 +545,12 @@ impl<'t> GraphMaker<'t> { /// fn monomorphize( entry: &EntryWithDeps, - deps: Vec<(SelectKey, Vec)>, + deps: &[(SelectKey, Vec)], ) -> Result, Vec> { // Collect the powerset of the union of used parameters, ordered by set size. let params_powerset: Vec> = { let mut all_used_params = BTreeSet::new(); - for (key, inputs) in &deps { + for (key, inputs) in deps { for input in inputs { all_used_params.extend(Self::used_params(key, input)); } @@ -575,7 +578,7 @@ impl<'t> GraphMaker<'t> { continue; } - match Self::choose_dependencies(&available_params, &deps) { + match Self::choose_dependencies(&available_params, deps) { Ok(Some(inputs)) => { let mut rule_edges = RuleEdges::default(); for (key, input) in inputs { @@ -589,10 +592,10 @@ impl<'t> GraphMaker<'t> { } // If none of the combinations was satisfiable, return the generated diagnostics. - if !combinations.is_empty() { - Ok(combinations) - } else { + if combinations.is_empty() { Err(diagnostics) + } else { + Ok(combinations) } } @@ -606,7 +609,7 @@ impl<'t> GraphMaker<'t> { /// fn choose_dependencies<'a>( available_params: &ParamTypes, - deps: &'a Vec<(SelectKey, Vec)>, + deps: &'a [(SelectKey, Vec)], ) -> Result>, Diagnostic> { let mut combination = Vec::new(); for (key, input_entries) in deps { @@ -664,18 +667,15 @@ impl<'t> GraphMaker<'t> { // identity) rather than dependents. let mut rules_by_kind: HashMap = HashMap::new(); for satisfiable_entry in satisfiable_entries.iter() { - match satisfiable_entry { - &Entry::WithDeps(ref wd) => { - rules_by_kind - .entry(wd.simplified(BTreeSet::new())) - .and_modify(|e| { - if e.0 > wd.params().len() { - *e = (wd.params().len(), satisfiable_entry); - } - }) - .or_insert((wd.params().len(), satisfiable_entry)); - } - _ => {} + if let &Entry::WithDeps(ref wd) = satisfiable_entry { + rules_by_kind + .entry(wd.simplified(BTreeSet::new())) + .and_modify(|e| { + if e.0 > wd.params().len() { + *e = (wd.params().len(), satisfiable_entry); + } + }) + .or_insert((wd.params().len(), satisfiable_entry)); } } @@ -710,7 +710,7 @@ impl<'t> GraphMaker<'t> { } fn powerset<'a, T: Clone>(slice: &'a [T]) -> impl Iterator> + 'a { - (0..(1 << slice.len())).into_iter().map(move |mask| { + (0..(1 << slice.len())).map(move |mask| { let mut ss = Vec::new(); let mut bitset = mask; while bitset > 0 { @@ -806,7 +806,7 @@ pub fn type_str(type_id: TypeId) -> String { pub fn params_str(params: &ParamTypes) -> String { params .iter() - .map(|type_id| type_str(type_id.clone())) + .map(|type_id| type_str(*type_id)) .collect::>() .join("+") } @@ -1002,7 +1002,7 @@ impl RuleGraph { .into_iter() .map(|d| { if d.details.is_empty() { - format!("{}", d.reason) + d.reason.clone() } else { format!("{}:\n {}", d.reason, d.details.join("\n ")) } From 392504dd69fe2c6f71e639d143b41f8b5346fc47 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 18 Sep 2018 18:10:39 -0700 Subject: [PATCH 10/13] Adjust rule formatting and fix rule tests. --- src/python/pants/engine/scheduler.py | 3 - src/rust/engine/src/rule_graph.rs | 45 ++++---- tests/python/pants_test/engine/test_rules.py | 106 ++++++++++--------- 3 files changed, 78 insertions(+), 76 deletions(-) diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index 8f452f10b91..de7e5851991 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -408,9 +408,6 @@ def __init__(self, scheduler, session): self._session = session self._run_count = 0 - def assert_ruleset_valid(self): - self._scheduler._assert_ruleset_valid() - def graph_len(self): return self._scheduler.graph_len() diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index a46fecae4c4..be0cc0faa78 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -804,11 +804,18 @@ pub fn type_str(type_id: TypeId) -> String { } pub fn params_str(params: &ParamTypes) -> String { - params - .iter() - .map(|type_id| type_str(*type_id)) - .collect::>() - .join("+") + match params.len() { + 0 => "()".to_string(), + 1 => type_str(*params.iter().next().unwrap()), + _ => format!( + "({})", + params + .iter() + .map(|type_id| type_str(*type_id)) + .collect::>() + .join("+") + ), + } } fn val_name(val: &Value) -> String { @@ -854,10 +861,7 @@ fn entry_with_deps_str(entry: &EntryWithDeps) -> String { &EntryWithDeps::Inner(InnerEntry { rule: Rule::Task(ref task_rule), ref params, - }) => { - // TODO: `for`, not `of` - format!("{} of {}", task_display(task_rule), params_str(params)) - } + }) => format!("{} for {}", task_display(task_rule), params_str(params)), &EntryWithDeps::Inner(InnerEntry { rule: Rule::Intrinsic(ref intrinsic), ref params, @@ -889,11 +893,7 @@ fn task_display(task: &Task) -> String { .map(|c| select_str(c)) .collect::>() .join(", "); - clause_portion = if task.clause.len() <= 1 { - format!("({},)", clause_portion) - } else { - format!("({})", clause_portion) - }; + clause_portion = format!("[{}]", clause_portion); let mut get_portion = task .gets .iter() @@ -1059,15 +1059,18 @@ impl RuleGraph { .rule_dependency_edges .iter() .filter_map(|(k, deps)| match k { - &EntryWithDeps::Inner(_) => Some(format!( - " \"{}\" -> {{{}}}", - entry_with_deps_str(k), - deps + &EntryWithDeps::Inner(_) => { + let mut deps_strs = deps .all_dependencies() .map(|d| format!("\"{}\"", entry_str(d))) - .collect::>() - .join(" ") - )), + .collect::>(); + deps_strs.sort(); + Some(format!( + " \"{}\" -> {{{}}}", + entry_with_deps_str(k), + deps_strs.join(" ") + )) + } _ => None, }) .collect::>(); diff --git a/tests/python/pants_test/engine/test_rules.py b/tests/python/pants_test/engine/test_rules.py index 2c4e4507355..c473a122130 100644 --- a/tests/python/pants_test/engine/test_rules.py +++ b/tests/python/pants_test/engine/test_rules.py @@ -73,7 +73,7 @@ def test_ruleset_with_missing_product_type(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 1 - (A, (Select(B),), noop): + (A, [Select(B)], noop): no rule was available to compute B with parameter type SubA """).strip(), str(cm.exception)) @@ -85,7 +85,7 @@ def test_ruleset_with_rule_with_two_missing_selects(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 1 - (A, (Select(B), Select(C)), noop): + (A, [Select(B), Select(C)], noop): no rule was available to compute B with parameter type SubA no rule was available to compute C with parameter type SubA """).strip(), @@ -106,9 +106,9 @@ def test_ruleset_with_superclass_of_selected_type_produced_fails(self): create_scheduler(rules) self.assert_equal_with_printing(dedent(""" Rules with errors: 2 - (A, (Select(B),), noop): + (A, [Select(B)], noop): no rule was available to compute B with parameter type C - (B, (Select(SubA),), noop): + (B, [Select(SubA)], noop): no rule was available to compute SubA with parameter type C """).strip(), str(cm.exception)) @@ -133,7 +133,7 @@ def test_ruleset_with_failure_due_to_incompatible_subject_for_singleton(self): # This error message could note near matches like the singleton. self.assert_equal_with_printing(dedent(""" Rules with errors: 1 - (D, (Select(C),), noop): + (D, [Select(C)], noop): no rule was available to compute C with parameter type A """).strip(), str(cm.exception)) @@ -144,7 +144,7 @@ def test_not_fulfillable_duplicated_dependency(self): rules = _suba_root_rules + [ TaskRule(B, [Select(D)], noop), TaskRule(D, [Select(A), Select(SubA)], noop, input_gets=[Get(A, C)]), - TaskRule(A, [Select(SubA)], noop) + TaskRule(A, [Select(C)], noop) ] with self.assertRaises(Exception) as cm: @@ -152,10 +152,10 @@ def test_not_fulfillable_duplicated_dependency(self): self.assert_equal_with_printing(dedent(""" Rules with errors: 2 - (B, (Select(D),), noop): + (B, [Select(D)], noop): no rule was available to compute D with parameter type SubA - (D, (Select(A), Select(SubA)), [Get(A, C)], noop): - no rule was available to compute A with parameter type C + (D, [Select(A), Select(SubA)], [Get(A, C)], noop): + no rule was available to compute A with parameter type SubA """).strip(), str(cm.exception)) @@ -163,7 +163,6 @@ def test_not_fulfillable_duplicated_dependency(self): class RuleGraphMakerTest(unittest.TestCase): - # TODO something with variants # TODO HasProducts? def test_smallest_full_test(self): @@ -178,9 +177,9 @@ def test_smallest_full_test(self): // root subject types: SubA // root entries "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} + "(A, [Select(SubA)], noop) for SubA" -> {"Param(SubA)"} }""").strip(), fullgraph) def test_full_graph_for_planner_example(self): @@ -231,15 +230,15 @@ def test_smallest_full_test_multiple_root_subject_types(self): "Select(A) for A" [color=blue] "Select(A) for A" -> {"Param(A)"} "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} "Select(B) for A" [color=blue] - "Select(B) for A" -> {"(B, (Select(A),), noop) of A"} + "Select(B) for A" -> {"(B, [Select(A)], noop) for A"} "Select(B) for SubA" [color=blue] - "Select(B) for SubA" -> {"(B, (Select(A),), noop) of SubA"} + "Select(B) for SubA" -> {"(B, [Select(A)], noop) for SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} - "(B, (Select(A),), noop) of A" -> {"Param(A)"} - "(B, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "(A, [Select(SubA)], noop) for SubA" -> {"Param(SubA)"} + "(B, [Select(A)], noop) for A" -> {"Param(A)"} + "(B, [Select(A)], noop) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} }""").strip(), fullgraph) @@ -255,9 +254,9 @@ def test_single_rule_depending_on_subject_selection(self): // root subject types: SubA // root entries "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} + "(A, [Select(SubA)], noop) for SubA" -> {"Param(SubA)"} }""").strip(), subgraph) @@ -274,10 +273,10 @@ def test_multiple_selects(self): // root subject types: SubA // root entries "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(SubA), Select(B)), noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(SubA), Select(B)], noop) for SubA"} // internal entries - "(A, (Select(SubA), Select(B)), noop) of SubA" -> {"Param(SubA)" "(B, (,), noop) of SubA"} - "(B, (,), noop) of SubA" -> {} + "(A, [Select(SubA), Select(B)], noop) for SubA" -> {"(B, [], noop) for ()" "Param(SubA)"} + "(B, [], noop) for ()" -> {} }""").strip(), subgraph) @@ -294,10 +293,10 @@ def test_one_level_of_recursion(self): // root subject types: SubA // root entries "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(B),), noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(B)], noop) for SubA"} // internal entries - "(A, (Select(B),), noop) of SubA" -> {"(B, (Select(SubA),), noop) of SubA"} - "(B, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} + "(A, [Select(B)], noop) for SubA" -> {"(B, [Select(SubA)], noop) for SubA"} + "(B, [Select(SubA)], noop) for SubA" -> {"Param(SubA)"} }""").strip(), subgraph) @@ -314,10 +313,10 @@ def test_noop_removal_in_subgraph(self): digraph { // root subject types: SubA // root entries - "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (,), noop) of SubA"} + "Select(A) for ()" [color=blue] + "Select(A) for ()" -> {"(A, [], noop) for ()"} // internal entries - "(A, (,), noop) of SubA" -> {} + "(A, [], noop) for ()" -> {} }""").strip(), subgraph) @@ -333,10 +332,10 @@ def test_noop_removal_full_single_subject_type(self): digraph { // root subject types: SubA // root entries - "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (,), noop) of SubA"} + "Select(A) for ()" [color=blue] + "Select(A) for ()" -> {"(A, [], noop) for ()"} // internal entries - "(A, (,), noop) of SubA" -> {} + "(A, [], noop) for ()" -> {} }""").strip(), fullgraph) @@ -355,9 +354,12 @@ def test_root_tuple_removed_when_no_matches(self): // root subject types: C, D // root entries "Select(A) for C" [color=blue] - "Select(A) for C" -> {"(A, (Select(C),), noop) of C"} + "Select(A) for C" -> {"(A, [Select(C)], noop) for C"} + "Select(B) for (C+D)" [color=blue] + "Select(B) for (C+D)" -> {"(B, [Select(D), Select(A)], noop) for (C+D)"} // internal entries - "(A, (Select(C),), noop) of C" -> {"Param(C)"} + "(A, [Select(C)], noop) for C" -> {"Param(C)"} + "(B, [Select(D), Select(A)], noop) for (C+D)" -> {"(A, [Select(C)], noop) for C" "Param(D)"} }""").strip(), fullgraph) @@ -375,10 +377,10 @@ def test_noop_removal_transitive(self): digraph { // root subject types: SubA // root entries - "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (,), noop) of SubA"} + "Select(A) for ()" [color=blue] + "Select(A) for ()" -> {"(A, [], noop) for ()"} // internal entries - "(A, (,), noop) of SubA" -> {} + "(A, [], noop) for ()" -> {} }""").strip(), subgraph) @@ -395,9 +397,9 @@ def test_get_with_matching_singleton(self): // root subject types: SubA // root entries "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(SubA),), [Get(B, C)], noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(SubA)], [Get(B, C)], noop) for SubA"} // internal entries - "(A, (Select(SubA),), [Get(B, C)], noop) of SubA" -> {"Singleton(B(), B)" "Param(SubA)"} + "(A, [Select(SubA)], [Get(B, C)], noop) for SubA" -> {"Param(SubA)" "Singleton(B(), B)"} }""").strip(), subgraph) @@ -415,10 +417,10 @@ def test_depends_on_multiple_one_noop(self): // root subject types: SubA // root entries "Select(B) for SubA" [color=blue] - "Select(B) for SubA" -> {"(B, (Select(A),), noop) of SubA"} + "Select(B) for SubA" -> {"(B, [Select(A)], noop) for SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} - "(B, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "(A, [Select(SubA)], noop) for SubA" -> {"Param(SubA)"} + "(B, [Select(A)], noop) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} }""").strip(), subgraph) @@ -436,15 +438,15 @@ def test_multiple_depend_on_same_rule(self): // root subject types: SubA // root entries "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "Select(A) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} "Select(B) for SubA" [color=blue] - "Select(B) for SubA" -> {"(B, (Select(A),), noop) of SubA"} + "Select(B) for SubA" -> {"(B, [Select(A)], noop) for SubA"} "Select(C) for SubA" [color=blue] - "Select(C) for SubA" -> {"(C, (Select(A),), noop) of SubA"} + "Select(C) for SubA" -> {"(C, [Select(A)], noop) for SubA"} // internal entries - "(A, (Select(SubA),), noop) of SubA" -> {"Param(SubA)"} - "(B, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} - "(C, (Select(A),), noop) of SubA" -> {"(A, (Select(SubA),), noop) of SubA"} + "(A, [Select(SubA)], noop) for SubA" -> {"Param(SubA)"} + "(B, [Select(A)], noop) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} + "(C, [Select(A)], noop) for SubA" -> {"(A, [Select(SubA)], noop) for SubA"} }""").strip(), subgraph) @@ -460,11 +462,11 @@ def test_get_simple(self): digraph { // root subject types: SubA // root entries - "Select(A) for SubA" [color=blue] - "Select(A) for SubA" -> {"(A, (,), [Get(B, D)], noop) of SubA"} + "Select(A) for ()" [color=blue] + "Select(A) for ()" -> {"(A, [], [Get(B, D)], noop) for ()"} // internal entries - "(A, (,), [Get(B, D)], noop) of SubA" -> {"(B, (Select(D),), noop) of D"} - "(B, (Select(D),), noop) of D" -> {"Param(D)"} + "(A, [], [Get(B, D)], noop) for ()" -> {"(B, [Select(D)], noop) for D"} + "(B, [Select(D)], noop) for D" -> {"Param(D)"} }""").strip(), subgraph) From c82c87bfdac2170b8092f5965a131fa6f1159f96 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Tue, 18 Sep 2018 20:37:13 -0700 Subject: [PATCH 11/13] Remove HasProducts and Variants --- src/python/pants/engine/native.py | 6 ------ src/python/pants/engine/scheduler.py | 8 +------- src/python/pants/engine/struct.py | 27 -------------------------- src/rust/engine/src/lib.rs | 4 ---- src/rust/engine/src/nodes.rs | 27 ++++++++------------------ src/rust/engine/src/types.rs | 2 -- tests/python/pants_test/engine/util.py | 8 ++------ 7 files changed, 11 insertions(+), 71 deletions(-) diff --git a/src/python/pants/engine/native.py b/src/python/pants/engine/native.py index 9b4d2be469f..db1f839961b 100644 --- a/src/python/pants/engine/native.py +++ b/src/python/pants/engine/native.py @@ -196,8 +196,6 @@ TypeConstraint, TypeConstraint, TypeConstraint, - TypeConstraint, - TypeConstraint, TypeId, TypeId, Buffer, @@ -801,9 +799,7 @@ def new_scheduler(self, construct_file, construct_link, construct_process_result, - constraint_has_products, constraint_address, - constraint_variants, constraint_path_globs, constraint_directory_digest, constraint_snapshot, @@ -835,8 +831,6 @@ def tc(constraint): func(construct_process_result), # TypeConstraints. tc(constraint_address), - tc(constraint_has_products), - tc(constraint_variants), tc(constraint_path_globs), tc(constraint_directory_digest), tc(constraint_snapshot), diff --git a/src/python/pants/engine/scheduler.py b/src/python/pants/engine/scheduler.py index de7e5851991..4dec907aa01 100644 --- a/src/python/pants/engine/scheduler.py +++ b/src/python/pants/engine/scheduler.py @@ -21,10 +21,9 @@ from pants.engine.nodes import Return, State, Throw from pants.engine.rules import RuleIndex, SingletonRule, TaskRule from pants.engine.selectors import Select, constraint_for -from pants.engine.struct import HasProducts, Variants from pants.util.contextutil import temporary_file_path from pants.util.dirutil import check_no_overlapping_paths -from pants.util.objects import Collection, SubclassesOf, datatype +from pants.util.objects import Collection, datatype from pants.util.strutil import pluralize @@ -103,9 +102,6 @@ def __init__( self._native = native self.include_trace_on_error = include_trace_on_error - # TODO: The only (?) case where we use inheritance rather than exact type unions. - has_products_constraint = SubclassesOf(HasProducts) - # Validate and register all provided and intrinsic tasks. rule_index = RuleIndex.create(list(rules)) self._root_subject_types = sorted(rule_index.roots, key=repr) @@ -132,9 +128,7 @@ def __init__( construct_file=File, construct_link=Link, construct_process_result=FallibleExecuteProcessResult, - constraint_has_products=has_products_constraint, constraint_address=constraint_for(Address), - constraint_variants=constraint_for(Variants), constraint_path_globs=constraint_for(PathGlobs), constraint_directory_digest=constraint_for(DirectoryDigest), constraint_snapshot=constraint_for(Snapshot), diff --git a/src/python/pants/engine/struct.py b/src/python/pants/engine/struct.py index 2e648eacb73..537566999a6 100644 --- a/src/python/pants/engine/struct.py +++ b/src/python/pants/engine/struct.py @@ -4,14 +4,12 @@ from __future__ import absolute_import, division, print_function, unicode_literals -from abc import abstractproperty from collections import MutableMapping, MutableSequence from future.utils import binary_type, text_type from pants.engine.addressable import addressable, addressable_list from pants.engine.objects import Serializable, SerializableFactory, Validatable, ValidationError -from pants.util.meta import AbstractClass from pants.util.objects import SubclassesOf, SuperclassesOf @@ -307,28 +305,3 @@ def dependencies(self): :rtype: list """ - - -class HasProducts(AbstractClass): - """A mixin for a class that has a collection of products which it would like to expose.""" - - @abstractproperty - def products(self): - """Returns a collection of products held by this class.""" - - -class Variants(Struct): - """A struct that holds default variant values. - - Variants are key-value pairs representing uniquely identifying parameters for a Node. - - Default variants are usually configured on a Target to be used whenever they are - not specified by a caller. - """ - - def __init__(self, default=None, **kwargs): - """ - :param dict default: A dict of default variant values. - """ - # TODO: enforce the type of variants using the Addressable framework. - super(Variants, self).__init__(default=default, **kwargs) diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 5209646bf60..044eb142c10 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -220,8 +220,6 @@ pub extern "C" fn scheduler_create( construct_link: Function, construct_process_result: Function, type_address: TypeConstraint, - type_has_products: TypeConstraint, - type_has_variants: TypeConstraint, type_path_globs: TypeConstraint, type_directory_digest: TypeConstraint, type_snapshot: TypeConstraint, @@ -261,8 +259,6 @@ pub extern "C" fn scheduler_create( construct_link: construct_link, construct_process_result: construct_process_result, address: type_address, - has_products: type_has_products, - has_variants: type_has_variants, path_globs: type_path_globs, directory_digest: type_directory_digest, snapshot: type_snapshot, diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs index 16ab7fc8ee6..db990a78860 100644 --- a/src/rust/engine/src/nodes.rs +++ b/src/rust/engine/src/nodes.rs @@ -133,26 +133,16 @@ impl Select { } /// - /// Looks for has-a or is-a relationships between the given value and the requested product. + /// Looks for an is-a relationship between the given value and the requested product. /// - /// Returns the resulting product Value for success, or the original Value for failure. + /// Returns the original product Value for either success or failure. /// - fn select_literal(&self, context: &Context, candidate: Value) -> Result { - // Check whether the subject is-a instance of the product. + fn select_literal(&self, candidate: Value) -> Result { if externs::satisfied_by(&self.selector.product, &candidate) { - return Ok(candidate); + Ok(candidate) + } else { + Err(candidate) } - - // Else, check whether it has-a instance of the product. - // TODO: returning only the first item of a particular type. - if externs::satisfied_by(&context.core.types.has_products, &candidate) { - for child in externs::project_multi(&candidate, "products") { - if externs::satisfied_by(&self.selector.product, &child) { - return Ok(child); - } - } - } - Err(candidate) } fn snapshot( @@ -307,8 +297,7 @@ impl WrappedNode for Select { fn run(self, context: Context) -> NodeFuture { // If the Subject "is a" or "has a" Product, then we're done. - if let Ok(value) = self.select_literal(&context, externs::val_for(self.params.expect_single())) - { + if let Ok(value) = self.select_literal(externs::val_for(self.params.expect_single())) { return ok(value); } @@ -316,7 +305,7 @@ impl WrappedNode for Select { self .gen_node(&context) .and_then(move |value| { - self.select_literal(&context, value).map_err(|value| { + self.select_literal(value).map_err(|value| { throw(&format!( "{} returned a result value that did not satisfy its constraints: {:?}", rule_graph::entry_str(&self.entry), diff --git a/src/rust/engine/src/types.rs b/src/rust/engine/src/types.rs index 8f41141c50d..d19dd2b446e 100644 --- a/src/rust/engine/src/types.rs +++ b/src/rust/engine/src/types.rs @@ -11,8 +11,6 @@ pub struct Types { pub construct_link: Function, pub construct_process_result: Function, pub address: TypeConstraint, - pub has_products: TypeConstraint, - pub has_variants: TypeConstraint, pub path_globs: TypeConstraint, pub directory_digest: TypeConstraint, pub snapshot: TypeConstraint, diff --git a/tests/python/pants_test/engine/util.py b/tests/python/pants_test/engine/util.py index b51894da490..9756569888e 100644 --- a/tests/python/pants_test/engine/util.py +++ b/tests/python/pants_test/engine/util.py @@ -16,7 +16,7 @@ from pants.engine.parser import SymbolTable from pants.engine.scheduler import Scheduler from pants.engine.selectors import Get -from pants.engine.struct import HasProducts, Struct +from pants.engine.struct import Struct from pants.option.global_options import DEFAULT_EXECUTION_OPTIONS from pants.util.memo import memoized from pants.util.objects import SubclassesOf @@ -105,15 +105,11 @@ def create_scheduler(rules, validate=True, native=None): ) -class Target(Struct, HasProducts): +class Target(Struct): def __init__(self, name=None, configurations=None, **kwargs): super(Target, self).__init__(name=name, **kwargs) self.configurations = configurations - @property - def products(self): - return self.configurations - @addressable_list(SubclassesOf(Struct)) def configurations(self): pass From 3eb21d16c773a621a804c6c11b225cfcfaf4bd1f Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 19 Sep 2018 20:43:38 -0700 Subject: [PATCH 12/13] Add a test of ambiguity. --- src/rust/engine/src/rule_graph.rs | 3 ++- tests/python/pants_test/engine/test_rules.py | 21 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/rust/engine/src/rule_graph.rs b/src/rust/engine/src/rule_graph.rs index be0cc0faa78..9010a72ac57 100644 --- a/src/rust/engine/src/rule_graph.rs +++ b/src/rust/engine/src/rule_graph.rs @@ -1000,10 +1000,11 @@ impl RuleGraph { diagnostics.dedup_by(|l, r| l.reason == r.reason); let errors = diagnostics .into_iter() - .map(|d| { + .map(|mut d| { if d.details.is_empty() { d.reason.clone() } else { + d.details.sort(); format!("{}:\n {}", d.reason, d.details.join("\n ")) } }) diff --git a/tests/python/pants_test/engine/test_rules.py b/tests/python/pants_test/engine/test_rules.py index c473a122130..aaaae549a34 100644 --- a/tests/python/pants_test/engine/test_rules.py +++ b/tests/python/pants_test/engine/test_rules.py @@ -78,6 +78,27 @@ def test_ruleset_with_missing_product_type(self): """).strip(), str(cm.exception)) + def test_ruleset_with_ambiguity(self): + rules = [ + TaskRule(A, [Select(C), Select(B)], noop), + TaskRule(A, [Select(B), Select(C)], noop), + RootRule(B), + RootRule(C), + # TODO: Without a rule triggering the selection of A, we don't detect ambiguity here. + TaskRule(D, [Select(A)], noop), + ] + with self.assertRaises(Exception) as cm: + create_scheduler(rules) + + self.assert_equal_with_printing(dedent(""" + Rules with errors: 1 + (D, [Select(A)], noop): + ambiguous rules for Select(A) with parameter types (B+C): + (A, [Select(B), Select(C)], noop) for (B+C) + (A, [Select(C), Select(B)], noop) for (B+C) + """).strip(), + str(cm.exception)) + def test_ruleset_with_rule_with_two_missing_selects(self): rules = _suba_root_rules + [TaskRule(A, [Select(B), Select(C)], noop)] with self.assertRaises(Exception) as cm: From 82992481d5418709924dfd73f967fa9dbca426d8 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Wed, 19 Sep 2018 23:57:36 -0700 Subject: [PATCH 13/13] Use smallvec. --- src/rust/engine/Cargo.lock | 1 + src/rust/engine/Cargo.toml | 1 + src/rust/engine/src/core.rs | 8 ++++---- src/rust/engine/src/lib.rs | 2 ++ 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/rust/engine/Cargo.lock b/src/rust/engine/Cargo.lock index 207251bc17e..cfc00f32d41 100644 --- a/src/rust/engine/Cargo.lock +++ b/src/rust/engine/Cargo.lock @@ -262,6 +262,7 @@ dependencies = [ "log 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)", "process_execution 0.0.1", "resettable 0.0.1", + "smallvec 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)", "tempfile 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", ] diff --git a/src/rust/engine/Cargo.toml b/src/rust/engine/Cargo.toml index 2aa72e9daf4..c472ca9de05 100644 --- a/src/rust/engine/Cargo.toml +++ b/src/rust/engine/Cargo.toml @@ -82,5 +82,6 @@ lazy_static = "0.2.2" log = "0.4" process_execution = { path = "process_execution" } resettable = { path = "resettable" } +smallvec = "0.6" tokio = "0.1" tempfile = "3" diff --git a/src/rust/engine/src/core.rs b/src/rust/engine/src/core.rs index 3d77424fd85..f54bd0615a9 100644 --- a/src/rust/engine/src/core.rs +++ b/src/rust/engine/src/core.rs @@ -10,6 +10,8 @@ use std::{fmt, hash}; use externs; use handles::Handle; +use smallvec::SmallVec; + pub type FNV = hash::BuildHasherDefault; /// @@ -18,15 +20,13 @@ pub type FNV = hash::BuildHasherDefault; /// For efficiency and hashability, they're stored as sorted Keys (with distinct TypeIds), and /// wrapped in an `Arc` that allows us to copy-on-write for param contents. /// -/// TODO: Consider replacing the Arc> with a [smallvec](https://crates.io/crates/smallvec). -/// #[repr(C)] #[derive(Clone, Debug, Default, Eq, Hash, PartialEq)] -pub struct Params(Arc>); +pub struct Params(SmallVec<[Key; 4]>); impl Params { pub fn new_single(param: Key) -> Params { - Params(Arc::new(vec![param])) + Params(smallvec![param]) } /// diff --git a/src/rust/engine/src/lib.rs b/src/rust/engine/src/lib.rs index 044eb142c10..1779fcf6051 100644 --- a/src/rust/engine/src/lib.rs +++ b/src/rust/engine/src/lib.rs @@ -51,6 +51,8 @@ extern crate lazy_static; extern crate log; extern crate process_execution; extern crate resettable; +#[macro_use] +extern crate smallvec; extern crate tokio; use std::ffi::CStr;