From 484e5e9bc463ce2cb67feee6855124c97b89e198 Mon Sep 17 00:00:00 2001 From: Shion Yamashita Date: Mon, 4 May 2020 15:38:17 +0000 Subject: [PATCH] Improve `--help` content in firecracker and jailer - Display default values along with the description - Segregate required and optional arguments - Sort and align arguments Signed-off-by: Shion Yamashita --- src/firecracker/src/main.rs | 16 +- src/utils/src/arg_parser.rs | 201 +++++++++++++++--- .../integration_tests/build/test_coverage.py | 2 +- 3 files changed, 175 insertions(+), 44 deletions(-) diff --git a/src/firecracker/src/main.rs b/src/firecracker/src/main.rs index 3459f191da3..08c73b78c40 100644 --- a/src/firecracker/src/main.rs +++ b/src/firecracker/src/main.rs @@ -100,21 +100,19 @@ fn main() { .takes_value(true) .default_value("2") .help( - "Level of seccomp filtering that will be passed to executed path as \ - argument.\n - - Level 0: No filtering.\n - - Level 1: Seccomp filtering by syscall number.\n - - Level 2: Seccomp filtering by syscall number and argument values.\n - ", + "Level of seccomp filtering (0: no filter | 1: filter by syscall number | 2: filter by syscall \ + number and argument values) that will be passed to executed path as argument." ), ) .arg( Argument::new("start-time-us") - .takes_value(true), + .takes_value(true) + .help("Process start time (wall clock, microseconds)."), ) .arg( Argument::new("start-time-cpu-us") - .takes_value(true), + .takes_value(true) + .help("Process start CPU time (wall clock, microseconds)."), ) .arg( Argument::new("config-file") @@ -236,7 +234,7 @@ fn main() { let start_time_cpu_us = arguments.value_as_string("start-time-cpu-us").map(|s| { s.parse::() - .expect("'start-time-cpu_us' parameter expected to be of 'u64' type.") + .expect("'start-time-cpu-us' parameter expected to be of 'u64' type.") }); let instance_info = InstanceInfo { id: instance_id, diff --git a/src/utils/src/arg_parser.rs b/src/utils/src/arg_parser.rs index f8de2af6c6f..f204f5ea0ef 100644 --- a/src/utils/src/arg_parser.rs +++ b/src/utils/src/arg_parser.rs @@ -1,7 +1,7 @@ // Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::collections::HashMap; +use std::collections::BTreeMap; use std::env; use std::fmt; use std::result; @@ -78,17 +78,53 @@ impl<'a> ArgParser<'a> { pub fn formatted_help(&self) -> String { let mut help_builder = vec![]; - for arg in self.arguments.args.values() { - help_builder.push(format!("{}\n", arg.format_help())); + let required_arguments = self.format_arguments(true); + if !required_arguments.is_empty() { + help_builder.push("required arguments:".to_string()); + help_builder.push(required_arguments); } - help_builder.concat() + let optional_arguments = self.format_arguments(false); + if !optional_arguments.is_empty() { + // Add line break if `required_arguments` is pushed. + if !help_builder.is_empty() { + help_builder.push("".to_string()); + } + + help_builder.push("optional arguments:".to_string()); + help_builder.push(optional_arguments); + } + + help_builder.join("\n") } /// Return a reference to `arguments` field. pub fn arguments(&self) -> &Arguments { &self.arguments } + + // Filter arguments by whether or not it is required. + // Align arguments by setting width to length of the longest argument. + fn format_arguments(&self, is_required: bool) -> String { + let filtered_arguments = self + .arguments + .args + .values() + .filter(|arg| is_required == arg.required) + .collect::>(); + + let max_arg_width = filtered_arguments + .iter() + .map(|arg| arg.format_name().len()) + .max() + .unwrap_or(0); + + filtered_arguments + .into_iter() + .map(|arg| arg.format_help(max_arg_width)) + .collect::>() + .join("\n") + } } /// Stores the characteristics of the `name` command line argument. @@ -150,20 +186,36 @@ impl<'a> Argument<'a> { self } - fn format_help(&self) -> String { + fn format_help(&self, arg_width: usize) -> String { let mut help_builder = vec![]; - help_builder.push(format!("--{}", self.name)); + let arg = self.format_name(); + help_builder.push(format!("{:", self.name)); - } + // Add three whitespaces between the argument and its help message for readability. + help_builder.push(" ".to_string()); + + match (self.help, &self.default_value) { + (Some(help), Some(default_value)) => { + help_builder.push(format!("{} [default: {}]", help, default_value)) + } + (Some(help), None) => help_builder.push(help.to_string()), + (None, Some(default_value)) => { + help_builder.push(format!("[default: {}]", default_value)) + } + (None, None) => (), + }; - if let Some(help) = self.help { - help_builder.push(format!(": {}", help)); - } help_builder.concat() } + + fn format_name(&self) -> String { + if self.takes_value { + format!(" --{name} <{name}>", name = self.name) + } else { + format!(" --{}", self.name) + } + } } /// Represents the value of an argument, which will be a `String` if @@ -190,11 +242,20 @@ impl Value { } } +impl fmt::Display for Value { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Value::Bool(b) => write!(f, "{}", b), + Value::String(s) => write!(f, "\"{}\"", s), + } + } +} + /// Stores the arguments of the parser. #[derive(Clone, Default)] pub struct Arguments<'a> { - // A Hash Map in which the key is an argument and the value is its associated `Argument`. - args: HashMap<&'a str, Argument<'a>>, + // A BTreeMap in which the key is an argument and the value is its associated `Argument`. + args: BTreeMap<&'a str, Argument<'a>>, // The arguments specified after `--` (i.e. end of command options). extra_args: Vec, } @@ -406,46 +467,112 @@ mod tests { #[test] fn test_arg_help() { // Checks help format for an argument. - let mut argument = Argument::new("exec-file").required(true).takes_value(true); + let width = 32; + let short_width = 16; + + let mut argument = Argument::new("exec-file").takes_value(false); + + assert_eq!( + argument.format_help(width), + " --exec-file " + ); + assert_eq!(argument.format_help(short_width), " --exec-file "); - assert_eq!(argument.format_help(), "--exec-file "); + argument = Argument::new("exec-file").takes_value(true); + + assert_eq!( + argument.format_help(width), + " --exec-file " + ); + assert_eq!( + argument.format_help(short_width), + " --exec-file " + ); argument = Argument::new("exec-file") - .required(true) .takes_value(true) .help("'exec-file' info."); assert_eq!( - argument.format_help(), - "--exec-file : 'exec-file' info." + argument.format_help(width), + " --exec-file 'exec-file' info." + ); + assert_eq!( + argument.format_help(short_width), + " --exec-file 'exec-file' info." ); - argument = Argument::new("no-api") - .requires("config-file") - .takes_value(false); + argument = Argument::new("exec-file") + .takes_value(true) + .default_value("./exec-file"); - assert_eq!(argument.format_help(), "--no-api"); + assert_eq!( + argument.format_help(width), + " --exec-file [default: \"./exec-file\"]" + ); + assert_eq!( + argument.format_help(short_width), + " --exec-file [default: \"./exec-file\"]" + ); - argument = Argument::new("no-api") - .requires("config-file") - .takes_value(false) - .help("'no-api' info."); + argument = Argument::new("exec-file") + .takes_value(true) + .default_value("./exec-file") + .help("'exec-file' info."); - assert_eq!(argument.format_help(), "--no-api: 'no-api' info."); + assert_eq!( + argument.format_help(width), + " --exec-file 'exec-file' info. [default: \"./exec-file\"]" + ); + assert_eq!( + argument.format_help(short_width), + " --exec-file 'exec-file' info. [default: \"./exec-file\"]" + ); } #[test] fn test_arg_parser_help() { // Checks help information when user passes `--help` flag. - let arg_parser = ArgParser::new().arg( - Argument::new("exec-file") - .required(true) - .takes_value(true) - .help("'exec-file' info."), + let mut arg_parser = ArgParser::new() + .arg( + Argument::new("exec-file") + .required(true) + .takes_value(true) + .help("'exec-file' info."), + ) + .arg( + Argument::new("api-sock") + .takes_value(true) + .help("'api-sock' info."), + ); + + assert_eq!( + arg_parser.formatted_help(), + "required arguments:\n \ + --exec-file 'exec-file' info.\n\n\ + optional arguments:\n \ + --api-sock 'api-sock' info." ); + + arg_parser = ArgParser::new() + .arg(Argument::new("id").takes_value(true).help("'id' info.")) + .arg( + Argument::new("seccomp-level") + .takes_value(true) + .help("'seccomp-level' info."), + ) + .arg( + Argument::new("config-file") + .takes_value(true) + .help("'config-file' info."), + ); + assert_eq!( arg_parser.formatted_help(), - "--exec-file : 'exec-file' info.\n" + "optional arguments:\n \ + --config-file 'config-file' info.\n \ + --id 'id' info.\n \ + --seccomp-level 'seccomp-level' info." ); } @@ -718,4 +845,10 @@ mod tests { "The argument 'foo' was provided more than once." ); } + + #[test] + fn test_value_display() { + assert_eq!(format!("{}", Value::Bool(true)), "true"); + assert_eq!(format!("{}", Value::String("foo".to_string())), "\"foo\""); + } } diff --git a/tests/integration_tests/build/test_coverage.py b/tests/integration_tests/build/test_coverage.py index f7143656ce4..0e15ae1b47b 100644 --- a/tests/integration_tests/build/test_coverage.py +++ b/tests/integration_tests/build/test_coverage.py @@ -17,7 +17,7 @@ import framework.utils as utils import host_tools.cargo_build as host # pylint: disable=import-error -COVERAGE_TARGET_PCT = 84.53 +COVERAGE_TARGET_PCT = 84.59 COVERAGE_MAX_DELTA = 0.05 CARGO_KCOV_REL_PATH = os.path.join(host.CARGO_BUILD_REL_PATH, 'kcov')