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.

Sign up for GitHub

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

Jump to bottom

Release a jobserver token while locking a file #6748

Merged
merged 2 commits into from
Mar 15, 2019
Merged

Release a jobserver token while locking a file #6748

merged 2 commits into from
Mar 15, 2019

Conversation

alexcrichton
Copy link
Member

This is a possible solution to #6747, but we'll ideally get some testing
in before landing!

@rust-highfive
Copy link

r? @ehuss

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

@alexcrichton alexcrichton mentioned this pull request Mar 14, 2019
Open
@alexcrichton
Release a jobserver token while locking a file
d19b41f 
This is intended to fix rust-lang#6747 where multiple Cargos invoked with the
same jobserver would all have their own token but not actually run
concurrently due to file locking. Instead the fix is that whenever Cargo
blocks for a file lock with a configured global jobserver, a token is
released just before we block and then reacquired afterwards. This way
we should ensure that we're not hogging a cpu/token unnecessarily
without doing any work!

Closes rust-lang#6747
@alexcrichton
Copy link
Member Author

@bors: r=ehuss

@bors
Copy link
Collaborator

bors commented Mar 15, 2019

📌 Commit d19b41f has been approved by ehuss

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 15, 2019
@bors
Copy link
Collaborator

bors commented Mar 15, 2019

⌛ Testing commit d19b41f with merge c24978b99f510f61213af8f6482763fa0d064f31...

@bors
Copy link
Collaborator

bors commented Mar 15, 2019

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 15, 2019
@alexcrichton
Remove minimal versions build from Windows
512fccb 
Looks like minimal versions don't work with the 0.6.5 release of `rand`
on Windows...
@alexcrichton
Copy link
Member Author

Hm ok I've had to push up a commit which removes the minimal-versions build on Windows. Unfortunately if you create a new project, add rand = "0.6.5" and build it for Windows it fails with a similar error. I feel like we're not really getting a lot of benefit from testing minimal versions on our CI so I figured it might be best to not hold this up on that. @ehuss mind taking another look though? @Eh2406 I suspect you'll want to weigh in on this as well.

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2019

I suspect this is the same issue found in #6636.

I can take a look at fixing rand's minimal versions.

