Skip to content

Commit

Permalink
Improve --help content in firecracker and jailer
Browse files Browse the repository at this point in the history 
- Display default values along with the description
- Segregate required and optional arguments
- Sort and align arguments

Signed-off-by: Shion Yamashita <shioyama1118@gmail.com>
  • Loading branch information
@shioyama18 @aghecenco
shioyama18 authored and aghecenco committed May 18, 2020
1 parent a7c25c7 commit 484e5e9
Show file tree
Hide file tree
Showing 3 changed files with 175 additions and 44 deletions.
16 changes: 7 additions & 9 deletions src/firecracker/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -236,7 +234,7 @@ fn main() {

let start_time_cpu_us = arguments.value_as_string("start-time-cpu-us").map(|s| {
s.parse::<u64>()
.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,
Expand Down
201 changes: 167 additions & 34 deletions src/utils/src/arg_parser.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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::<Vec<_>>();

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::<Vec<_>>()
.join("\n")
}
}

/// Stores the characteristics of the `name` command line argument.
Expand Down Expand Up @@ -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!("{:<arg_width$}", arg, arg_width = arg_width));

if self.takes_value {
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
Expand All @@ -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<String>,
}
Expand Down Expand Up @@ -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 <exec-file>");
argument = Argument::new("exec-file").takes_value(true);

assert_eq!(
argument.format_help(width),
" --exec-file <exec-file> "
);
assert_eq!(
argument.format_help(short_width),
" --exec-file <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>: 'exec-file' info."
argument.format_help(width),
" --exec-file <exec-file> 'exec-file' info."
);
assert_eq!(
argument.format_help(short_width),
" --exec-file <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 <exec-file> [default: \"./exec-file\"]"
);
assert_eq!(
argument.format_help(short_width),
" --exec-file <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> 'exec-file' info. [default: \"./exec-file\"]"
);
assert_eq!(
argument.format_help(short_width),
" --exec-file <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> 'exec-file' info.\n\n\
optional arguments:\n \
--api-sock <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>: 'exec-file' info.\n"
"optional arguments:\n \
--config-file <config-file> 'config-file' info.\n \
--id <id> 'id' info.\n \
--seccomp-level <seccomp-level> 'seccomp-level' info."
);
}

Expand Down Expand Up @@ -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\"");
}
}
2 changes: 1 addition & 1 deletion tests/integration_tests/build/test_coverage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down

0 comments on commit 484e5e9

Please sign in to comment.