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

Fix erroring when explicitly using "--features default" without specifying one in Cargo.toml #8220

Closed
wants to merge 1 commit into from

Conversation

hbina
Copy link
Contributor

@hbina hbina commented May 7, 2020

Also added a regression test for this.

This will likely cause a problem if a crate specify a "default" feature.

Related: #8164

Edit: This solutions feels clumsy at best xD

@rust-highfive
Copy link

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2020
@hbina hbina changed the title Fix erroring when explicitly using --features default Fix erroring when explicitly using "--features default" without specifying one in Cargo.toml May 7, 2020
@@ -265,6 +265,8 @@ pub fn resolve_features<'b>(
let reqs = build_requirements(s, opts)?;
let mut ret = Vec::new();
let mut used_features = HashSet::new();
// Add `default` as a feature that must exist.
used_features.insert("default".into());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the only read of used_features is below at .filter(|s| !used_features.contains(s)).
Std::HashSet only allocates when something is added, and this set is only used if there are enabled optional dependencies. So maybe if we add a .filter(|s| s != "default") then we can avoid the allocation here.
(yes, I know, this is micro optimizing when not on the hot path.)

@Eh2406
Copy link
Contributor

Eh2406 commented May 13, 2020

The CI problems were fixed by rerunning. I added a micro optimizing nit.
I know that @alexcrichton wanted to say something about whether we should do this.
If we decide to go ahead, I'd want to double check if there are other test we should add. Like does it work with the nightly resolve=2 flag.

@bors
Copy link
Collaborator

bors commented Jun 22, 2020

☔ The latest upstream changes (presumably #8395) made this pull request unmergeable. Please resolve the merge conflicts.

Also added a regression test for this.
@hbina hbina marked this pull request as ready for review June 23, 2020 20:22
@ehuss
Copy link
Contributor

ehuss commented Jan 13, 2021

Closing as this as become somewhat stale, and per #8164 (comment) it is not really clear that we want to change the behavior here.

@ehuss ehuss closed this Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants