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

Should --each-feature and --feature-powerset run --all-features? #42

Closed
taiki-e opened this issue May 24, 2020 · 8 comments · Fixed by #61
Closed

Should --each-feature and --feature-powerset run --all-features? #42

taiki-e opened this issue May 24, 2020 · 8 comments · Fixed by #61
Assignees
@taiki-e
Labels
A-features Area: features (--feature-powerset, --each-feature, etc.)
Milestone
v0.4

Comments

@taiki-e
Copy link
Owner

taiki-e commented May 24, 2020

default/no-default features are already included, so all-features also need to be included?
@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label May 24, 2020
@taiki-e taiki-e added this to the v0.4 milestone May 24, 2020
@taiki-e taiki-e changed the title Should --each-feature and --feature-powerset include --all-features? Should --each-feature and --feature-powerset run --all-features? May 24, 2020
@taiki-e taiki-e removed the C-enhancement Category: A new feature or an improvement for an existing one label Jun 4, 2020
@taiki-e
Copy link
Owner Author

taiki-e commented Jun 5, 2020

It would make sense for --feature-powerset to include this, because --all-features is one of the combinations of features. (I'm not sure about --each-feature should do this or not.)

Also, if --skip is passed, then --all-features should be skipped.

@taiki-e
Copy link
Owner Author

taiki-e commented Jul 11, 2020

Also, if --skip is passed, then --all-features should be skipped.

Nah, it's not clear how this should interact with #48.

@taiki-e taiki-e modified the milestones: v0.4, v0.5 Jul 11, 2020
@taiki-e taiki-e added the A-features Area: features (--feature-powerset, --each-feature, etc.) label Oct 10, 2020
@taiki-e taiki-e mentioned this issue Oct 16, 2020
Closed
@taiki-e
Copy link
Owner Author

taiki-e commented Oct 16, 2020

I prefer to do this on both flags and to add --skip-all-features flag, as said in #48.

@taiki-e taiki-e removed this from the v0.5 milestone Oct 16, 2020
@taiki-e taiki-e self-assigned this Oct 16, 2020
@taiki-e taiki-e added this to the v0.4 milestone Oct 16, 2020
@taiki-e taiki-e mentioned this issue Oct 16, 2020
Merged
@bors bors bot closed this as completed in 1e79699 Oct 16, 2020
@jhpratt
Copy link
Contributor

jhpratt commented Oct 24, 2020

With regard to all-features, why does this need to be included, given that it's inherently already part of the powerset? Likewise for no-default-features. I found the previous behavior odd, and was surprised to learn that it was expanded.

@taiki-e
Copy link
Owner Author

taiki-e commented Oct 24, 2020

@jhpratt I don't remember if it is exactly the same at this time, but all-features will behave differently than a normal combination of features, at least in the future (rust-lang/cargo#8799).
For no-default-feature, should be fixed in #77 (previous behavior is wrong).

@jhpratt
Copy link
Contributor

jhpratt commented Oct 24, 2020

Neat. I didn't realize namespaced features were progressing. Thanks for the info.

@taiki-e
Copy link
Owner Author

taiki-e commented Nov 3, 2020 •
edited
Loading

That said, I think #61 implemented this in a somewhat wrong way.
Probably --all-features should not be included in the combination if some features have already excluded by --exclude-features (--skip). (I didn't notice this behavior until I actually encountered a CI error on the crossbeam repo.)

UPDATE: fixed in 0.4.2

@taiki-e taiki-e mentioned this issue Nov 3, 2020
Merged
bors bot added a commit that referenced this issue Nov 3, 2020
@bors @taiki-e
Merge #86
875f6e4 
86: Do not include --all-features in feature combination if --exclude-features passed r=taiki-e a=taiki-e

See #42 (comment)

Co-authored-by: Taiki Endo <[email protected]>
@taiki-e taiki-e mentioned this issue Dec 29, 2021
Merged
bors bot added a commit that referenced this issue Dec 29, 2021
@bors @taiki-e
Merge #141
7260fc3 
141: Do not add --all-features to combinations if it is already covered by another combination r=taiki-e a=taiki-e

This fixes an issue where `--feature-powerset` and `--each-feature` add `--all-features` as one of the combinations, even if it is already covered by another combination. (The initial work on this fix was included in #140.)

Hopefully, the new behavior should be explained almost exactly in the long help message.

https://github.com/taiki-e/cargo-hack/blob/4234b37e670368067b64dba249ac41616fe2da85/tests/long-help.txt#L30-L37

https://github.com/taiki-e/cargo-hack/blob/4234b37e670368067b64dba249ac41616fe2da85/tests/long-help.txt#L39-L47

Follow-up: #42

Co-authored-by: Taiki Endo <[email protected]>
@taiki-e
Copy link
Owner Author

taiki-e commented Dec 29, 2021

I think #141 (published in 0.5.9) has fixed the rest of the problems here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@taiki-e taiki-e

Labels
A-features Area: features (--feature-powerset, --each-feature, etc.)
Projects
None yet
Milestone
v0.4
Development

Successfully merging a pull request may close this issue.

Treat --all-features flag as one of feature combinations taiki-e/cargo-hack
2 participants
@jhpratt @taiki-e