Finally, remove anyhow from the libcontainer dependency.
#1937
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1937 +/- ##
==========================================
+ Coverage 64.86% 64.88% +0.01%
==========================================
Files 127 127
Lines 14680 14674 -6
==========================================
- Hits 9522 9521 -1
+ Misses 5158 5153 -5 |
crates/libcontainer/src/error.rs
Outdated
|
|
||
| // Catch all errors that are not covered by the above | ||
| #[error("syscall error")] | ||
| OtherSyscall(#[source] nix::Error), | ||
| #[error("IO error")] | ||
| OtherIO(#[source] std::io::Error), | ||
| #[error("Serialization error")] |
There was a problem hiding this comment.
| #[error("Serialization error")] | |
| #[error("serialization error")] |
| impl Executor for WasmEdgeExecutor { | ||
| fn exec(&self, spec: &Spec) -> Result<()> { | ||
| impl WasmEdgeExecutor { | ||
| fn exec_inner(spec: &Spec) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Why don't we use this error here?
There was a problem hiding this comment.
This is mostly to minimize the changes since the original code uses anyhow. The implementation of the 3 wasm executor is implemented in youki, not libcontainer, so there is no hard requirement to remove anyhow. I was not sure how best to approach this so I wrapped the original implementation into the exec_inner and only map_err once to meet the trait requirement of Executor.
I also wasn't sure how best to implement the Executor trait and the interface libcontainer should provide. This PR does the minimal so there are still rooms for improvement. I have a vague plan to implement the Executor trait with an Error associated type, so the implementation can provide whatever error type they wish. However, this proves to be tricky because we need to store the Executor in ContainerBuilder and ContainerArgs. I could not figure out how to store a trait object with unknown associated type yet. Perhaps the new Generic Associated Type can be of some help. In the end, I decided to keep things simple for now to get this PR merged, and then we can figure out how best to approach this.
On a different note, I suspect that Executor interface can become a great thing for youki and libcontainer, because it opens up a lot of possibility. Currently, we switching the the default execvp calls for the wasm calls. I can also switching the default execvp to run an arbitrary rust function. This would allow us to better and easier testing the libcontainer directly, without the full setup of the oci bundle.
This is a long winded way to answer your question :) But I can certainly remove the anyhow from these executor as well.
There was a problem hiding this comment.
Done. Fixed these wasm executor not using anyhow.
Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
yihuaf
force-pushed
the
yihuaf/remove-anyhow
branch
from
5437190 to
a286eb6
Compare
|
@yihuaf Thanks a lot 🙏 |
This is the final PR that implements the
thiserrorin libcontainer. Fix #1377