Always rebuild targets when using cargo-fix #5750

killercup · 2018-07-20T10:18:47Z

Assume you run cargo fix and only get warnings that cannot be fixed.
Then, without changing the code, you run cargo fix again. The compiler
will see that no files have changed and output… nothing. This is very
confusing to the end user.

To mitigate this, we now touch the entry point files to trick
rustc/cargo into emitting the same warnings again.

This is a fix but a very "WIP" one for #5736

rust-highfive · 2018-07-20T10:18:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

killercup · 2018-07-20T10:20:05Z

src/cargo/ops/fix.rs

+    ws: &Workspace<'a>,
+    options: &CompileOptions<'a>,
+) -> CargoResult<()> {
+    // Begin cargo cult from `ops::cargo_compile::compile_ws`


I've only pasted this code here because I couldn't find a quick way to get the compilation target entry-point files. I'd very happily change this :)

matklad · 2018-07-20T10:29:01Z

We do something similar for --build-plan which also needs to compile everything regardless of freshness: https://github.com/rust-lang/cargo/pull/5301/files#diff-36216dee2e5190f6c670787ea469e00aR97

Perhaps we can add a force_rebuild: bool flag to cover both of these use-cases

Eh2406 · 2018-07-20T19:49:14Z

I just noted that cargo clippy has the same problematic behavior.

alexcrichton · 2018-07-21T15:49:31Z

Thanks for the PR @killercup! I agree though with @matklad that we probably want to avoid this duplication and plumb a boolean to the backend which forces rebuild and forces busting the cache. Would that be possible to implement?

Could you also add a test where there's a few top-level targets like an integration and a binary as well as a library?

killercup · 2018-07-21T16:36:36Z

Absolutely. I'll try to get around to this by Monday evening. (I might ask some more questions by then, too)

…

On Sat, 21 Jul 2018, 17:49 Alex Crichton, ***@***.***> wrote: Thanks for the PR @killercup <https://github.com/killercup>! I agree though with @matklad <https://github.com/matklad> that we probably want to avoid this duplication and plumb a boolean to the backend which forces rebuild and forces busting the cache. Would that be possible to implement? Could you also add a test where there's a few top-level targets like an integration and a binary as well as a library? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5750 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX_4DqTqyjVVieejsVCPzboojg2XNks5uI02OgaJpZM4VXzJf> .

Assume you run `cargo fix` and only get warnings that cannot be fixed. Then, without changing the code, you run `cargo fix` again. The compiler will see that no files have changed and output… nothing. This is very confusing to the end user. To mitigate this, we now `touch` the entry point files to trick rustc/cargo into emitting the same warnings again. This is a fix but a very "WIP" one for rust-lang#5736

And use it for cargo-fix

Sadly, this doesn't seem to work right now

killercup · 2018-07-24T13:00:24Z

src/cargo/core/compiler/mod.rs

@@ -133,7 +135,7 @@ fn compile<'a, 'cfg: 'a>(
        let dirty = work.then(link_targets(cx, unit, false)?).then(dirty);
        let fresh = link_targets(cx, unit, true)?.then(fresh);

-        if exec.force_rebuild(unit) {
+        if exec.force_rebuild(unit) || force_rebuild {


@matklad I've not added a force_rebuild field to the BuildConfig struct, and from what I can tell this should be the right place to use it. Indeed, when I add a debug!("rebuilding {:?} (force? {:?})", unit, force_rebuild); here, my new test case triggers it and shows the correct data. But I don't get any diagnostics output! Do I… need to go deeper?

I think it's working. I think the issue is that the second call to cargo fix won't display "fixing" because the files have already been fixed in the first call.

Ahh, damn, I copied the wrong example! Thank you!

alexcrichton

Thanks! I think though that the force_rebuild option here may be a bit too aggressive though? It forces a rebuild of the entire crate graph I think, right? Could this perhaps only be limited to a few packages selected by -p on the command line?

Turns out that using test cases that we can actually fix was not a good way to test that warnings continue to show up cross runs.

alexcrichton · 2018-07-30T22:11:23Z

ping @killercup, have you had a chance to take another look at this?

killercup · 2018-07-31T06:14:06Z

Not yet, spent last the last few days writing CLI docs. Will try to get around to this by this evening

…

On Tue, 31 Jul 2018, 00:11 Alex Crichton, ***@***.***> wrote: ping @killercup <https://github.com/killercup>, have you had a chance to take another look at this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5750 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX2CVRD1Lmno4Pcc2yhevGzjE3Nwiks5uL4SOgaJpZM4VXzJf> .

alexcrichton · 2018-08-23T18:56:20Z

I'm gonna close this for now to help clear out the queue, but always up for merging a rebase :)

@killercup

Force `cargo fix` to rebuild Fixes #5736 This is a resubmit of @killercup's #5750, rebased on current master. @alexcrichton From browsing the code I feel like `-p` would still restrict the packages to rebuild, despite the rebuild flag added. But I might be misreading or not-fully-reading the code. Could you give me some mentoring instructions for the test cases you're concerned with?

killercup requested review from alexcrichton and matklad July 20, 2018 10:18

rust-highfive assigned alexcrichton Jul 20, 2018

killercup commented Jul 20, 2018

View reviewed changes

killercup added 5 commits July 24, 2018 14:36

Fix typo in cargo fix --help

e72f9d1

Remove file-touching workaround

76f872e

Introduce force_rebuild flag in BuildConfig

d858bf4

And use it for cargo-fix

cargo-fix: test with multiple target rebuilds

1eacc05

Sadly, this doesn't seem to work right now

killercup force-pushed the rustfix-likes-to-touch-files branch from 5dc17de to 1eacc05 Compare July 24, 2018 12:40

killercup commented Jul 24, 2018

View reviewed changes

Update tests after rebase

f29903c

alexcrichton reviewed Jul 24, 2018

View reviewed changes

Fix Pascal's stupidity

a54a6cb

Turns out that using test cases that we can actually fix was not a good way to test that warnings continue to show up cross runs.

dwijnand mentioned this pull request Jul 25, 2018

force full re-clippy rust-lang/rust-clippy#2604

Closed

killercup changed the title ~~Touch entry-point files before running cargo-fix~~ Always rebuild targets when using cargo-fix Jul 31, 2018

alexcrichton closed this Aug 23, 2018

dwijnand mentioned this pull request Aug 29, 2018

Force cargo fix to rebuild #5944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always rebuild targets when using cargo-fix #5750

Always rebuild targets when using cargo-fix #5750

killercup commented Jul 20, 2018

rust-highfive commented Jul 20, 2018

killercup Jul 20, 2018

matklad commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

alexcrichton commented Jul 21, 2018

killercup commented Jul 21, 2018 via email

killercup Jul 24, 2018

ehuss Jul 24, 2018

killercup Jul 24, 2018

alexcrichton left a comment

alexcrichton commented Jul 30, 2018

killercup commented Jul 31, 2018 via email

alexcrichton commented Aug 23, 2018

Always rebuild targets when using cargo-fix #5750

Always rebuild targets when using cargo-fix #5750

Conversation

killercup commented Jul 20, 2018

rust-highfive commented Jul 20, 2018

killercup Jul 20, 2018

Choose a reason for hiding this comment

matklad commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

alexcrichton commented Jul 21, 2018

killercup commented Jul 21, 2018 via email

killercup Jul 24, 2018

Choose a reason for hiding this comment

ehuss Jul 24, 2018

Choose a reason for hiding this comment

killercup Jul 24, 2018

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton commented Jul 30, 2018

killercup commented Jul 31, 2018 via email

alexcrichton commented Aug 23, 2018