Use dep jobslot instead of dep jobserver

[NobodyXu]

Signed-off-by: Jiahao XU [email protected]

What does this PR try to resolve?

This PR replaces dep jobserver with a fork jobslot.

Why fork jobserver?

• jobserver's maintainer @alexcrichton is not willing to merge PR for
  bug fix.
• Better performance on unix: jobserver's implementation uses
  std::os::unix::process::CommandExt::pre_exec, which prevents Command::spawn
  from using vfork on unix.

[rust-highfive]

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

Please see the contribution instructions for more information.

[ehuss]

Sorry, I don't think the unwillingness to merge a PR is a sign that a crate is unmaintained. jobserver is one of our core dependencies, and I would prefer to not change it unless it was clear that it can't be updated. Looking at the PR, it seems like there were recurrent, legitimate concerns. I appreciate that you have some things you want to address, but I do not think this is the right solution for cargo.

[NobodyXu]

Sorry, I don't think the unwillingness to merge a PR is a sign that a crate is unmaintained.

I agree that the wording is a bit confusing and I would change that, but jobserver contains a bug on windows where the Client can be dropped before Command is spawned.

Looking at the PR, it seems like there were recurrent, legitimate concerns.

The main concern is change of API, I can see that this being a problem as all the dependencies of jobserver now have to bump the major version and fix their usage of jobserver::Client.

However, IMHO this is worth the effort because this would fix a potential bug.

[alexcrichton]

While I respect @NobodyXu your right to fork any open source project, I personally like it's a bit aggressive to send PRs to a bunch of dependencies, tag me, claim that I basically don't maintain the project any more, and then push a claim that your fork is not only more correct but also faster.

Your PR to fix the mentioned bug, which I felt was pretty minor in the grand scheme of things, had quite a lot of issues with it. I ran out of energy helping you write a patch because it felt like I was the one authoring the PR. Your current fork of the crate has what I personally consider a serious bug, which is that you turn off CLOEXEC in the parent process meaning that in a mulithreaded process where multiple threads are forking you're leaking jobserver file descriptors into child processes. Personally I consider that a much more serious bug then the one you were trying to fix.

To be clear, I think forking is fine, but I want to be sure to set the record straight in that I am still maintaining jobserver and there's real technical reasons I'm not merging your PR, not because I simply don't want to.

[NobodyXu]

While I respect @NobodyXu your right to fork any open source project, I personally like it's a bit aggressive to send PRs to a bunch of dependencies, tag me, claim that I basically don't maintain the project any more, and then push a claim that your fork is not only more correct but also faster.

I'm very sorry for this and I have removed these unbased claims.

Your current fork of the crate has what I personally consider a serious bug, which is that you turn off CLOEXEC in the parent process meaning that in a mulithreaded process where multiple threads are forking you're leaking jobserver file descriptors into child processes.

Hmmm, I did not consider that as a bug.
I will fix that.

[NobodyXu]

Personally I consider that a much more serious bug then the one you were trying to fix.

Fixed in 0.2.6.

BTW, if you don't mind changing the API interface, I can submit another PR for this or attempts to upstream other improvements I made.

[ehuss]

I'm going to close as this is unlikely a change we want to make at this time. If there are specific issues related directly to Cargo, it would be best to start with an issue on the issue tracker.