Always rebuild targets when using cargo-fix #5750
Conversation
|
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. |
src/cargo/ops/fix.rs
Outdated
| ws: &Workspace<'a>, | ||
| options: &CompileOptions<'a>, | ||
| ) -> CargoResult<()> { | ||
| // Begin cargo cult from `ops::cargo_compile::compile_ws` |
There was a problem hiding this comment.
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 :)
|
We do something similar for Perhaps we can add a |
|
I just noted that |
|
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? |
|
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
force-pushed
the
rustfix-likes-to-touch-files
branch
from
5dc17de
to
1eacc05
Compare
| @@ -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 { | |||
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh, damn, I copied the wrong example! Thank you!
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.
|
ping @killercup, have you had a chance to take another look at this? |
|
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>
.
|
killercup
changed the title
|
I'm gonna close this for now to help clear out the queue, but always up for merging a rebase :) |
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?
Assume you run
cargo fixand only get warnings that cannot be fixed.Then, without changing the code, you run
cargo fixagain. The compilerwill see that no files have changed and output… nothing. This is very
confusing to the end user.
To mitigate this, we now
touchthe entry point files to trickrustc/cargo into emitting the same warnings again.
This is a fix but a very "WIP" one for #5736