cargo-apk: Reimplement "default" (--) subcommand trailing args for cargo (#363)

* cargo-apk: Reimplement "default" subcommand trailing args for `cargo` with `--` separator

`clap` [does not currently support] parsing unknown arguments into a
side `Vec`, which is exactly what `cargo apk --` needs to parse a few
known `cargo` arguments (such as `--target` and `-p`) but forward the
rest verbatim to the underlying `cargo` subcommand.

`allow_hyphen_values = true` could partially help us out with this, but
it parses all remaining arguments into that `Vec` upon encountering the
first unknown flag/arg, resulting in all known flags after that to also
be treated as "unknown" instead of filling up our `args: Args` struct.

Since [a workaround for this isn't currently functioning], introduce
pure trailing args with an additional `--` separator to make it clear
which arguments go to `cargo-apk` (and are almost all, except
`--device`, forwarded to `cargo`) and which are only passed to `cargo
<subcommand>`.

[does not currently support]: clap-rs/clap#1404
[a workaround for this isn't currently functioning]: clap-rs/clap#1404 (comment)

* cargo-apk: Separate unrecognized `cargo` arguments from cargo-apk `Args`

With some custom logic, and assuming (validated with an `assert!`) our
`Args` struct doesn't have any positionals, we can implement argument
separation ourselves: this allows the user to mix and match `cargo
<subcommand>` arguments with arguments recognized by `cargo-apk`, and
expect `cargo-apk` to set up the environment as expected (as it did
previously) by taking these arguments into account while disregarding
_only_ unknown arguments.

* Add `args: Args` back to `Ndk` subcommand to see them in `-h`

Mixed `cargo-apk` and `cargo` args will still be split out from
`cargo_args`, and they'll be appended to existing values for `args`.

Author: MarijnS95

cargo-apk/src/apk.rs

@@ -306,7 +306,7 @@ impl<'a> ApkBuilder<'a> {
         Ok(())
     }
 
-    pub fn default(&self, cargo_cmd: &str) -> Result<(), Error> {
+    pub fn default(&self, cargo_cmd: &str, cargo_args: &[String]) -> Result<(), Error> {
         for target in &self.build_targets {
             let mut cargo = cargo_ndk(
                 &self.ndk,
@@ -322,6 +322,10 @@ impl<'a> ApkBuilder<'a> {
                 cargo.arg("--target").arg(triple);
             }
 
+            for additional_arg in cargo_args {
+                cargo.arg(additional_arg);
+            }
+
             if !cargo.status()?.success() {
                 return Err(NdkError::CmdFailed(cargo).into());
             }

cargo-apk/src/main.rs

@@ -1,6 +1,8 @@
+use std::collections::HashMap;
+
 use cargo_apk::{ApkBuilder, Error};
 use cargo_subcommand::Subcommand;
-use clap::Parser;
+use clap::{CommandFactory, FromArgMatches, Parser};
 
 #[derive(Parser)]
 struct Cmd {
@@ -17,7 +19,8 @@ enum ApkCmd {
     },
 }
 
-#[derive(Parser)]
+#[derive(Clone, Debug, Eq, PartialEq, Parser)]
+#[group(skip)]
 struct Args {
     #[clap(flatten)]
     subcommand_args: cargo_subcommand::Args,
@@ -27,7 +30,6 @@ struct Args {
 }
 
 #[derive(clap::Subcommand)]
-#[clap(trailing_var_arg = true)]
 enum ApkSubCmd {
     /// Analyze the current package and report errors, but don't build object files nor an apk
     #[clap(visible_alias = "c")]
@@ -44,9 +46,18 @@ enum ApkSubCmd {
     /// Invoke `cargo` under the detected NDK environment
     #[clap(name = "--")]
     Ndk {
+        /// `cargo` subcommand to run
         cargo_cmd: String,
+
+        // This struct will be filled up later by arguments that are intermixed
+        // with unknown args and ended up in `cargo_args` below.
         #[clap(flatten)]
         args: Args,
+
+        /// Arguments passed to cargo. Some arguments will be used to configure
+        /// the environment similar to other `cargo apk` commands
+        #[clap(trailing_var_arg = true, allow_hyphen_values = true)]
+        cargo_args: Vec<String>,
     },
     /// Run a binary or example apk of the local package
     #[clap(visible_alias = "r")]
@@ -66,6 +77,61 @@ enum ApkSubCmd {
     Version,
 }
 
+fn split_apk_and_cargo_args(mut args: Args, input: Vec<String>) -> (Args, Vec<String>) {
+    // Clap doesn't support parsing unknown args properly:
+    // https://github.com/clap-rs/clap/issues/1404
+    // https://github.com/clap-rs/clap/issues/4498
+    // Introspect the `Args` struct and extract every known arg, and whether it takes a value. Use
+    // this information to separate out known args from unknown args, and re-parse all the known
+    // args into an existing `args: Args` struct instance.
+
+    let known_args_taking_value = Args::command()
+        .get_arguments()
+        .flat_map(|arg| {
+            assert!(!arg.is_positional());
+            arg.get_short_and_visible_aliases()
+                .iter()
+                .flat_map(|shorts| shorts.iter().map(|short| format!("-{}", short)))
+                .chain(
+                    arg.get_long_and_visible_aliases()
+                        .iter()
+                        .flat_map(|longs| longs.iter().map(|short| format!("--{}", short))),
+                )
+                .map(|arg_str| (arg_str, arg.get_action().takes_values()))
+                // Collect to prevent lifetime issues on temporaries created above
+                .collect::<Vec<_>>()
+        })
+        .collect::<HashMap<_, _>>();
+
+    #[derive(Debug, Default)]
+    struct SplitArgs {
+        apk_args: Vec<String>,
+        cargo_args: Vec<String>,
+        next_takes_value: bool,
+    }
+
+    let split_args = input
+        .into_iter()
+        .fold(SplitArgs::default(), |mut split_args, elem| {
+            let known_arg = known_args_taking_value.get(&elem);
+            if known_arg.is_some() || split_args.next_takes_value {
+                // Recognized arg or value for previously recognized arg
+                split_args.apk_args.push(elem)
+            } else {
+                split_args.cargo_args.push(elem)
+            }
+
+            split_args.next_takes_value = known_arg.copied().unwrap_or(false);
+            split_args
+        });
+
+    let m = Args::command()
+        .no_binary_name(true)
+        .get_matches_from(&split_args.apk_args);
+    args.update_from_arg_matches(&m).unwrap();
+    (args, split_args.cargo_args)
+}
+
 fn main() -> anyhow::Result<()> {
     env_logger::init();
     let Cmd {
@@ -84,10 +150,16 @@ fn main() -> anyhow::Result<()> {
                 builder.build(artifact)?;
             }
         }
-        ApkSubCmd::Ndk { cargo_cmd, args } => {
+        ApkSubCmd::Ndk {
+            cargo_cmd,
+            args,
+            cargo_args,
+        } => {
+            let (args, cargo_args) = split_apk_and_cargo_args(args, cargo_args);
+
             let cmd = Subcommand::new(args.subcommand_args)?;
             let builder = ApkBuilder::from_subcommand(&cmd, args.device)?;
-            builder.default(&cargo_cmd)?;
+            builder.default(&cargo_cmd, &cargo_args)?;
         }
         ApkSubCmd::Run { args, no_logcat } => {
             let cmd = Subcommand::new(args.subcommand_args)?;
@@ -107,3 +179,144 @@ fn main() -> anyhow::Result<()> {
     }
     Ok(())
 }
+
+#[test]
+fn test_split_apk_and_cargo_args() {
+    // Set up a default because cargo-subcommand doesn't derive/implement Default
+    let args_default = Args::parse_from(std::iter::empty::<&str>());
+
+    assert_eq!(
+        split_apk_and_cargo_args(args_default.clone(), vec!["--quiet".to_string()]),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    quiet: true,
+                    ..args_default.subcommand_args.clone()
+                },
+                ..args_default.clone()
+            },
+            vec![]
+        )
+    );
+
+    assert_eq!(
+        split_apk_and_cargo_args(
+            args_default.clone(),
+            vec!["unrecognized".to_string(), "--quiet".to_string()]
+        ),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    quiet: true,
+                    ..args_default.subcommand_args.clone()
+                },
+                ..args_default.clone()
+            },
+            vec!["unrecognized".to_string()]
+        )
+    );
+
+    assert_eq!(
+        split_apk_and_cargo_args(
+            args_default.clone(),
+            vec!["--unrecognized".to_string(), "--quiet".to_string()]
+        ),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    quiet: true,
+                    ..args_default.subcommand_args.clone()
+                },
+                ..args_default.clone()
+            },
+            vec!["--unrecognized".to_string()]
+        )
+    );
+
+    assert_eq!(
+        split_apk_and_cargo_args(
+            args_default.clone(),
+            vec!["-p".to_string(), "foo".to_string()]
+        ),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    package: vec!["foo".to_string()],
+                    ..args_default.subcommand_args.clone()
+                },
+                ..args_default.clone()
+            },
+            vec![]
+        )
+    );
+
+    assert_eq!(
+        split_apk_and_cargo_args(
+            args_default.clone(),
+            vec![
+                "-p".to_string(),
+                "foo".to_string(),
+                "--unrecognized".to_string(),
+                "--quiet".to_string()
+            ]
+        ),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    quiet: true,
+                    package: vec!["foo".to_string()],
+                    ..args_default.subcommand_args.clone()
+                },
+                ..args_default.clone()
+            },
+            vec!["--unrecognized".to_string()]
+        )
+    );
+
+    assert_eq!(
+        split_apk_and_cargo_args(
+            args_default.clone(),
+            vec![
+                "--no-deps".to_string(),
+                "-p".to_string(),
+                "foo".to_string(),
+                "--unrecognized".to_string(),
+                "--quiet".to_string()
+            ]
+        ),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    quiet: true,
+                    package: vec!["foo".to_string()],
+                    ..args_default.subcommand_args.clone()
+                },
+                ..args_default.clone()
+            },
+            vec!["--no-deps".to_string(), "--unrecognized".to_string()]
+        )
+    );
+
+    assert_eq!(
+        split_apk_and_cargo_args(
+            args_default.clone(),
+            vec![
+                "--no-deps".to_string(),
+                "--device".to_string(),
+                "adb:test".to_string(),
+                "--unrecognized".to_string(),
+                "--quiet".to_string()
+            ]
+        ),
+        (
+            Args {
+                subcommand_args: cargo_subcommand::Args {
+                    quiet: true,
+                    ..args_default.subcommand_args
+                },
+                device: Some("adb:test".to_string()),
+            },
+            vec!["--no-deps".to_string(), "--unrecognized".to_string()]
+        )
+    );
+}