Liboci additional flags and subcommands, as required by ociplex

[c3d]

These are a few changes in the liboci-cli crate that are necessary for some additional commands that I am implementing in ociplex.

[codecov-commenter]

Codecov Report

Merging #2149 (cdb860d) into main (e5f74ca) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2149      +/-   ##
==========================================
- Coverage   64.82%   64.64%   -0.18%     
==========================================
  Files         129      131       +2     
  Lines       14769    14809      +40     
==========================================
  Hits         9574     9574              
- Misses       5195     5235      +40     

[yihuaf]

Also, you can run just lint and just spellcheck locally to check for these misc things.

[utam0k]

@c3d May I ask you to fix the error from clippy? If you need, please ignore it

[c3d]

@c3d May I ask you to fix the error from clippy? If you need, please ignore it

I saw an error about variants size pertaining to the subcommands.

error: large size difference between variants
Error:   --> crates/youki/src/main.rs:48:1
   |
48 | / enum SubCommand {
49 | |     // Standard and common commands handled by the liboci_cli crate
50 | |     #[clap(flatten)]
51 | |     Standard(liboci_cli::StandardCmd),
   | |     --------------------------------- the second-largest variant contains at least 104 bytes
52 | |     #[clap(flatten)]
53 | |     Common(liboci_cli::CommonCmd),
   | |     ----------------------------- the largest variant contains at least 320 bytes
...  |
57 | |     Completion(commands::completion::Completion),
58 | | }
   | |_^ the entire enum is at least 320 bytes

There is indeed a lot of variations between the subcommands, since this branch adds a lot of command-specific options. I think that this is expected. Adding boxing is probably of little value for command-line arguments (they won't use that much memory to start with).

Is there a way to tell the rust linter to ignore this specific case?

[yihuaf]

Is there a way to tell the rust linter to ignore this specific case?

Try:

#[derive(Parser, Debug)]
#[allow(clippy::large_enum_variant)]
enum SubCommand {
...
}

The main issue is that this gets triggered in youki but the affected changes comes from liboci-cli which are suppose to be consumed as an individual library. We can use the above allow to suppress youki but is there any other consumer for liboci-cli out there?

Alternatively, you can box up a few of the large structure such as Exec.

[c3d]

Is there a way to tell the rust linter to ignore this specific case?

Try:

#[derive(Parser, Debug)]
#[allow(clippy::large_enum_variant)]
enum SubCommand {
...
}

The main issue is that this gets triggered in youki but the affected changes comes from liboci-cli which are suppose to be consumed as an individual library. We can use the above allow to suppress youki but is there any other consumer for liboci-cli out there?

Well, I'm co-developing the "other" user at the moment (a project called ociplex), which is the reason why this was put in a separate crate to start with.

Alternatively, you can box up a few of the large structure such as Exec.

I'll give this approach a try. My initial thinking is that this was more overhead than the difference in size, knowing that we don't have that many command structures to start with in a normal application, so the extra cost of heap management might not be worth it.

[c3d]

@yihuaf I added a separate commit. Do you see this as an improvement? I can't say I like it much, but it does eliminate the just lint warning.

[yihuaf]

@yihuaf I added a separate commit. Do you see this as an improvement? I can't say I like it much, but it does eliminate the just lint warning.

Yea I don't like this as well, especially since this breaks up the liboci-cli apis to make each sub command argument inconsistent. The user now have to pay attention on which is box and which is not. How about we box the SubCommand in the application? In this way, each users of the liboci-cli can decide how to proceed or if to enforce the lint rule at all. I find this to be a better alternative. In the following diff, I boxed up the enum from liboci-cli in the youki application.

[eng@stardust youki]$ git diff
diff --git a/crates/liboci-cli/src/lib.rs b/crates/liboci-cli/src/lib.rs
index 6dafaf44..89c48a6d 100644
--- a/crates/liboci-cli/src/lib.rs
+++ b/crates/liboci-cli/src/lib.rs
@@ -52,7 +52,7 @@ pub enum StandardCmd {
 pub enum CommonCmd {
     Checkpointt(Checkpoint),
     Events(Events),
-    Exec(Box<Exec>),
+    Exec(Exec),
     Features(Features),
     List(List),
     Pause(Pause),
diff --git a/crates/youki/src/main.rs b/crates/youki/src/main.rs
index 09374a41..6a92be8d 100644
--- a/crates/youki/src/main.rs
+++ b/crates/youki/src/main.rs
@@ -48,9 +48,9 @@ struct Opts {
 enum SubCommand {
     // Standard and common commands handled by the liboci_cli crate
     #[clap(flatten)]
-    Standard(liboci_cli::StandardCmd),
+    Standard(Box<liboci_cli::StandardCmd>),
     #[clap(flatten)]
-    Common(liboci_cli::CommonCmd),
+    Common(Box<liboci_cli::CommonCmd>),
 
     // Youki specific extensions
     Info(info::Info),
@@ -106,7 +106,7 @@ fn main() -> Result<()> {
     let systemd_cgroup = opts.global.systemd_cgroup;
 
     let cmd_result = match opts.subcmd {
-        SubCommand::Standard(cmd) => match cmd {
+        SubCommand::Standard(cmd) => match *cmd {
             StandardCmd::Create(create) => {
                 commands::create::create(create, root_path, systemd_cgroup)
             }
@@ -115,12 +115,12 @@ fn main() -> Result<()> {
             StandardCmd::Delete(delete) => commands::delete::delete(delete, root_path),
             StandardCmd::State(state) => commands::state::state(state, root_path),
         },
-        SubCommand::Common(cmd) => match cmd {
+        SubCommand::Common(cmd) => match *cmd {
             CommonCmd::Checkpointt(checkpoint) => {
                 commands::checkpoint::checkpoint(checkpoint, root_path)
             }
             CommonCmd::Events(events) => commands::events::events(events, root_path),
-            CommonCmd::Exec(exec) => match commands::exec::exec(*exec, root_path) {
+            CommonCmd::Exec(exec) => match commands::exec::exec(exec, root_path) {
                 Ok(exit_code) => std::process::exit(exit_code),
                 Err(e) => {
                     eprintln!("exec failed : {e}");
[eng@stardust youki]$ 

If you are OK with this approach, I can push the commit to this PR and merge.

[yihuaf]

I will help you to get this merged since this is the last outstanding items.