Support inheriting jobserver fd for external subcommands #10511

weihanglo · 2022-03-26T11:09:37Z

What does this PR try to resolve?

If cargo detects the existence of a jobserver, cargo should pass the
jobserver down to the external subcommand. Here are the reasons:

The existence of jobserver implies the user "expects" the amount of
job is under control. However, before this commit, external
subcommands cannnot benefit from the global view of the jobserver.
cargo-clippy as an external subcommand migth also love to respect
the jobserver protocol.
There are several well-known external subcommands calling "cargo"
interally (cargo-fuzz, cargo-tarpaulin, etc.)

Fixes #10477

zulip stream

How should we test and review this PR?

A new test jobserver::external_subcommand_inherits_jobserver is added.

You can also execute cargo-clippy from make -j2 to test the behaviour
(I usually run procs -w rustc to observe the amount of rustc invocations):

# Makefile
all:
	+./target/debug/cargo clippy --release

Caveats

Job without special prefix + might still be considered as a sub-make
and would inherit the jobserver, though I don't see it as an issue.

According to GNU Make Manual "13.1.1 POSIX Jobserver Interaction",
if --jobserver-auth option is available in MAKEFLAGS but the file
descriptors are closed, it means that the calling make didn't consider
our tool awas a recursive make invocation. I make an assumption that
if those fds are still open, we are happy to use those jobserver tokens.

rust-highfive · 2022-03-26T11:09:40Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

weihanglo · 2022-03-26T11:10:30Z

