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(fix): Address problems with implicit -> explicit feature migration #14018

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Jun 5, 2024

What does this PR try to resolve?

Within the scope of cargo fix there are two problems

  • We wipe out existing feature activations if it has the same name as an optional dependency
  • The Cargo.toml isn't parseable because the unused optional dependency won't "exist" if just dep_name/feature_name is used

Fixes #14010

How should we test and review this PR?

As for the unused optional dependency not "existing" error,

Depending on what solution we go with for #14016, we might want to revisit the second migration within this PR. This is one reason I made the commit separate (in addition to just making it clearer whats happening as this gets into some finer details of features).

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Jun 5, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2024
Comment on lines +485 to +487
let Some(activations) = activations.as_array_mut() else {
let _ = gctx.shell().warn(format_args!("skipping fix of feature `{feature_name}` in package `{}`: unsupported feature schema", pkg.name()));
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check was written with rust-lang/rfcs#3416 in mind

That syntax is likely to work on any Edition and if we forget to touch this code, I'd prefer to make that known so people can report it to us and we can fix it rather than silently ignoring it.

The question that remains is to warn or error. I figured we shouldn't block the user from getting other fixes for something they can't workaround in a reasonable way.

@epage epage force-pushed the dep_name branch 4 times, most recently from 8a02016 to 21c0928 Compare June 10, 2024 17:54
@ehuss
Copy link
Contributor

ehuss commented Jun 17, 2024

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 17, 2024

📌 Commit 21c0928 has been approved by ehuss

It is now in the queue for this repository.

@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 Jun 17, 2024
@bors
Copy link
Collaborator

bors commented Jun 17, 2024

⌛ Testing commit 21c0928 with merge 83a3964...

@bors
Copy link
Collaborator

bors commented Jun 17, 2024

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 83a3964 to master...

@bors bors merged commit 83a3964 into rust-lang:master Jun 17, 2024
22 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Update cargo

13 commits in a1f47ec3f7cd076986f1bfcd7061f2e8cb1a726e..3ed207e416fb2f678a40cc79c02dcf4f936a21ce
2024-06-15 01:10:07 +0000 to 2024-06-18 19:18:22 +0000
- test: prefer raw string for regex reduction (rust-lang/cargo#14099)
- test: migrate tree and tree_graph_features to snapbox (rust-lang/cargo#14094)
- test: Migrate some files to snapbox (rust-lang/cargo#14069)
- remove some legacy public dependency code from the resolver (rust-lang/cargo#14090)
- fix(fix): Address problems with implicit -> explicit feature migration (rust-lang/cargo#14018)
- refactor: 1.79 cleanup (rust-lang/cargo#14088)
- test: migrate `git_(gc|shallow)` to snapbox (rust-lang/cargo#14087)
- test: migrate timings_works to snapbox (rust-lang/cargo#14082)
- test: migrate minimal_versions to snapbox (rust-lang/cargo#14080)
- Remove `run_expect_error` to avoid tests incorrectly passing (rust-lang/cargo#14078)
- test: migrate help to snapbox (rust-lang/cargo#14060)
- test: Migrate tests/testsuite/co*.rs to snapbox (rust-lang/cargo#14079)
- Use `std::fs::absolute` instead of reimplementing it (rust-lang/cargo#14075)

<!--
r? ghost
-->
@rustbot rustbot added this to the 1.81.0 milestone Jun 19, 2024
@epage epage deleted the dep_name branch June 19, 2024 19:46
epage added a commit to epage/cargo that referenced this pull request Jul 9, 2024
This doesn't revert the last commit of rust-lang#14018 so that it works properly
on Editions 2021 and 2024.

Fixes rust-lang#14016
epage added a commit to epage/cargo that referenced this pull request Jul 10, 2024
This doesn't revert the last commit of rust-lang#14018 so that it works properly
on Editions 2021 and 2024.

Fixes rust-lang#14016
epage added a commit to epage/cargo that referenced this pull request Jul 10, 2024
This doesn't revert the last commit of rust-lang#14018 so that it works properly
on Editions 2021 and 2024.

Fixes rust-lang#14016
bors added a commit that referenced this pull request Jul 10, 2024
fix: Ensure dep/feature activates the dependency on 2024

### What does this PR try to resolve?

Fixes #14016

### How should we test and review this PR?

This doesn't revert the last commit of #14018 so that it works properly
on Editions 2021 and 2024.

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2024 implicit feature migration deletes dependency features
5 participants