Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Consider feature dependencies in test matrix #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+20 −27
Open

Consider feature dependencies in test matrix #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98a8f35
refactor(features_finder): append always_include_features before reso…
demmerichs Sep 18, 2023
38ff2a4
feat(features_finder): feature dependencies will be considered for th…
demmerichs Aug 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/features_finder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub fn fetch_feature_sets(package: &crate::cargo_metadata::Package) -> Vec<Featu
let max_combination_size = package.max_combination_size.unwrap_or(features.len());
for n in 0..=max_combination_size {
'outer: for feature_set in features.iter().combinations(n) {
let feature_set: Vec<_> = feature_set
let feature_set: HashSet<_> = feature_set
.into_iter()
.chain(package.always_include_features.iter())
.collect();
Expand All @@ -87,6 +87,21 @@ pub fn fetch_feature_sets(package: &crate::cargo_metadata::Package) -> Vec<Featu
// skip_feature_set matches: do not add it to feature_sets
continue 'outer;
}
for feat in feature_set.clone() {
Copy link
Collaborator

@Manishearth Manishearth Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we shouldn't have to clone here, we're not mutating. borrow it

for dep in package
.feature_map
.get(&feat.0)
.unwrap_or(&FeatureList::default())
.iter()
{
if dep.strip_prefix("dep:").is_some() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

note that this won't work for old-style implicit dependency features

but we haven't been careful about that in this tool anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same mechanism is used in line 22. So I would leave this as is, and an issue could be opened discussing this further. I have no knowledge about the old-style you are referring to, so I personally would not open that issue ;)

continue;
}
if !feature_set.contains(&dep) {
continue 'outer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: it would be nice if these skipped feature combos were still included in the output at the bottom, with an explanation of which run they are equivalent to. unsure if that's going to be really annoying to do properly, could be fine with "skipped covered feature set [...] due to dependency from .. to .."

Copy link
Contributor Author

@demmerichs demmerichs Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at my follow-up work regarding the rule abstraction, those dependencies would be encoded with the same rule mechanism as other selection rules, e.g. skip_feature_sets, denylists etc.
For this reason, I would not want to build this into this for-loop, but maybe externally creating a list of skipped feature sets because of dependencies. This could even be generalized, to print for every skipped feature set one or even all rules, that result in the set skipped. But I also think that in normal use cases, this output is unwanted.

What you are probably thinking of is helping users understand all the rules applied to the feature set selection and help "debugging" those rules. Probably, this workflow is started by recognizing a specific feature set is not run while the dev does not understand why it is skipped. So the dev wants to "ask" why a certain feature set was skipped. We could provide a CLI flag, taking a feature set as argument value, and we print all the conflicting rules to the screen, if it is skipped.

I think this would be an easy and nice addition to the tool, but I would like to implement it on top of the rule abstraction, because as of now, this would be quite tedious handling every "rule" case separately.

What do you think @Manishearth?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's fine, also this was a "thought"; I wasn't requiring this of this PR.

I'm also not sure we should print every skipped set.

}
}
}
feature_sets.push(feature_set.into_iter().cloned().collect());
}
}
Expand Down
20 changes: 0 additions & 20 deletions tests/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,15 @@ fn simple() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["C"],
vec!["oDepB"],
vec!["A", "B"],
vec!["A", "C"],
vec!["A", "oDepB"],
vec!["B", "C"],
vec!["B", "oDepB"],
vec!["C", "oDepB"],
vec!["A", "B", "C"],
vec!["A", "B", "oDepB"],
vec!["A", "C", "oDepB"],
vec!["B", "C", "oDepB"],
vec!["A", "B", "C", "oDepB"],
];
test_settings("", valid_feature_sets, None)
Expand All @@ -37,11 +33,9 @@ fn skip_sets_1() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["oDepB"],
vec!["A", "B"],
vec!["A", "oDepB"],
vec!["B", "oDepB"],
vec!["A", "B", "oDepB"],
];
test_settings(settings, valid_feature_sets, None)
Expand All @@ -57,14 +51,11 @@ fn skip_sets_2() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["C"],
vec!["oDepB"],
vec!["A", "B"],
vec!["A", "C"],
vec!["A", "oDepB"],
vec!["B", "C"],
vec!["B", "oDepB"],
vec!["A", "B", "C"],
vec!["A", "B", "oDepB"],
];
Expand All @@ -81,14 +72,11 @@ fn skip_sets_3() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["C"],
vec!["oDepB"],
vec!["A", "B"],
vec!["A", "C"],
vec!["A", "oDepB"],
vec!["B", "C"],
vec!["B", "oDepB"],
vec!["C", "oDepB"],
vec!["A", "B", "C"],
vec!["A", "B", "oDepB"],
Expand All @@ -105,11 +93,9 @@ fn skip_opt_deps() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["C"],
vec!["A", "B"],
vec!["A", "C"],
vec!["B", "C"],
vec!["A", "B", "C"],
];
test_settings(settings, valid_feature_sets, None)
Expand All @@ -132,11 +118,9 @@ fn denylist() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["oDepB"],
vec!["A", "B"],
vec!["A", "oDepB"],
vec!["B", "oDepB"],
vec!["A", "B", "oDepB"],
];
test_settings(settings, valid_feature_sets, None)
Expand All @@ -151,19 +135,15 @@ fn extra_feats() -> Result<(), Box<dyn std::error::Error>> {
let valid_feature_sets = vec![
vec![],
vec!["A"],
vec!["B"],
vec!["C"],
vec!["oDepB"],
vec!["A", "B"],
vec!["A", "C"],
vec!["A", "oDepB"],
vec!["B", "C"],
vec!["B", "oDepB"],
vec!["C", "oDepB"],
vec!["A", "B", "C"],
vec!["A", "B", "oDepB"],
vec!["A", "C", "oDepB"],
vec!["B", "C", "oDepB"],
vec!["A", "B", "C", "oDepB"],
];
test_settings(settings, valid_feature_sets, None)
Expand Down
Loading