Integration tests, refactoring, and powerful feature rule expressions #50
Conversation
… parsing more readable
…list setting Even when providing allowlist, using the boolean flag skip_optional_dependencies can be helpful for quickly switching between two behaviors without the need of changing allowlist. BREAKING CHANGE: Instead of erroring when provided with an allowlist and skip_optional_dependencies, now the optional dependencies will be removed from the allowlist prior to building all feature sets.
…ways_include_features A skip_feature_set [A,B] together with an always_include_feature A basically turns into a denylist B. However, allowing the first setting, can allow easy testing and switching on and off different settings by only changing always_include_feature without the need of removing skip_feature_sets and adding elements to the denylist. BREAKING CHANGE: Instead of erroring out when a feature is present in skip_feature_sets and always_include_features, now both settings are taken into account. Note, that this can lead to empty sets, e.g. when both A and B are always included but also provided as skip set.
…e selected feature sets Fixes frewsxcv#25. Fixes frewsxcv#43. 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.
This simplifies future additions of other selection settings.
…fying very general feature set rules Fix frewsxcv#42. See added tests.
…ighting conflicting rules BREAKING CHANGE: Instead of not running any feature set, but exiting ok, we now produce an error with a helpful mesage.
|
@frewsxcv @Manishearth @robertbastian Is anyone of you still actively maintaining this project? |
|
@frewsxcv is the main owner, Manish and I recently got merge rights for our own changes, but I don't feel comfortable approving such a big change. |
|
@robertbastian He gave you also maintainer rights on crates.io Are you sure he intended for you to only merge your stuff and not to also help maintain this package? It seems he has been unresponsive for quite some time on this project. |
|
I'm fine with us approving some of this, but in general I would very much prefer this be split into smaller changes that can be landed separately. |
|
I am happy to split it up into multiple PR, if that is what you mean. Or do you want also the commits to be smaller? |
Agreed with Manish, that this is way too big of a PR to review. In the future, if you want to contribute, please discuss with us before making such large changes |
|
Multiple PRs, yes. Keep things focused to one thing at a time. I can't even begin to review the rule changes without this being split up. Likely I would also want to see a rough design before I look at code for that, but that depends on how complex it is. |
|
@frewsxcv I am sorry if I ran in some doors here, this was not my intention. I am okay with any or all commits being rejected :) The reasons I took this path:
|
|
@Manishearth I will split this up into multiple PRs tonight, the commits before the Regarding the However, the main goal is documented in a concise way in the README. Thanks for all the pointers, this is my first bigger commit and I appreciate you helping me to make this PR(s) better :) |
Any progress on this? This proposal looks like a very nice addition to the tool and would also help in my case.
For the purpose of increasing the chances to get this accepted, I suggest you copy-paste from the file you linked to above, and add as an initial PR to add the design description accepted in first. Once that is in, the rest of the changes can be split in basic functional bits in separate PRs. Disclaimer: I am not involved in the project, I'm just trying to help get this in. |
|
Hi @eddyp thanks for the endorsement.
This is currently in progress. The integration tests got already merged into master, and currently some fixes of existing issues and refactoring are an open PR #52. Feel free to review and comment, I think the maintainers are quite busy and can't go through it right now. It might help if you and me already have iterated that PR to move things along. [dependencies]
cargo-all-features = { git = "https://github.com/frewsxcv/cargo-all-features", rev = "refs/pull/50/head" }
Not sure, I got the just of it. Is it common practice to have PRs that are basically empty and just are a room for discussion of design decisions? Wouldn't you use issues for that? Splitting up the functional bits would happen after the current PR gets accepted and I will see how small PRs I can make that still make kind of sense standalone. |
The first few commits add integration tests and minor refactoring.
Then there is one larger commit, also just doing refactoring, but using rule-based abstraction for resolving valid feature sets. The rules are constructed from nested logical expressions.
The following commit makes us of this, implements a rule parser on top, which allows us to specify arbitrary rule expressions for filtering the set of feature combinations to try out. I link a few issues that can be handled nicely by these changes, and any future requests for strange feature dependencies should be covered by this.
Finally, I added some more code to give an informing error output in case the set of valid feature sets is empty, so that the user can more easily identify the conflicting rules.
Fixes #25.
Fixes #43.
Fixes #42.
Fixes #12.