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

Fix fix_path_deps test error. #6236

Merged
merged 1 commit into from
Oct 31, 2018
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 29, 2018

No description provided.

@rust-highfive
Copy link

r? @matklad

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

@ehuss
Copy link
Contributor Author

ehuss commented Oct 29, 2018

This test has been failing a fair bit:

https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/17275967/job/a3g5pmwknyk7xiiq
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18375223/job/dt34b41jngxgqd3d
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18377838/job/vw9srbgwl6eygjpo
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18525511/job/tj5otfhhf01938j1
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/18558150/job/l69ggfmdrs74x44y
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/19478171/job/wpyw1vy3n50vngxb
https://ci.appveyor.com/project/rust-lang-libs/cargo/builds/19890689/job/xna48q6vt8moh2vi

I can't repro locally, but this should work. There's a little race between which side says Checking vs Fixing first. An alternate solution might be to print Checking before spawning the thread like this:

diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs
index 1bb654d1..5ee957d7 100644
--- a/src/cargo/core/compiler/job_queue.rs
+++ b/src/cargo/core/compiler/job_queue.rs
@@ -400,6 +400,12 @@ impl<'a> JobQueue<'a> {
             let res = job.run(fresh, &JobState { tx: my_tx.clone() });
             my_tx.send(Message::Finish(key, res)).unwrap();
         };
+
+        if !build_plan {
+            // Print out some nice progress information
+            self.note_working_on(config, &key, fresh)?;
+        }
+
         match fresh {
             Freshness::Fresh => doit(),
             Freshness::Dirty => {
@@ -407,11 +413,6 @@ impl<'a> JobQueue<'a> {
             }
         }

-        if !build_plan {
-            // Print out some nice progress information
-            self.note_working_on(config, &key, fresh)?;
-        }
-
         Ok(())
     }

@alexcrichton
Copy link
Member

Ah yeah I think we probably want to guarantee that we print out the result first, want to go ahead and apply the alternative patch to ensure that status messages go out before we spawn work?

@ehuss ehuss force-pushed the fix-fix_path_deps branch from 2cfb2f1 to 52f45b5 Compare October 30, 2018 00:06
@ehuss
Copy link
Contributor Author

ehuss commented Oct 30, 2018

want to go ahead and apply the alternative patch

Sure! Updated with the alternate approach. I agree it is better, I was just concerned of unexpected consequences.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 30, 2018

📌 Commit 52f45b5 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 30, 2018

⌛ Testing commit 52f45b5 with merge c067ab0f6b80962f6845e1866571fd88605aeba6...

@bors
Copy link
Contributor

bors commented Oct 30, 2018

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

bors added a commit that referenced this pull request Oct 31, 2018
@bors
Copy link
Contributor

bors commented Oct 31, 2018

⌛ Testing commit 52f45b5 with merge 56d630f...

@bors
Copy link
Contributor

bors commented Oct 31, 2018

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

@bors bors merged commit 52f45b5 into rust-lang:master Oct 31, 2018
@ehuss ehuss mentioned this pull request Nov 6, 2018
bors added a commit that referenced this pull request Nov 6, 2018
Fix fix_path_deps again.

The previous fix in #6236 wasn't enough to make this test not fail. On Travis MacOS it fails about 5% of the time. Essentially the first "FIXING" line is sometimes delayed until after the second "CHECKING" line. The "FIXING" message is sent async to the diagnostic server, so I think it's just a race that doesn't have a good way to enforce.  Oddly, I can only repro this on Travis.

Error seen at:
https://travis-ci.org/rust-lang/cargo/jobs/450956693
@ehuss ehuss added this to the 1.32.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.

5 participants