Don't close jobserver fd when executing external subcommand

trinity-1686a commented 2 years ago

Problem

I tried to use cargo-fuzz from a Makefile, expecting it to use make's jobserver to limit concurency. When running it as cargo-fuzz, it worked as intended, but when running it as cargo fuzz, it did not seem to acknowledge make job count. I found cargo-fuzz environment variables contained --jobserver-auth=3,4 as expected, but these file descriptors are closed.

Steps

create a script called cargo-sleep and put it into your $PATH
```
#!/bin/bash
sleep $2
```
write a simple Makefile calling this command with jobserver passing
```
all:
    +cargo sleep 10
```
run the makefile with jobserver
```
make -j2
```
look at the environment of sleep. It contains MAKEFLAGS= -j2 --jobserver-auth=3,4 (actual number may vary)
look at open file descriptors of sleep, 3 and 4 (in my case) are not open.

Possible Solution(s)

No response

Notes

As stated there is a simple workaround, calling the sub-command directly instead of through cargo

Version

cargo 1.59.0 (49d8809dc 2022-02-10)
release: 1.59.0
commit-hash: 49d8809dc2d3e6e0d5ec634fcf26d8e2aab67130
commit-date: 2022-02-10
host: x86_64-unknown-linux-gnu
libgit2: 1.3.0 (sys:0.13.23 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1l)
os: Arch Linux [64-bit]

weihanglo commented 2 years ago

Thanks for the report. The behaviour indeed reflects how Cargo runs external subcommands: Cargo doesn't call ProcessBuilder::inherit_jobserver() for external subcommands^1, so file descriptors get close due to FD_CLOEXEC flag (on Linux).

Not sure whether this issue is a bug or a feature. GNU Make says^2:

If your tool determines that the --jobserver-auth option is available in MAKEFLAGS but that the file descriptors specified are closed, this means that the calling make process did not think that your tool was a recursive make invocation (e.g., the command line was not prefixed with a + character). You should notify your users of this situation.

If I get it correctly, it's up to Cargo to determine whether the process should inherit the jobserver or not. It seems that cargo-fuzz is a reasonable use case and personally I'd love to see it working.

weihanglo commented 2 years ago

Hi. We have discussed inheriting jobserver fd on Zulip. There might be some compatibility issues: It's rare but some commands might use fd 3 and 4 without notice of the existence of jobserver. As you said, there is a simple workaround. I'd recommend sticking with that.

trinity-1686a commented 2 years ago

I understand the compatibility concern.

You should notify your users of this situation.

Could cargo do that, so that at least other peoples stumbling on this bug/feature are made aware of the workaround? There are multiple external subcommands that actually call cargo internally (cargo-fuzz, cargo-tarpaulin...).

Also some such external subcommands do not support being called directly (I'm thinking about cargo-ndk). This is not my use case, but it could me someone else's.

rust-lang / cargo