-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
Consider feature dependencies in test matrix #52
base: master
Are you sure you want to change the base?
Conversation
…lving skip_feature_set conflicts This has no change in behavior, as cargo_metadata guards against features appearing in both always_include and skip_feature_sets.
…e selected feature sets Fixes frewsxcv#25. Fixes frewsxcv#43. Even though cargo activates dependent features automatically, we can reduce the amount of feature sets when already resolving these dependencies during creation of all feature sets. We decide to go for the more verbose option, selecting [A, B] over [B] when A is a feature dependency of B. This allows an easier implementation, as dependency chains C -> B -> A can be verified one by one, and it takes other rules like skip_feature_sets correctly into account without modification. BREAKING CHANGE: As previously feature sets [B], [A,B] were included separatly, now there will only be [A,B] run if A is a dependency of B. The testcase [B] is redundant, as it activates feature A automatically.
I'm having trouble understanding this change. Can you share a minimal |
The first commit should change nothing, it was just a refactoring making the second commit easier. The second commit performs the actual fix. All test cases in [package]
name = "testdummy"
version = "0.1.0"
[features]
A = []
B = ["A"]
C = ["dep:optDepC"]
[dependencies]
fixDepA = {path = "fixDepA"}
oDepB = {path = "optDepB", package = "optDepB", optional = true}
optDepC = {path = "optDepC", optional = true} Feature However, this is not only an optimization, but in conjunction with skip_feature_sets a true fix. Imagine, skip_feature_sets is set to skip combinations containing both feature |
Please add code comments so it's obvious what the code is doing and why |
.unwrap_or(&FeatureList::default()) | ||
.iter() | ||
{ | ||
if dep.strip_prefix("dep:").is_some() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ;)
@@ -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() { |
There was a problem hiding this comment.
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
continue; | ||
} | ||
if !feature_set.contains(&dep) { | ||
continue 'outer; |
There was a problem hiding this comment.
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 .."
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
These two small commits fix #25 in that transitive, or dependent features are considered during the construction of the test matrix. It also closes #43.
Commit message:
Even though cargo activates dependent features automatically, we can reduce
the amount of feature sets when already resolving these dependencies during creation of all feature sets.
We decide to go for the more verbose option, selecting [A, B] over [B] when A is a feature dependency of B.
This allows an easier implementation, as dependency chains C -> B -> A can be verified one by one, and it takes other rules like
skip_feature_sets correctly into account without modification. (see
__nightly
example from #25)BREAKING CHANGE: As previously feature sets [B], [A,B] were included separatly, now there will only be [A,B] run if A is a dependency of B. The testcase [B] is redundant, as it activates feature A automatically.