-
-
Notifications
You must be signed in to change notification settings - Fork 115
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?
Changes from all commits
8b9f5c7
7dc717f
a77e9cb
00a6f41
5542dcf
51e5858
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import os | ||
| import platform # noqa | ||
| import subprocess | ||
|
|
||
| import pytest | ||
|
|
||
| from deltachat_rpc_client import DeltaChat, RpcFIFO | ||
|
|
||
|
|
||
| @pytest.mark.skipif("platform.system() == 'Windows'") | ||
| def test_rpc_fifo(tmp_path): | ||
| fn_request_fifo = tmp_path.joinpath("request_fifo") | ||
| 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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #7545 for this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| rpc = RpcFIFO(fn_response_fifo=fn_response_fifo, fn_request_fifo=fn_request_fifo) | ||
| with rpc: | ||
| dc = DeltaChat(rpc) | ||
| assert dc.rpc.get_system_info()["deltachat_core_version"] is not None | ||
| popen.wait() | ||
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_pathwas recently introduced. we can re-introduce**kwargshere if actually needed. Passing**kwargsall 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
**kwargsexisted 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