I think switching to a direct-dependency-only minimal versions might be the better long-term solution (#5657 (comment)). But I have not thought much about how it could be implemented or what complications may arise.

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 15, 2019

Given that #6636 and this are stuck on fixing rands minimal version, I think declaring the minimal-version experiment dead is justified, or at least suspending it until someone fixes rand. I have had only enough time to determine that it won't be trivial.

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2019

It looks like the fundamental rand issues in 0.6 have been fixed (via rust-random/rand#703), but new versions of some of the subcrates (like rand_hc) have not been published. It's not clear to me from the last comment in that PR what is blocking publishing new versions. Additionally, the root Cargo.toml in the rand project will need to set the patch version for every sub-crate, and push a new update as well. Then any crate depending on 0.6 will need to set their minimum version to that new version (in this case, I think it is just jobserver, and proptest for the other situation).

Given the comments (rust-random/rand#741 (comment)) I'm not sure if they'd be receptive to doing the work to accommodate this. I can open an issue on rand, but I don't really want to police the entire ecosystem to keep this working.

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 15, 2019

So this is the logjam as I see it:

  1. People are using --minimal-versions and it generally works, sometimes they ask for fixes from upstream. This is awkward to ask or not fixed upstream because it is not approved/stabilized by the Cargo Team. (for example Test builds with minimal versions rust-random/rand#741 (comment))
  2. The Cargo Team is not ready to stabilize it as everyone who has used it has hit some case that does not work.

There are 2 nash-equilibrium. We stabilize it and the ecosystem adapts or we officially give up on it. Maybe we find some smaller version that can get us out of the jam. But the current state of affairs is not stable.

@alexcrichton
Copy link
Member Author

Yeah I definitely agree the direct-dependency-only version would likely solve this, but @ehuss I wouldn't necessarily take this as a signal that we need to fix rand (moreso that the direct-dependency version is more plausible!).

Are y'all ok though temporarily at least removing this from Windows CI?

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2019

Are y'all ok though temporarily at least removing this from Windows CI?

I'm OK with this.

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 15, 2019

I am ok with it.

@alexcrichton
Copy link
Member Author

Ok!

@bors: r=ehuss

@bors
Copy link
Collaborator

bors commented Mar 15, 2019

📌 Commit 512fccb has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2019
@bors
Copy link
Collaborator

bors commented Mar 15, 2019

⌛ Testing commit 512fccb with merge efd29db...

bors added a commit that referenced this pull request Mar 15, 2019
@bors
Auto merge of #6748 - alexcrichton:release-then-acquire, r=ehuss
efd29db 
Release a jobserver token while locking a file

This is a possible solution to #6747, but we'll ideally get some testing
in before landing!
@Eh2406 Eh2406 mentioned this pull request Mar 15, 2019
Open
@bors
Copy link
Collaborator

bors commented Mar 15, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: ehuss
Pushing efd29db to master...

@bors bors merged commit 512fccb into rust-lang:master Mar 15, 2019
Eh2406 added a commit to Eh2406/cargo that referenced this pull request Mar 16, 2019
@Eh2406
Proptest 0.9.1
ed9386b 
This is a rework of rust-lang#6636 now possible do to the change in testing from rust-lang#6748
@Eh2406 Eh2406 mentioned this pull request Mar 16, 2019
Merged
@Eh2406
Copy link
Contributor

Eh2406 commented Mar 16, 2019

Anyone know why we only hit this on windows, but hit it on travis for #6753 ?

@ehuss
Copy link
Contributor

ehuss commented Mar 16, 2019

Anyone know why we only hit this on windows, but hit it on travis for #6753 ?

Jobserver only uses rand on windows.

bors added a commit that referenced this pull request Mar 16, 2019
@bors
Auto merge of #6753 - Eh2406:proptest, r=dwijnand
7d09f17 
Proptest 0.9.1

This is a rework of #6636 now possible do to the change in testing from #6748
@ehuss ehuss mentioned this pull request Mar 29, 2019
Merged
bors added a commit to rust-lang/rust that referenced this pull request Mar 30, 2019
@bors
Auto merge of #59516 - ehuss:update-cargo, r=alexcrichton
befeeb7 
Update cargo

Update cargo

22 commits in 0e35bd8af0ec72d3225c4819b330b94628f0e9d0..63231f438a2b5b84ccf319a5de22343ee0316323
2019-03-13 06:52:51 +0000 to 2019-03-27 12:26:45 +0000
- Code cleanup (rust-lang/cargo#6787)
- Add cargo:rustc-link-arg to pass custom linker arguments (rust-lang/cargo#6298)
- Testsuite: remove some unnecessary is_nightly checks. (rust-lang/cargo#6786)
- cargo metadata: Don't show `null` deps. (rust-lang/cargo#6534)
- Some fingerprint cleanup. (rust-lang/cargo#6785)
- Fix fingerprint for canceled build script. (rust-lang/cargo#6782)
- Canonicalize default target if it ends with `.json` (rust-lang/cargo#6778)
- Fix setting `panic=unwind` compiling lib a extra time. (rust-lang/cargo#6781)
- Always nicely show errors from crates.io if possible (rust-lang/cargo#6771)
- Testsuite: Make `cwd()` relative to project root. (rust-lang/cargo#6768)
- Allow `cargo fix` if gitignore matches root working dir. (rust-lang/cargo#6767)
- Remove redundant imports (rust-lang/cargo#6763)
- Handle backcompat hazard with `toml` crate (rust-lang/cargo#6761)
- Fix spurious error in dirty_both_lib_and_test. (rust-lang/cargo#6756)
- Update toml requirement from 0.4.2 to 0.5.0 (rust-lang/cargo#6760)
- Reuse std::env::consts::EXE_SUFFIX (rust-lang/cargo#6758)
- Proptest 0.9.1 (rust-lang/cargo#6753)
- Don't need extern crate in 2018 (rust-lang/cargo#6752)
- Release a jobserver token while locking a file (rust-lang/cargo#6748)
- Minor doc fix for publish command synopsis (rust-lang/cargo#6749)
- Stricter package change detection. (rust-lang/cargo#6740)
- Fix resolving yanked crates when using a local registry. (rust-lang/cargo#6742)
@alexcrichton alexcrichton deleted the release-then-acquire branch May 6, 2019 18:13
@alexcrichton alexcrichton mentioned this pull request Aug 1, 2019
Closed
@ehuss ehuss added this to the 1.35.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss approved these changes

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.35.0
Development

Successfully merging this pull request may close these issues.
5 participants
@alexcrichton @rust-highfive @bors @ehuss @Eh2406