-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Skip all cargo fix
that tends to write to registry cache.
#9938
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
46bfbc1
to
27c0651
Compare
Note that unfortunately the warning still points to file of dependencies, which is wrong 😞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me!
@ehuss mind also taking a look though? You're more in the thick-of-things with cargo fix
than I am.
tests/testsuite/fix.rs
Outdated
@@ -1791,3 +1791,58 @@ fn non_edition_lint_migration() { | |||
// Check that it made the edition migration. | |||
assert!(contents.contains("from_utf8(crate::foo::FOO)")); | |||
} | |||
|
|||
// This test will break after rust-lang/rust#88514 being fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the test mostly just assert that nothing is fixed, which means that this wouldn't break when rust-lang/rust is updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Updated. Thank for the review 😆
27c0651
to
0565daf
Compare
Thanks! Looks good. I have two minor concerns, but I don't think they are too important:
@bors r+ |
📌 Commit 0565daf has been approved by |
☀️ Test successful - checks-actions |
Update cargo 7 commits in d56b42c549dbb7e7d0f712c51b39400260d114d4..c7957a74bdcf3b11e7154c1a9401735f23ebd484 2021-09-27 13:44:18 +0000 to 2021-10-11 20:17:07 +0000 - Add some more information to verbose version. (rust-lang/cargo#9968) - Skip all `cargo fix` that tends to write to registry cache. (rust-lang/cargo#9938) - Stabilize named profiles (rust-lang/cargo#9943) - Update git2 (rust-lang/cargo#9963) - Distinguish lockfile version req from normal dep in resolver error message (rust-lang/cargo#9847) - nit: Allocated slightly bigger vec than needed (rust-lang/cargo#9954) - Add shell completion for shorthand commands (rust-lang/cargo#9951)
Fix fix::fix_in_dependency to not rely on rustc This changes the `fix::fix_in_dependency` test so that it does not rely on the behavior of rustc. This test is checking the behavior when rustc includes a suggestion diagnostic that modifies a file in CARGO_HOME from a dependency. rustc should not be emitting suggestions that point outside of the current crate, but there are some known bugs where it does this. #9938 added a workaround for this to avoid writing to CARGO_HOME. However, the current test was relying on one of those bugs in rustc to exhibit its behavior. rust-lang/rust#119204 is fixing that particular behavior. Instead of relying on issues in rustc, this PR changes the test so that the test creates a fake `rustc` executable that will emit a hard-coded JSON message that tells `cargo fix` to modify a file in CARGO_HOME. This should be stable as it is no longer relying on rustc. Testing or validating this change is a little tricky. You have to essentially comment out [these three lines](https://github.com/rust-lang/cargo/blob/4f70d1781a2b2d3611087e1ee6e8096903c9b73a/src/cargo/ops/fix.rs#L654-L656) from the original #9938 change, and verify that the test fails (it says "Fixed" in the output). With those three lines in place, it should pass.
Skip all
cargo fix
that tends to write to registry cache.This is a temporary hack for #9857. The real fix may need to touch rustc.