feat: support async calls for the rust crate #30
Conversation
crate/src/lib.rs
Outdated
| let dl = dl_cache | ||
| .download(true, &tmp_dir, &["near-sandbox"], &bin_url()) | ||
| .map_err(anyhow::Error::msg)? | ||
| .ok_or_else(|| anyhow!("Could not install near-sandbox"))?; |
There was a problem hiding this comment.
The install function can potentially be swapped over to an async version as well, but a bit more involved since we'd have to replace binary_install and roll our own async version (no simple async download/install crate off the top of my head). This can be reserved for later, so no need to do it here
There was a problem hiding this comment.
Took a crack at this and I guess we could go with something like this: https://github.com/itegulov/binary-install-async. I used tokio just for the sake of simplicity, but I guess we would have to be async runtime agnostic, right?
To push it even further we can replace tar with async-tar. Unfortunately, the latter is tightly coupled with async-std so we would have to refactor that too.
There was a problem hiding this comment.
I guess we would have to be async runtime agnostic, right?
yup, this is where it's going to be "fun". Since some of the fs operations does spawn blocking, we'll need to pass in a futures::task::Executor to make that part agnostic.
Generally, this comment pinpoints a good amount of details to make things runtime agnostic: rust-lang/wg-async#45 (comment). Let me know if you hit a snag though, since there could be dragons somewhere with async ecosystem being this fragmented
There was a problem hiding this comment.
That is a great source, thanks for sharing. I followed the suggestions and arrived at this: itegulov/binary-install-async@6522b50. Not sure what is the best way to give feedback, but feel free to leave comments directly under that commit.
Also, should we move the repo to NEAR org or is it fine to keep it as a personal repo for now?
There was a problem hiding this comment.
we can just make it a workspace member crate of sandbox repo, or add it as helper functions. Whichever is easier.
feel free to leave comments directly under that commit
didn't know you could leave comments on commits to master, but will leave my feedback there
There was a problem hiding this comment.
Left a comment here: itegulov/binary-install-async@6522b50#r69286897
crate/src/lib.rs
Outdated
| pub async fn run_with_options(options: &[&str]) -> anyhow::Result<Child> { | ||
| let bin_path = crate::ensure_sandbox_bin()?; | ||
| Command::new(bin_path) | ||
| .args(options) | ||
| .envs(log_vars()) | ||
| .envs(crate::log_vars()) | ||
| .spawn() | ||
| .map_err(Into::into) | ||
| } |
There was a problem hiding this comment.
I didn't realize that spawn isn't async for async_process::Command. Looking at the code, it doesn't do anything different much from a regular std Command in this case, but the returned async_process::Child is still relevant for the asynchronous actions we can perform afterwards. So think its best to get rid of these async labels since we aren't doing any async work here. But if we were to make install async, then this function would then be async as well, so leaving this one up to you whether we should keep as async
There was a problem hiding this comment.
Yeah, that's fair. Will do if we don't reach a conclusion on your first point short term and will handle the rest in a separate PR.
|
I have removed |
Fixes #29
Decided to move sync functions to a separate module and expose async ones by default since this seems to be a common practice in Rust crates. Also, because you can't create a module named
async:)