From 419df14a104cd38162d902a3216da37ef2fe16d0 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 23 Nov 2022 13:45:46 +0100 Subject: [PATCH] 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 `. [does not currently support]: https://github.com/clap-rs/clap/issues/1404 [a workaround for this isn't currently functioning]: https://github.com/clap-rs/clap/issues/1404#issuecomment-1309254274 * 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 ` 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`. --- cargo-apk/src/apk.rs | 6 +- cargo-apk/src/main.rs | 223 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 223 insertions(+), 6 deletions(-) diff --git a/cargo-apk/src/apk.rs b/cargo-apk/src/apk.rs index 3a625d6d..ceb7cb65 100644 --- a/cargo-apk/src/apk.rs +++ b/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()); } diff --git a/cargo-apk/src/main.rs b/cargo-apk/src/main.rs index 8ad81b19..fab80d0d 100644 --- a/cargo-apk/src/main.rs +++ b/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, }, /// 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) -> (Args, Vec) { + // 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::>() + }) + .collect::>(); + + #[derive(Debug, Default)] + struct SplitArgs { + apk_args: Vec, + cargo_args: Vec, + 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()] + ) + ); +}