feat: support inheriting jobserver for `cargo run`

[weihanglo]

What does this PR try to resolve?

External subcommands are already able to inherit the jobserver from env since https://github.com/rust-lang/cargo/pull/10511. However, users reported that they've expected cargo run to behave the same as external subcommands.

A popular example "cargo-xtask" pattern is used as scripting to run arbitrary tasks. Users may want to invoke cargo run from Make and expect some parallelism. This PR tries to provide such an ability.

Note that this PR doesn't create any jobserver client if there is no existing jobserver from the environment. Nor -j/--jobs would create a new client. Reasons for this decision:

• There might be crates don't want the jobserver to pollute their file descriptors, although they might be rare
• Creating a jobsever driver with the new FIFO named pipe style is not yet supported as of [email protected]. Once we can create a named pipe-based jobserver, it will be less risky and inheritance by default can be implemented.

How should we test and review this PR?

• 54c8fe82f0321b5225663bd0f2ee88c04bb37160 demonstrates the current behavior — cargo run doesn't support inheriting jobserver from env.
• 993a70b18ca45914566dd68dc0c7840f58cb659d Cargo starts supporting such case.

Additional informations

r? ehuss

cc @petrochenkov, does this meet your needs?

fixes #12597

[petrochenkov]

cc @petrochenkov, does this meet your needs?

It's hard to say from looking at the changes because I'm not familiar with cargo code. The added test looks like https://github.com/rust-lang/cargo/pull/10511#issuecomment-1092057057, so that use case seems covered.

Is the target.runner case covered as well? Is it possible to test this change with https://github.com/rust-lang/rust/pull/113730 (after removing the Miri workaround)?

[weihanglo]

Is the target.runner case covered as well? Is it possible to test this change with rust-lang/rust#113730 (after removing the Miri workaround)?

I believe the answer is yes, as it calls target_process, which respects target.runner config. https://github.com/rust-lang/cargo/blob/5a2ea1930909dde2bae22f3a1261970de22ea4a2/src/cargo/ops/cargo_run.rs#L95

Is it possible to test this change with rust-lang/rust#113730 (after removing the Miri workaround)?

Looks like miri calls cargo an rustc from CARGO and RUSTC environment variables, so it should works if you build cargo from source and set RUSTC to the actual compiler exe path test it.

[petrochenkov]

Looks like miri calls cargo an rustc from CARGO and RUSTC environment variables, so it should works if you build cargo from source and set RUSTC to the actual compiler exe path test it.

I'll never have time to do it in observable future :D I guess it's simple to merge this and then try to remove the Miri FIXME when the new cargo with this change reaches bootstrap.

[petrochenkov]

cc @belovdv

[ehuss]

Is the target.runner case covered as well? Is it possible to test this change with rust-lang/rust#113730 (after removing the Miri workaround)?

I believe the answer is yes, as it calls target_process, which respects target.runner config.

I'm confused by this. I believe the answer is "no", since target_process does not inherit the jobserver?

I'm fine with this PR as-is, I just wanted to make sure I understand. I think I would also be fine with inheriting in a runner as well. I'm skeptical that there is really any risk with including extra file descriptors open.

[weihanglo]

I'm confused by this. I believe the answer is "no", since target_process does not inherit the jobserver?

Unless I misunderstood the question, doesn't this PR make the target_process, either test executable or target runner, inherit jobserver? 🤔

[ehuss]

Unless I misunderstood the question, doesn't this PR make the target_process, either test executable or target runner, inherit jobserver? 🤔

I'm not sure if we are looking at the same thing. This PR (as of 7ad590451626749802995dcd491a921fa6b7b409) modifies src/cargo/ops/cargo_run.rs to have cargo run inherit the jobserver. It does not appear to modify target_process or anything in the chain that would affect the target.*.runner configuration variable for anything other than cargo run (specifically, cargo test or cargo bench aren't supported).

I assume the question (Is the target.runner case covered as well?) is referring to the general ability of commands like cargo test to also forward the jobserver to target.*.runner executables.

[weihanglo]

I see. Thanks for the clarification. Yeah I misunderstood that 😕

[ehuss]

@weihanglo Just checking, are you ok with moving inherit_jobserver to target_process to also cover runners as mentioned in #12597?

[rustbot]

Could not assign reviewer from: ehuss. User(s) ehuss are either the PR author, already assigned, or on vacation, and there are no other candidates. Use r? to specify someone else to assign.

[weihanglo]

Ended up making target runner inherit jobserver.

@rustbot ready

[weihanglo]

It's hard to find a place to document which part of cargo gets access to jobserver. I'll do it in a follow-up when figuring out.

[ehuss]

Thanks!

@bors r+

[bors]

:pushpin: Commit 678f3ff487b7ce823d7616a1a0f480f9e78df038 has been approved by ehuss

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 678f3ff487b7ce823d7616a1a0f480f9e78df038 with merge 7bb7b539558dc88bea44cee4168b6269bf8177b0...

[bors]

:sunny: Test successful - checks-actions Approved by: ehuss Pushing 7bb7b539558dc88bea44cee4168b6269bf8177b0 to master...

[RalfJung]

Given that the Miri issue, as I understood it, was about the cargo-managed jobserver (that exists during build time) not being around at run time, I don't think this lets us remove the Miri workaround. It might be good for other usecases though. :)