-
-
Notifications
You must be signed in to change notification settings - Fork 114
feat: allow to connect a python rpc-client to an rpc-server via a unix FIFO file instead of subprocess #7535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
510fbee to
1ab8329
Compare
| fn_response_fifo = tmp_path.joinpath("response_fifo") | ||
| os.mkfifo(fn_request_fifo) | ||
| os.mkfifo(fn_response_fifo) | ||
| popen = subprocess.Popen(f"deltachat-rpc-server <{fn_request_fifo} >{fn_response_fifo}", shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For any real use FIFOs should likely be replaced with the UNIX socket, which exists on all platforms (including Windows) and can handle multiple connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #7545 for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense to rather go for UNIX sockets, but i think it's fine to merge this PR already. Shouldn't be too hard to write another RpcUNIXSocket subclass after #7545 is done.
| """ | ||
| from .client import Bot | ||
|
|
||
| _run_cli(Bot, hooks, argv, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know if it is actually unused e.g. for rpc_server_path, at least the documentation should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rpc_server_path was recently introduced. we can re-introduce **kwargs here if actually needed. Passing **kwargs all around makes reasoning about what happens harder in my experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason **kwargs existed in the past was to be able to control the created process, ex. IIRC TUI clients need to tweak it so stderr don't dirty the screen,
I am talking about the removal in the Rpc class, but other approaches like allowing to pass a Popen directly to the start method in RpcProcess would also work I guess
also internally refactors rpc.py a bit and strikes unneccessary
**kwargs