Add functionality to pass targets to a dependency #235
Conversation
mrogenmoserTT
force-pushed
the
pass_trgts
branch
from
411bc74 to
7a93306
Compare
mrogenmoserTT
force-pushed
the
pass_trgts
branch
from
7a93306 to
c1a4aad
Compare
| .pass_targets | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|s| s.to_lowercase()) |
There was a problem hiding this comment.
Are all passed targets internally converted to lowercase?
There was a problem hiding this comment.
Targets are handled as case-insensitive in bender (especially as the resulting defines are capitalized).
| let passed_targets: Vec<IndexMap<String, IndexSet<String>>> = ranks | ||
| .clone() | ||
| .into_iter() | ||
| .chain(once(vec![Some(self.sess.manifest)])) | ||
| .map(|manifests| { | ||
| let vec_of_indexmaps = manifests | ||
| .clone() | ||
| .into_iter() | ||
| .flatten() | ||
| .map(|m| { | ||
| m.dependencies | ||
| .clone() | ||
| .into_iter() | ||
| .map(|(d, c)| { | ||
| let tmp_tgts = match c { | ||
| config::Dependency::Path(_, ref pass_tgts) => pass_tgts, | ||
| config::Dependency::Version(_, ref pass_tgts) => pass_tgts, | ||
| config::Dependency::GitRevision(_, _, ref pass_tgts) => { | ||
| pass_tgts | ||
| } | ||
| config::Dependency::GitVersion(_, _, ref pass_tgts) => { | ||
| pass_tgts | ||
| } | ||
| }; | ||
| (d.clone(), tmp_tgts.clone()) | ||
| }) | ||
| .collect::<IndexMap<String, Vec<String>>>() | ||
| }) | ||
| .collect::<Vec<IndexMap<String, Vec<String>>>>(); | ||
| let mut final_indexmap = IndexMap::<String, IndexSet<String>>::new(); | ||
| for element in vec_of_indexmaps { | ||
| for (k, v) in element { | ||
| let existing: IndexSet<String> = | ||
| final_indexmap.get(&k).unwrap_or(&IndexSet::new()).clone(); | ||
| final_indexmap.insert( | ||
| k, | ||
| IndexSet::from_iter( | ||
| [existing.into_iter().collect::<Vec<_>>(), v] | ||
| .concat() | ||
| .into_iter(), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| final_indexmap | ||
| }) | ||
| .collect(); | ||
|
|
||
| let mut final_indexmap = IndexMap::<String, IndexSet<String>>::new(); | ||
| for element in passed_targets { | ||
| for (k, v) in element { | ||
| let existing: IndexSet<String> = | ||
| final_indexmap.get(&k).unwrap_or(&IndexSet::new()).clone(); | ||
| final_indexmap.insert( | ||
| k, | ||
| IndexSet::from_iter( | ||
| [ | ||
| existing.into_iter().collect::<Vec<_>>(), | ||
| v.into_iter().collect::<Vec<_>>(), | ||
| ] | ||
| .concat() | ||
| .into_iter(), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| let final_indexmap: IndexMap<String, TargetSet> = final_indexmap | ||
| .into_iter() | ||
| .map(|(k, v)| (k, TargetSet::new(v.into_iter()))) | ||
| .collect(); | ||
|
|
There was a problem hiding this comment.
I had to study this for a while until I understood the concepts of ranks in dependency. I think the implementation is correct. I was wondering though whether the targets need to be reduced hierarchically by rank or whether it is also possible to just flatten all the manifests and then reduce/collect the targets. This would be much easier to implement since it just a nested for loop over manifests + dependency of a manifest. Or do you see an issue with that? I am not sure whether that might cause issues with ordering etc.,
There was a problem hiding this comment.
We want to ensure the output for the scripts is ordered by ranks to have dependencies compiled before the top by any tool that uses it. I'm not sure of the effects in this specific location, but keeping this order can't hurt. There are likely quite a few optimizations that could be done around bender for this.
| debugln!("resolve: registering {} from lockfile", &name); | ||
| let dep = match &locked_package.source { | ||
| LockedSource::Path(p) => config::Dependency::Path(p.clone()), | ||
| LockedSource::Path(p) => config::Dependency::Path(p.clone(), Vec::new()), |
There was a problem hiding this comment.
I am not sure, but would it make sense to record the targets in the lockfile?
There was a problem hiding this comment.
The lockfile is only changed when a bender update is run, but realistically, the targets that are passed to dependencies could be adjusted irrespective of full-blown dependency updates. Thus, I think that is an unhelpful restriction to keep this information in the lockfile.
| .unwrap_or_else(|| SourceGroup { | ||
| package: Default::default(), | ||
| independent: true, | ||
| target: TargetSpec::Wildcard, | ||
| include_dirs: Default::default(), | ||
| export_incdirs: Default::default(), | ||
| defines: Default::default(), | ||
| files: Default::default(), | ||
| dependencies: Default::default(), | ||
| version: None, | ||
| passed_targets: TargetSet::empty(), | ||
| }); |
There was a problem hiding this comment.
Unrelated to your changes, but we could derive/implement the Default trait for SourceGroup🤓 Because there are a couple of locations where an empty source group is created.
There was a problem hiding this comment.
That sounds like a great idea! Separate PR?
2 participants
Add
pass_targetsto dependencies to allow passing targets for hierarchical file filtering (not only global).