As a side note, I found that cargo doesn't respect make -j1 because GNU make won't set any value in MAKEFLAGS if job count equals 1. I cannot see any way that cargo can tell the difference between -j1 and no -j specified. Actually make defaults to run jobs sequentially, which is the opposite of cargo's behaviour, so I don't think this can be fixed easily (and probably won't fix?).

If cargo detects the existence of a jobserver, cargo should pass the jobserver down to the external subcommand. Here are the reasons: 1. The existence of jobserver implies the user "expects" the amount of job is under control. However, before this commit, external subcommands cannnot benefit from the global view of the jobserver. 2. `cargo-clippy` as an external subcommand migth also love to respect the jobserver protocol. 3. There are several well-known external subcommands calling "cargo" interally (cargo-fuzz, cargo-tarpaulin, etc.) Caveats: Job without special prefix `+` might still be considered as a sub-make and would inherit the jobserver, though I don't see it as an issue. According to GNU Make Manual "13.1.1 POSIX Jobserver Interaction" [^1], if `--jobserver-auth` option is available in `MAKEFLAGS` but the file descriptors are closed, it means that the calling `make` didn't consider our tool awas a recursive `make` invocation. I make an assumption that if those fds are still open, we are happy to use those jobserver tokens. [^1]: https://www.gnu.org/software/make/manual/make.html#POSIX-Jobserver

petrochenkov · 2022-04-07T18:12:27Z

So I've encountered a very similar situation with cargo run.

I try to use cargo run -p my_tool_that_uses_cbindgen_as_a_library (and cbindgen will recursively call cargo one more time).
Such a tool also wants to inherit the jobserver.

If the descriptors are closed, they later may be reopened by something unrelated, which will ICE rustc or cargo because they will still attempt to read/write something jobserver-like from those descriptors. (That's what happen in my build environment at least.)

Maybe cargo needs some kind of explicit option to avoid closing jobserver descriptors in such cases.

UPD: The workaround is to not use cargo run but to use cargo build and to run built executable as two separate commands + use the SNEAK_DOLLAR_MAKE_INTO_THE_COMMAND=$(MAKE) trick (rust-lang/rust#46981 (comment)) for both commands.

weihanglo · 2022-05-26T12:46:30Z

@petrochenkov, thanks for sharhing the interesting case. I feel like cargo run should not implicitly pass jobserver down to the executable. What cargo run runs is not always an external subcommand. Making that assumption might be a bit overkill I guess? Anyway, if the closed descriptors do cause problems for you, could you open a new issue for that?

bors · 2022-08-01T02:00:02Z

☔ The latest upstream changes (presumably #9892) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-08-03T15:47:08Z

☔ The latest upstream changes (presumably #10934) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2022-09-02T14:29:16Z

Sorry for the long delay. I think this makes sense. There's a tiny risk that it might interfere with some existing tools or workflows, but I think on balance it is the right behavior.

@bors r+

bors · 2022-09-02T14:29:18Z

📌 Commit c1b90b9 has been approved by ehuss

It is now in the queue for this repository.

bors · 2022-09-02T14:29:31Z

⌛ Testing commit c1b90b9 with merge 646e9a0...

bors · 2022-09-02T15:09:50Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 646e9a0 to master...

8 commits in 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5..646e9a0b9ea8354cc409d05f10e8dc752c5de78e 2022-08-27 18:41:39 +0000 to 2022-09-02 14:29:28 +0000 - Support inheriting jobserver fd for external subcommands (rust-lang/cargo#10511) - refactor(cli): Lazy load config (rust-lang/cargo#11029) - chore: Don't show genned docs in ripgrep (rust-lang/cargo#11040) - Document private items for Cargo and publish under contributor guide (rust-lang/cargo#11019) - Add names to CI jobs (rust-lang/cargo#11039) - Rework test error handling (rust-lang/cargo#11028) - Very slight `cargo add` documentation improvements (rust-lang/cargo#11033) - Update compiling requirements. (rust-lang/cargo#11030)

Update cargo 8 commits in 4ed54cecce3ce9ab6ff058781f4c8a500ee6b8b5..646e9a0b9ea8354cc409d05f10e8dc752c5de78e 2022-08-27 18:41:39 +0000 to 2022-09-02 14:29:28 +0000 - Support inheriting jobserver fd for external subcommands (rust-lang/cargo#10511) - refactor(cli): Lazy load config (rust-lang/cargo#11029) - chore: Don't show genned docs in ripgrep (rust-lang/cargo#11040) - Document private items for Cargo and publish under contributor guide (rust-lang/cargo#11019) - Add names to CI jobs (rust-lang/cargo#11039) - Rework test error handling (rust-lang/cargo#11028) - Very slight `cargo add` documentation improvements (rust-lang/cargo#11033) - Update compiling requirements. (rust-lang/cargo#11030)

External subcommands are already able to inherit the jobserver from env since rust-lang#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 rate. * Creating a jobsever driver with the new FIFO named pipe style is not yet supported as `[email protected]`. Once we can create a named pipe based jobserver, it will be less risky and inheritance by default can be implemented.

External subcommands are already able to inherit the jobserver from env since rust-lang#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.

External subcommands are already able to inherit the jobserver from env since rust-lang#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 provides such an ability to the general `target.<...>.runner`, which affects `cargo test`, `cargo bench`, and `cargo run`. 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.

This was done in <rust-lang#10511>.

rust-highfive assigned ehuss Mar 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2022

weihanglo added 2 commits March 26, 2022 19:12

test: external subcommand inherits jobserver

b099d39

weihanglo force-pushed the issue-10477 branch from 3d77523 to c1b90b9 Compare March 26, 2022 11:13

weihanglo force-pushed the issue-10477 branch from b28b667 to c1b90b9 Compare August 4, 2022 20:12

weihanglo mentioned this pull request Aug 30, 2022

cargo --config 'k=v' <plugin> isn't passed down to the plugin #11031

Open

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 Sep 2, 2022

bors merged commit 646e9a0 into rust-lang:master Sep 2, 2022

weihanglo deleted the issue-10477 branch September 2, 2022 15:24

weihanglo mentioned this pull request Sep 5, 2022

Update cargo rust-lang/rust#101460

Merged

ehuss added this to the 1.65.0 milestone Sep 21, 2022

This was referenced Aug 30, 2023

Report errors in jobserver inherited through environment variables rust-lang/rust#113730

Merged

Support inheriting jobserver when using cargo run #12597

Closed

weihanglo mentioned this pull request Oct 6, 2023

feat: inherit jobserver from env for all kinds of runner #12776

Merged

weihanglo added a commit to weihanglo/cargo that referenced this pull request May 6, 2024

docs: add missing CARGO_MAKEFLAGS env for plugins

525283a

This was done in <rust-lang#10511>.

weihanglo added a commit to weihanglo/cargo that referenced this pull request May 6, 2024

docs: add missing CARGO_MAKEFLAGS env for plugins

6dc080f

This was done in <rust-lang#10511>.

weihanglo mentioned this pull request May 6, 2024

docs: add missing CARGO_MAKEFLAGS env for plugins #13872

Merged

bend-n pushed a commit to bend-n/cargo that referenced this pull request May 7, 2024

docs: add missing CARGO_MAKEFLAGS env for plugins

fdeea3d

This was done in <rust-lang#10511>.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support inheriting jobserver fd for external subcommands #10511

Support inheriting jobserver fd for external subcommands #10511

weihanglo commented Mar 26, 2022

rust-highfive commented Mar 26, 2022

weihanglo commented Mar 26, 2022 •

edited

Loading

petrochenkov commented Apr 7, 2022 •

edited

Loading

weihanglo commented May 26, 2022

bors commented Aug 1, 2022

bors commented Aug 3, 2022

ehuss commented Sep 2, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

Support inheriting jobserver fd for external subcommands #10511

Support inheriting jobserver fd for external subcommands #10511

Conversation

weihanglo commented Mar 26, 2022

What does this PR try to resolve?

How should we test and review this PR?

Caveats

rust-highfive commented Mar 26, 2022

weihanglo commented Mar 26, 2022 • edited Loading

petrochenkov commented Apr 7, 2022 • edited Loading

weihanglo commented May 26, 2022

bors commented Aug 1, 2022

bors commented Aug 3, 2022

ehuss commented Sep 2, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

weihanglo commented Mar 26, 2022 •

edited

Loading

petrochenkov commented Apr 7, 2022 •

edited

Loading