feat(dvc): make dvc named pipe proxy cross-platform#896
Conversation
Coverage Report 🤖 ⚙️Past: New: Diff: +0.00% [this comment will be updated automatically] |
Vladyslav Nikonov (pacmancoder)
changed the title
Vladyslav Nikonov (pacmancoder)
force-pushed
the
feat/dvc-proxy-cross-platform
branch
from
b071a39 to
3ec5fba
Compare
Vladyslav Nikonov (pacmancoder)
requested a review
from Benoît Cortier (CBenoit)
| /* | ||
| match fs::metadata(&ctx.pipe_name).await | ||
| { | ||
| Ok(metadata) => { |
There was a problem hiding this comment.
question: Leftover?
| "Win32_System_IO", | ||
| ] } | ||
| tokio = { version = "1", features = ["net", "rt", "sync", "macros", "io-util"]} | ||
| async-trait = "0.1" |
There was a problem hiding this comment.
note: We loose the sync-friendly non-tokio-specific interface you made efforts to keep, but at the same time the tradeoff seems worth it.
question: Just to be on the safe side, can you double check you really enabled only the minimal amount of features?
There was a problem hiding this comment.
Yep, feature set is minimal
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
LGTM!
I appreciate that the API surface is very small. I see you were careful about making tokio an internal, implementation detail.
However, the downside is that we end up using two runtimes, and it’s overall a bit heavier. Maybe you could expose an async function to "ignite" the worker manually into the tokio thread pool of the consumer, and use some opaque handle that can be re-injected into DvcNamedPipeProxy, and re-used in start.
Feel free to modify and merge yourself!
Vladyslav Nikonov (pacmancoder)
force-pushed
the
feat/dvc-proxy-cross-platform
branch
from
1e0f622 to
9294658
Compare
Decided to keep it as-is for now, as the current dvc proxy uses |
Vladyslav Nikonov (pacmancoder)
deleted the
feat/dvc-proxy-cross-platform
branch
2 participants
Changes
tokio::net::unix::UnixStream)tokio::net::windows::named_pipeTesting
This feature can be used in the same way as on Windows, however instead of GUI test app there is new basic CLI app