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.

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

Intelligently exclude redundant items in powerset #81

Closed
jhpratt opened this issue Oct 24, 2020 · 9 comments · Fixed by #85
Closed

Intelligently exclude redundant items in powerset #81

jhpratt opened this issue Oct 24, 2020 · 9 comments · Fixed by #85
Assignees
Labels
A-features Area: features (--feature-powerset, --each-feature, etc.) C-enhancement Category: A new feature or an improvement for an existing one

Comments

@jhpratt
Copy link
Contributor

jhpratt commented Oct 24, 2020

Consider the following case (the actual crate doesn't matter; I just tested it on an empty crate).

[features]
std = ["alloc"]
alloc = []

I then perform cargo hack check --feature-powerset --exclude-all-features (I've inquired about true necessity of the latter flag elsewhere). This is the output:

info: running `cargo check --no-default-features` on tmp (1/4)
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
info: running `cargo check --no-default-features --features alloc` on tmp (2/4)
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
info: running `cargo check --no-default-features --features std` on tmp (3/4)
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
info: running `cargo check --no-default-features --features alloc,std` on tmp (4/4)
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

Presumably it would be possible to determine from Cargo.toml that std relies on alloc, and as such alloc,std is redundant (because std is already in the powerset).

@taiki-e taiki-e added A-features Area: features (--feature-powerset, --each-feature, etc.) C-enhancement Category: A new feature or an improvement for an existing one labels Oct 24, 2020
@taiki-e taiki-e self-assigned this Nov 10, 2020
@bors bors bot closed this as completed in 9453db9 Nov 28, 2020
@taiki-e
Copy link
Owner

taiki-e commented Nov 30, 2020

Published in 0.4.6.

@jhpratt
Copy link
Contributor Author

jhpratt commented Nov 30, 2020

Looks good! By no means perfect, but it's still a significant improvement.

@jhpratt
Copy link
Contributor Author

jhpratt commented Nov 30, 2020

How much can I push this? 😛 I wasn't even expecting this to be implemented for quite a while, to the point I debated whether to even file the issue to begin with! It was more "this is theoretically possible" initially. CI is already relatively fast for me, as using existing flags in addition to this change has significantly cut the size of the powerset. I know you work on some projects that are larger than mine, so I suppose it depends on whether you're happy with your CI times.

The time crate currently has the following (effectively, I've simplified a bit)

[dependencies]
const_fn = "0.4.3"
quickcheck-dep = { package = "quickcheck", version = "0.9.2", optional = true }
rand = { version = "0.7.3", optional = true }
serde = { version = "1.0.117", optional = true }
time-macros = { version = "=0.2.0-optional = true }

[target.'cfg(windows)'.dependencies]
winapi = "0.3.9"

Currently there is one unnecessary check that I see that isn't possible to work around with existing flags:

cargo check --no-default-features --features serde,rand,quickcheck
cargo check --no-default-features --features std,serde,rand,quickcheck

As I said, this isn't an issue for me personally; it's just an observation.

My naïve thought is to effectively brute force the minimal powerset. A forest of initially identical graphs could be created, pruning each as necessary. I might write a proof of concept to generate a minimal powerset at some point. There's probably some algorithm to this effect, but I'm not aware of anything in particular. Of course, this is all assuming equal weight to each dependency, but that's something that certainly isn't worth the effort.

Thank you for your work, of course! Just throwing some things out there.

@taiki-e
Copy link
Owner

taiki-e commented Dec 1, 2020

Currently there is one unnecessary check that I see that isn't possible to work around with existing flags:

cargo check --no-default-features --features serde,rand,quickcheck
cargo check --no-default-features --features std,serde,rand,quickcheck

Note that technically you can write code that will cause the compile to fail if these features are used together.
#85 excludes the perfect equivalent based on how the cargo features works. On the other hand, this probably requires something like a heuristic that excludes feature combinations based on how cfg is actually used. (I think it is probably very hard to implement it to work properly.)

I think a reasonable (and currently available) approach for crates with many features is the option that limits the max number of simultaneous feature flags of --feature-powerset (proposed in #58 and implemented as --depth in #59).

@jhpratt
Copy link
Contributor Author

jhpratt commented Dec 1, 2020

Whoops, just realized that I didn't include the list of features in that comment. quickcheck depends on std in that example. If it didn't, it would naturally be possible to break.

@taiki-e
Copy link
Owner

taiki-e commented Dec 1, 2020

Ah, I saw what's happening in time, but it doesn't seem to work well when used with --group-features. (It's definitely a bug.)

@jhpratt
Copy link
Contributor Author

jhpratt commented Dec 1, 2020

That'll do it. I actually need to ungroup quickcheck regardless, as I recently realized that it means I'm not covering a plausible scenario.

@taiki-e
Copy link
Owner

taiki-e commented Dec 1, 2020

Given that --group-features is something like a user-defined heuristic and users need to manually check that the grouped features are being used correctly.
If it considered to difficult to use --group-features properly, it may be preferable to deprecate it in favor of using --depth to limit feature combinations.

FYI: filed #97 to track that bug.

@taiki-e
Copy link
Owner

taiki-e commented Dec 3, 2020

--group-features bug has been fixed in 0.4.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features Area: features (--feature-powerset, --each-feature, etc.) C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants