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

Add report if cargo fix --edition changes features. #9268

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Mar 15, 2021

This adds a small report if using cargo fix --edition to transition from 2018 to 2021 to determine if the new resolver will result in different features.

There is a gauntlet of checks that must pass before it even tries to show the differences:

  • If the resolver is already specified, skip.
  • If in a virtual workspace, skip (no way to specify global edition).
  • If not migrating the root package, skip. The edition only changes the resolver in the root package.
  • If not migrating all targets, skip. It's uncertain if the user intends to set the package edition or not.

Closes #9048

@rust-highfive
Copy link

r? @alexcrichton

(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 Mar 15, 2021
@ehuss
Copy link
Contributor Author

ehuss commented Mar 15, 2021

Overall I'm not too happy with this. The code seems a little awkward, though I can't say specifically why.

I am a little concerned that it is unreliable. Another concern is that the new resolver depends on many factors (the target, the --features flags used, and whether or not dev-dependencies are used). This report only captures a small snapshot of that (whatever settings the user provided to cargo fix).

Also, this doesn't actually run the final verification step with the new resolver. That will require significant changes to cargo fix, which I'm not sure how feasible that is.

I wanted to open this up for discussion to see if this is the direction we should go.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This looks good to me, personally. Given the context of this, migrating between editions, I'd expect the code to be lossy, a bit odd, and possibly very specific to some situations. Also I would not expect 100% correctness guarantees from this code (just no false positive warnings).

I view the cargo fix --edition warnings as "best effort" where we do what we can for the 90% use case in theory, but there's likely to always be something that slips through the cracks or just isn't worth detecting and warning about.

The only thing I found surprising here was the check for --all-targets, was that necessary to reduce the warnings emitted?

&opts.compile_opts.features,
opts.compile_opts.all_features,
!opts.compile_opts.no_default_features,
),
Copy link
Member

Choose a reason for hiding this comment

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

Drive-by comment not related to your PR, but this seems like it's ripe for a opts.copmile_opts.requested_features() method

@ehuss
Copy link
Contributor Author

ehuss commented Mar 15, 2021

The only thing I found surprising here was the check for --all-targets, was that necessary to reduce the warnings emitted?

My thinking was that if you run cargo fix --edition --bin foo to incrementally migrate a package, you're intending to set edition on the bin target. I can probably remove that check, since I think that is a highly unlikely use case, and probably doesn't hurt to report differences.

@ehuss ehuss force-pushed the 2021-fix-edition-v2-warning branch from 3093968 to 6c69e9d Compare March 16, 2021 18:42
@alexcrichton
Copy link
Member

Want to go ahead and land this? We can presumably in the near-ish future try testing out cargo fix --edition for existing projects to see how it goes?

@ehuss
Copy link
Contributor Author

ehuss commented Mar 16, 2021

Yea, I think it's ok to go. It can always be tweaked or removed. Hopefully we'll get enough people to test it before 2021 is stable. I'll assume that was an approval.

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 16, 2021

📌 Commit 6c69e9d has been approved by alexcrichton

@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 Mar 16, 2021
@bors
Copy link
Contributor

bors commented Mar 16, 2021

⌛ Testing commit 6c69e9d with merge 90691f2...

@bors
Copy link
Contributor

bors commented Mar 16, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 90691f2 to master...

@bors bors merged commit 90691f2 into rust-lang:master Mar 16, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 17, 2021
Update cargo

8 commits in 32da9eaa5de5be241cf8096ca6b749a157194f77..90691f2bfe9a50291a98983b1ed2feab51d5ca55
2021-03-13 01:18:40 +0000 to 2021-03-16 21:36:55 +0000
- Add report if `cargo fix --edition` changes features. (rust-lang/cargo#9268)
- Fix --feature pkg/feat for V1 resolver for non-member. (rust-lang/cargo#9275)
- Fix doc duplicate removal of root units. (rust-lang/cargo#9276)
- Add CLI help text for patch-in-config (rust-lang/cargo#9271)
- Document `-Zpatch-in-config` (rust-lang/cargo#9270)
- Support [patch] in .cargo/config files (rust-lang/cargo#9204)
- Add `--future-incompat-report` support to `cargo test` (rust-lang/cargo#9264)
- 🍱 Crop favicon (rust-lang/cargo#9262)
@ehuss ehuss added this to the 1.52.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Prepare Cargo for resolver = "2" as the default in the 2021 edition
4 participants