Simplify dependency registration. Remove dependency registration as a method and have it do so in the `create_target` method. Bug: 528225104 Change-Id: Ia1c5ee87d494bf75c6507c3def4159e36a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/25560 Commit-Queue: Matt Stark <msta@google.com> Reviewed-by: Takuto Ikuta <tikuta@google.com>
diff --git a/src/gn/starlark/crates/attr/src/attr.rs b/src/gn/starlark/crates/attr/src/attr.rs index 5e1fc56..bd73c85 100644 --- a/src/gn/starlark/crates/attr/src/attr.rs +++ b/src/gn/starlark/crates/attr/src/attr.rs
@@ -261,35 +261,34 @@ } } - /// Registers target dependencies contained within this attribute value. - pub fn register_dependencies<S: crate::Session>( - &self, - session: &S, - source: S::TargetRef, - toolchain: LabelRef<'_>, + /// Collects dependencies contained within this attribute value to `out`. + pub fn add_dependencies<'a>( + &'a self, + toolchain: LabelRef<'a>, + out: &mut SmallSet<(LabelRef<'a>, LabelRef<'a>)>, ) { match self { Self::Label(Some(LabelOrFile::Label(lbl))) => { - session.register_dependency(source, lbl.as_ref(), toolchain); + out.insert((lbl.as_ref(), toolchain)); }, Self::LabelList(list) => { for lf in list { if let LabelOrFile::Label(lbl) = lf { - session.register_dependency(source.clone(), lbl.as_ref(), toolchain); + out.insert((lbl.as_ref(), toolchain)); } } }, Self::LabelKeyedStringDict(dict) => { for (lf, _) in dict { if let LabelOrFile::Label(lbl) = lf { - session.register_dependency(source.clone(), lbl.as_ref(), toolchain); + out.insert((lbl.as_ref(), toolchain)); } } }, Self::StringKeyedLabelDict(dict) => { for (_, lf) in dict { if let LabelOrFile::Label(lbl) = lf { - session.register_dependency(source.clone(), lbl.as_ref(), toolchain); + out.insert((lbl.as_ref(), toolchain)); } } }, @@ -297,7 +296,7 @@ for (_, list) in dict { for lf in list { if let LabelOrFile::Label(lbl) = lf { - session.register_dependency(source.clone(), lbl.as_ref(), toolchain); + out.insert((lbl.as_ref(), toolchain)); } } }
diff --git a/src/gn/starlark/crates/attr/src/value.rs b/src/gn/starlark/crates/attr/src/value.rs index a8075ae..8bc8b55 100644 --- a/src/gn/starlark/crates/attr/src/value.rs +++ b/src/gn/starlark/crates/attr/src/value.rs
@@ -232,8 +232,6 @@ #[cfg(test)] mod tests { - use std::collections::HashSet; - use starlark::{ environment::Module, values::{list::UnpackList, UnpackValue as _, ValueLike as _}, @@ -446,19 +444,11 @@ ) .unwrap(); - let source_target = session.insert_empty_target(PackageRef::root(), "source"); - attr.register_dependencies( - &session, - source_target.clone(), - session.default_toolchain.as_ref(), - ); - + let mut deps = SmallSet::new(); + attr.add_dependencies(session.default_toolchain.as_ref(), &mut deps); assert_eq!( - source_target.registered_deps(), - HashSet::from([( - target_single.label().to_owned(), - session.default_toolchain.clone() - )]) + deps, + SmallSet::from_iter([(target_single.label(), session.default_toolchain.as_ref(),)]) ); let AttrValue {
diff --git a/src/gn/starlark/crates/ffi/src/eval_context.rs b/src/gn/starlark/crates/ffi/src/eval_context.rs index 5a1a604..19d8b3b 100644 --- a/src/gn/starlark/crates/ffi/src/eval_context.rs +++ b/src/gn/starlark/crates/ffi/src/eval_context.rs
@@ -97,6 +97,6 @@ _rule: starlark::values::FrozenValue, _attrs: Vec<attr::Attr>, ) -> starlark::Result<<Self::Session as types::Session>::TargetRef> { - todo!() + todo!("Create C++ target and register dependencies"); } }
diff --git a/src/gn/starlark/crates/ffi/src/session.rs b/src/gn/starlark/crates/ffi/src/session.rs index ae97f24..898660d 100644 --- a/src/gn/starlark/crates/ffi/src/session.rs +++ b/src/gn/starlark/crates/ffi/src/session.rs
@@ -113,13 +113,4 @@ fn get_target(&self, _label: LabelRef<'_>, _toolchain: LabelRef<'_>) -> Self::TargetRef { todo!() } - - fn register_dependency<'a>( - &self, - _source: Self::TargetRef, - _label: LabelRef<'a>, - _toolchain: LabelRef<'a>, - ) { - todo!() - } }
diff --git a/src/gn/starlark/crates/ffi/src/target_ref.rs b/src/gn/starlark/crates/ffi/src/target_ref.rs index 7626a88..238a59f 100644 --- a/src/gn/starlark/crates/ffi/src/target_ref.rs +++ b/src/gn/starlark/crates/ffi/src/target_ref.rs
@@ -61,14 +61,6 @@ todo!() } - fn register_dependencies<S: types::Session<TargetRef = Self>>( - &self, - _session: &S, - _toolchain: LabelRef<'_>, - ) { - todo!() - } - fn output_type(&self) -> Option<types::OutputType> { todo!() }
diff --git a/src/gn/starlark/crates/rule/src/frozen_rule.rs b/src/gn/starlark/crates/rule/src/frozen_rule.rs index c29fedd..056ab77 100644 --- a/src/gn/starlark/crates/rule/src/frozen_rule.rs +++ b/src/gn/starlark/crates/rule/src/frozen_rule.rs
@@ -13,7 +13,7 @@ values::{FrozenHeap, FrozenValue, StarlarkValue, Value}, }; use starlark_derive::{starlark_value, NoSerialize}; -use types::{EvaluatorContextExt, Scope, TargetRef}; +use types::{EvaluatorContextExt, Scope}; use crate::rule::{build_signature, OutputType}; @@ -113,16 +113,15 @@ }) .collect::<Result<Vec<_>, _>>()?; - let target = if let Some(builtin) = self.builtin { + if let Some(builtin) = self.builtin { // Collect all the arguments we don't recognise and pass them to the native // implementation. let kwargs: SmallMap<String, Value<'v>> = param_parser.next()?; let child_scope = scope.copy_with(kwargs.iter().map(|(k, v)| (k.as_str(), *v)))?; - context.create_target(Some(builtin), target_name, &*child_scope, me, attrs)? + context.create_target(Some(builtin), target_name, &*child_scope, me, attrs)?; } else { - context.create_target(None, target_name, scope, me, attrs)? - }; - target.register_dependencies(context.session(), context.current_toolchain()); + context.create_target(None, target_name, scope, me, attrs)?; + } Ok(Value::new_none()) })
diff --git a/src/gn/starlark/crates/rule/tests/frozen_rule.rs b/src/gn/starlark/crates/rule/tests/frozen_rule.rs index 45b814f..589cb4f 100644 --- a/src/gn/starlark/crates/rule/tests/frozen_rule.rs +++ b/src/gn/starlark/crates/rule/tests/frozen_rule.rs
@@ -2,10 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -use std::{ - collections::{HashMap, HashSet}, - sync::Mutex, -}; +use std::collections::{HashMap, HashSet}; use attr::{Attr, LabelOrFile}; use rule::FrozenRule; @@ -95,7 +92,7 @@ output_type: Some(OutputType::SharedLibrary), rule: rule(&native, "custom_shared_library"), cxx_attrs: unknown_attrs.clone(), - dependencies: Mutex::new(HashSet::new()), + dependencies: HashSet::new(), } ); @@ -109,7 +106,7 @@ output_type: Some(OutputType::StaticLibrary), rule: rule(&native, "static_library"), cxx_attrs: unknown_attrs.clone(), - dependencies: Mutex::new(HashSet::new()), + dependencies: HashSet::new(), } ); @@ -134,10 +131,10 @@ output_type: None, rule: rule(&pure, "parent_rule"), cxx_attrs: HashMap::new(), - dependencies: Mutex::new(HashSet::from([( + dependencies: HashSet::from([( Label::new(PackageRef::root().to_owned(), "parent".to_owned()), toolchain.clone(), - )])), + )]), } ); @@ -158,10 +155,10 @@ output_type: None, rule: rule(&pure, "child_rule"), cxx_attrs: HashMap::new(), - dependencies: Mutex::new(HashSet::from([( + dependencies: HashSet::from([( Label::new(PackageRef::root().to_owned(), "child".to_owned()), toolchain.clone(), - )])), + )]), } ); @@ -182,10 +179,10 @@ output_type: None, rule: rule(&pure, "child_rule"), cxx_attrs: HashMap::new(), - dependencies: Mutex::new(HashSet::from([( + dependencies: HashSet::from([( Label::new(PackageRef::root().to_owned(), "custom_val".to_owned()), toolchain.clone(), - )])), + )]), } ); }
diff --git a/src/gn/starlark/crates/testutils/src/eval_context.rs b/src/gn/starlark/crates/testutils/src/eval_context.rs index 570f750..47a678e 100644 --- a/src/gn/starlark/crates/testutils/src/eval_context.rs +++ b/src/gn/starlark/crates/testutils/src/eval_context.rs
@@ -140,8 +140,16 @@ ) -> Result<<Self::Session as AttrSession>::TargetRef> { let label = Label::new(self.package.clone(), target_name.to_owned()); let toolchain = self.current_toolchain().to_owned(); + let mut deps = starlark::collections::SmallSet::new(); + for attr in &attrs { + attr.add_dependencies(toolchain.as_ref(), &mut deps); + } Ok(self.session.insert_target(FakeTarget { label, + dependencies: deps + .into_iter() + .map(|(l, tc)| (l.to_owned(), tc.to_owned())) + .collect(), toolchain, output_type: target_type, rule: if rule.is_none() { @@ -154,7 +162,6 @@ cxx_attrs: scope.0.clone(), outputs: vec![], attrs, - dependencies: Default::default(), })) } }
diff --git a/src/gn/starlark/crates/testutils/src/session.rs b/src/gn/starlark/crates/testutils/src/session.rs index 9118d7c..a9d9017 100644 --- a/src/gn/starlark/crates/testutils/src/session.rs +++ b/src/gn/starlark/crates/testutils/src/session.rs
@@ -94,17 +94,4 @@ ); } } - - fn register_dependency<'a>( - &self, - source: Self::TargetRef, - target: LabelRef<'a>, - toolchain: LabelRef<'a>, - ) { - source - .dependencies - .lock() - .unwrap() - .insert((target.to_owned(), toolchain.to_owned())); - } }
diff --git a/src/gn/starlark/crates/testutils/src/target.rs b/src/gn/starlark/crates/testutils/src/target.rs index 4889eed..899abe5 100644 --- a/src/gn/starlark/crates/testutils/src/target.rs +++ b/src/gn/starlark/crates/testutils/src/target.rs
@@ -5,7 +5,7 @@ use std::{ collections::{HashMap, HashSet}, ops::Deref, - sync::{Arc, Mutex}, + sync::Arc, }; use allocative::Allocative; @@ -15,9 +15,7 @@ values::{Heap, ProvidesStaticType, StarlarkValue, Value, ValueLike}, }; use starlark_derive::{starlark_value, NoSerialize}; -use types::{ - File, IPromiseToImplementStarlarkEqAndHash, Label, LabelRef, OutputType, Session, TargetRef, -}; +use types::{File, IPromiseToImplementStarlarkEqAndHash, Label, LabelRef, OutputType, TargetRef}; use crate::FakeEvalContext; @@ -36,7 +34,7 @@ pub cxx_attrs: HashMap<String, Value<'static>>, /// Registered target dependencies. #[allocative(skip)] - pub dependencies: Mutex<HashSet<(Label, Label)>>, + pub dependencies: HashSet<(Label, Label)>, } impl PartialEq for FakeTarget { @@ -54,7 +52,7 @@ .get(k) .is_some_and(|ov| v.equals(*ov).unwrap_or(false)) }) - && *self.dependencies.lock().unwrap() == *other.dependencies.lock().unwrap() + && self.dependencies == other.dependencies } } @@ -77,7 +75,7 @@ /// Returns the registered dependencies of this target. pub fn registered_deps(&self) -> HashSet<(Label, Label)> { - self.dependencies.lock().unwrap().clone() + self.dependencies.clone() } } @@ -160,16 +158,6 @@ format!("{prefix}$TOOLCHAIN/{suffix}$LABEL") } - fn register_dependencies<S: Session<TargetRef = Self>>( - &self, - session: &S, - toolchain: LabelRef<'_>, - ) { - for attr in &self.get().attrs { - attr.register_dependencies(session, self.clone(), toolchain); - } - } - fn builtin_attrs<'v>(&self, _heap: &Heap<'v>) -> Vec<Value<'v>> { self.get() .output_type
diff --git a/src/gn/starlark/crates/types/src/session.rs b/src/gn/starlark/crates/types/src/session.rs index 863dcb6..1b87a80 100644 --- a/src/gn/starlark/crates/types/src/session.rs +++ b/src/gn/starlark/crates/types/src/session.rs
@@ -11,12 +11,4 @@ /// Look up a target in the session by its label and current toolchain. fn get_target(&self, label: LabelRef<'_>, toolchain: LabelRef<'_>) -> Self::TargetRef; - - /// Registers a dependency from `source` to (label, toolchain). - fn register_dependency<'a>( - &self, - source: Self::TargetRef, - label: LabelRef<'a>, - toolchain: LabelRef<'a>, - ); }
diff --git a/src/gn/starlark/crates/types/src/target_ref.rs b/src/gn/starlark/crates/types/src/target_ref.rs index f24c749..08e88fa 100644 --- a/src/gn/starlark/crates/types/src/target_ref.rs +++ b/src/gn/starlark/crates/types/src/target_ref.rs
@@ -4,7 +4,7 @@ use starlark::values::{AllocValue, Heap, StarlarkValue, Value}; -use crate::{File, LabelRef, OutputType, Session}; +use crate::{File, LabelRef, OutputType}; /// Unfortunately while we could specify that Eq and Hash are implemented, there /// is no way to delegate starlark's equality and hash function to it @@ -42,12 +42,6 @@ label_prefix: &str, package_name_separator: &str, ) -> String; - /// Registers target dependencies contained within this target's attributes. - fn register_dependencies<S: Session<TargetRef = Self>>( - &self, - session: &S, - toolchain: LabelRef<'_>, - ); /// Returns the target's output type. fn output_type(&self) -> Option<OutputType>;