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

Fix bug when with resolver = "1" non-virtual package was allowing unknown features #9437

Merged
merged 12 commits into from
May 24, 2021
Merged

Fix bug when with resolver = "1" non-virtual package was allowing unknown features #9437

merged 12 commits into from
May 24, 2021

Conversation

In-line
Copy link
Contributor

@In-line In-line commented Apr 30, 2021

No description provided.

@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2021
@In-line
Copy link
Contributor Author

In-line commented Apr 30, 2021

I accidently found, that if I remove resolver = "2" from this test if fails. It seems that doing cargo build --features package_name/some-nonsense is acceptable by cargo

@In-line
Copy link
Contributor Author

In-line commented Apr 30, 2021

Output with cargo ea0b5ca

thread 'package_features::virtual_typo_member_feature_default_resolver' panicked at '
Expected: execs
    but: differences:
  0 - |[ERROR] none of the selected packages contains these features: a/deny-warning|
    + |error: package `a v0.1.0 (/home/alik/Documents/projects/cargo/target/cit/t0/foo)` does not have a dependency named `a`|

But with d087aeb

thread 'package_features::virtual_typo_member_feature_default_resolver' panicked at '
Expected: execs
    but: exited with exit code: 0
--- stdout

--- stderr

@In-line In-line mentioned this pull request Apr 30, 2021
Merged
@ehuss
Copy link
Contributor

ehuss commented Apr 30, 2021

Hm, looks like that slipped through. Would you be interested in trying to fix it?

@In-line
Copy link
Contributor Author

In-line commented May 3, 2021

@ehuss I will be looking to it.

@In-line In-line changed the title Add failing test Fix bug when with resolver = "1" non-virtual package was allowing unknown features May 3, 2021
In-line
In-line commented May 3, 2021
View reviewed changes
src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@In-line
Copy link
Contributor Author

In-line commented May 3, 2021

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned alexcrichton May 3, 2021
ehuss
ehuss reviewed May 7, 2021
View reviewed changes
src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented May 10, 2021

Hm, so I'm a bit uncertain about the approach here. If I understand correctly, the main concern is that with --features foo/featname, and foo is the "current" package, then featname is more or less ignored. Would it be possible to change it so that in that scenario, the feature is added to cwd_features instead? That should pass the feature to the normal machinery for checking if the feature exists. Then, the check for !member_specific_features.is_empty() can be treated more as a bug, since the two loops should end up matching one another.

@In-line
Copy link
Contributor Author

In-line commented May 13, 2021

@ehuss do you have any more ideas for test cases?

@ehuss
Copy link
Contributor

ehuss commented May 24, 2021

Thanks!

I think the tests are good enough, I can't think of anything specific to check.

I pushed a small change to remove masquerade_as_nightly_cargo which wasn't needed (it was an accident it wasn't removed in the other test).

@bors r+

@bors
Copy link
Collaborator

bors commented May 24, 2021

📌 Commit 61b762b has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2021
@bors
Copy link
Collaborator

bors commented May 24, 2021

⌛ Testing commit 61b762b with merge 07340c9...

@bors
Copy link
Collaborator

bors commented May 24, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 07340c9 to master...

@bors bors merged commit 07340c9 into rust-lang:master May 24, 2021
@In-line In-line deleted the unknown-feature-resolver-1 branch May 25, 2021 11:24
@ehuss ehuss mentioned this pull request May 25, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 26, 2021
@JohnTitor
Rollup merge of rust-lang#85701 - ehuss:update-cargo, r=ehuss
6582cf5 
Update cargo

7 commits in 070e459c2d8b79c5b2ac5218064e7603329c92ae..e931e4796b61de593aa1097649445e535c9c7ee0
2021-05-11 18:12:23 +0000 to 2021-05-24 16:17:27 +0000
- Add `cargo:rustc-link-arg-bin` flag. (rust-lang/cargo#9486)
- Add a cargo-doc.browser config option (rust-lang/cargo#9473)
- Fix bug when with resolver = "1" non-virtual package was allowing unknown features (rust-lang/cargo#9437)
- Add GitHub link to contributor guide. (rust-lang/cargo#9493)
- Add temporary fix for rustup on windows in CI. (rust-lang/cargo#9498)
- 3 typos and some capitalization (rust-lang/cargo#9495)
- fix 6 typos (rust-lang/cargo#9484)
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 26, 2021
@JohnTitor
Rollup merge of rust-lang#85701 - ehuss:update-cargo, r=ehuss
7c0677a 
Update cargo

7 commits in 070e459c2d8b79c5b2ac5218064e7603329c92ae..e931e4796b61de593aa1097649445e535c9c7ee0
2021-05-11 18:12:23 +0000 to 2021-05-24 16:17:27 +0000
- Add `cargo:rustc-link-arg-bin` flag. (rust-lang/cargo#9486)
- Add a cargo-doc.browser config option (rust-lang/cargo#9473)
- Fix bug when with resolver = "1" non-virtual package was allowing unknown features (rust-lang/cargo#9437)
- Add GitHub link to contributor guide. (rust-lang/cargo#9493)
- Add temporary fix for rustup on windows in CI. (rust-lang/cargo#9498)
- 3 typos and some capitalization (rust-lang/cargo#9495)
- fix 6 typos (rust-lang/cargo#9484)
@ehuss ehuss added this to the 1.54.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
5 participants
@In-line @rust-highfive @ehuss @bors @alexcrichton