Fix Client::configure* on unix
#100
Conversation
| Ok(unsafe { len.assume_init() } as usize) | ||
| } | ||
|
|
||
| pub fn configure(&self, cmd: &mut Command) { | ||
| match self { | ||
| if matches!(self.creation_arg, ClientCreationArg::Fifo { .. }) { |
There was a problem hiding this comment.
Now that we are passing the fds specified in the environment, and we always clone the fds before using them or setting cloexec, I suppose we could remove this function completely? cc @the8472
This should improve the performance dramatically on platform with vfork available.
There was a problem hiding this comment.
I think this should happen in a separate PR since we need to consider both the environment and the file descriptors together, basically #64 but also the role of client.configure. It'd be a breaking change of that API. Right now the docs say that only children built with this method should inherit the jobserver.
The goal of this PR is to fix the fd to fifo conversion and I think it's already big enough for that.
|
This also needs a regression test. Apparently nothing ensured that when inheriting a configuration from the parent we pass it through properly to a child. |
|
cc @the8472 Updated, added regression test as suggested. |
rust-lang#100 Signed-off-by: Jiahao XU <[email protected]>
* Fix `Client::configure*` on unix rust-lang#100 Signed-off-by: Jiahao XU <[email protected]> * Fix linux compilation Signed-off-by: Jiahao XU <[email protected]> --------- Signed-off-by: Jiahao XU <[email protected]>
There was a problem hiding this comment.
Thanks for the fix. Could we fix up some commits?
Also, I'd prefer presenting the regression test first in a commit, followed by the other commits actually fix the behavior and tests. That would make the first commit a minimal reproduction and we know we're fixing the right thing.
That might be a bit hard since I wrote after fixing it |
So that setting `O_NONBLOCK` would not affect other. Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
|
@weihanglo I squashed the commits, can't get the regression test commit to be the first, but it's much smaller now. |
Co-authored-by: Weihang Lo <[email protected]> Signed-off-by: Jiahao XU <[email protected]>
|
Would appreciate if you have time publishing this :) (We should probably revisit #89 and rust-lang/infra-team#117) |
Fixed #99