jobserver fd_check: Allow sockets.#117
Conversation
|
Can add some details ; I already have a piece of software running that creates a token pool across all builds on the local machine using an abstract unix domain socket. It works well with GNU Make, Ninja and gcc. |
|
I'm more than willing to contribute, and to consider alternative approaches. |
NobodyXu
left a comment
NobodyXu
left a comment
There was a problem hiding this comment.
I'm not a maintainer of jobserver though I did have some contributions and subscribe to updates to jobserver closely and is a maintainer of cc, which use jobserver, so take my comment with a grain of salt.
And I'm not sure if I should merge the PR here on behalf of the maintainer, so probably not unless nobody has time to review it
| FromEnvErrorInner::CannotOpenFd(..) => FromEnvErrorKind::CannotOpenFd, | ||
| FromEnvErrorInner::NegativeFd(..) => FromEnvErrorKind::NegativeFd, | ||
| FromEnvErrorInner::NotAPipe(..) => FromEnvErrorKind::NotAPipe, | ||
| FromEnvErrorInner::NotAPipeOrSocket(..) => FromEnvErrorKind::NotAPipeOrSocket, |
There was a problem hiding this comment.
This would probably be an undesired breaking change, don't think we can rename this enum variant which is unfortunate
Could add a NotASocket
There was a problem hiding this comment.
I checked all uses in cargo/rust, this value was never explicitly referenced.
There was a problem hiding this comment.
It is better to avoid MSRV bump. I would suggest adding new variant and leave the old one there. We already have #[non_exhaustive] so should be fine adding new variants.
There was a problem hiding this comment.
Kept the old public variant and marked it as deprecated
xim
force-pushed
the
allow-socket-fds
branch
2 times, most recently
from
4e4f256 to
e0de330
Compare
There was a problem hiding this comment.
Is it possible we have tests for the new behavior including successful paths and failing paths?
| FromEnvErrorInner::CannotOpenFd(..) => FromEnvErrorKind::CannotOpenFd, | ||
| FromEnvErrorInner::NegativeFd(..) => FromEnvErrorKind::NegativeFd, | ||
| FromEnvErrorInner::NotAPipe(..) => FromEnvErrorKind::NotAPipe, | ||
| FromEnvErrorInner::NotAPipeOrSocket(..) => FromEnvErrorKind::NotAPipeOrSocket, |
There was a problem hiding this comment.
It is better to avoid MSRV bump. I would suggest adding new variant and leave the old one there. We already have #[non_exhaustive] so should be fine adding new variants.
weihanglo
left a comment
weihanglo
left a comment
There was a problem hiding this comment.
Thanks for the update. Would you mind rebasing onto master so the CI should pass?
Any fd with compatible semantics can be used for jobserver tokens, not just pipes. Multiple jobserver implementations use other fd types already. Other client implementations don't require pipes either, like those in `make` and `ninja`. Actually fixes issue rust-lang#6 Co-Authored-By: CertainLach <6235312+CertainLach@users.noreply.github.com>
Pipes, sockets and character devices are supported. Regular files are not supported. Additionally, check_pipe=false skips the type check.
4 participants
I agree with the creator of issue #6 that jobserver fds should be allowed to be sockets as well as pipes. A project I'm working on uses a jobserver implemented over Unix domain sockets.