Fix Client::configure* on unix - #100
Conversation
|
|
||
| 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 <Jiahao_XU@outlook.com>
* Fix `Client::configure*` on unix rust-lang#100 Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com> * Fix linux compilation Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com> --------- Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
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.
the8472
left a comment
the8472
left a comment
There was a problem hiding this comment.
Functionally it seems fine to me now. I think it could use a few more comments here and there.
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 <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
|
@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 <weihanglo@users.noreply.github.com> Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
|
Would appreciate if you have time publishing this :) (We should probably revisit #89 and rust-lang/infra-team#117) |
3 participants
Fixed #99