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

Force cargo fix to rebuild #5944

Merged
merged 5 commits into from
Aug 31, 2018
Merged

Conversation

dwijnand
Copy link
Member

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?

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Ah yeah sure! I believe the pathological behavior can be seen by adding a dependency and I believe both invocations (in the test) will recompile the dependency, but only the requested crate (the one we're fixing) should be recompiled

@dwijnand
Copy link
Member Author

Thanks @alexcrichton I was able to come up with a test. It fails with this PR because it aggressively rebuilds and fails without this PR because it doesn't rebuild anything. 😄

Now on how to fix the problem..

@alexcrichton
Copy link
Member

Oh interesting, looks like CI passed? I personally prefer to use with_stderr as it's a more exhaustive match which may help?

@dwijnand
Copy link
Member Author

Yep, I think this is fixed/done now! Good shout on using with_stderr, looks good now.

@dwijnand
Copy link
Member Author

My friend path::deep_dependencies_trigger_rebuild

@dwijnand dwijnand closed this Aug 30, 2018
@dwijnand dwijnand reopened this Aug 30, 2018
@alexcrichton
Copy link
Member

@bors: r+

Clever!

@bors
Copy link
Contributor

bors commented Aug 31, 2018

📌 Commit 7a42790 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 31, 2018

⌛ Testing commit 7a42790 with merge aecaef3...

bors added a commit that referenced this pull request Aug 31, 2018
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?
@bors
Copy link
Contributor

bors commented Aug 31, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing aecaef3 to master...

@bors bors merged commit 7a42790 into rust-lang:master Aug 31, 2018
@dwijnand dwijnand deleted the fix-force-rebuild branch September 1, 2018 04:03
@ehuss ehuss added this to the 1.30.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out how to force cargo fix to run
6 participants