-
Notifications
You must be signed in to change notification settings - Fork 63
Construct ExecutionContext in humility-bin
#677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mkeeter/subcommand-enum
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,12 @@ | |
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| use clap::{ArgGroup, CommandFactory, FromArgMatches, Parser}; | ||
| use clap::{ | ||
| ArgGroup, CommandFactory, FromArgMatches, Parser, parser::ValueSource, | ||
| }; | ||
| mod cmd; | ||
| use humility_cli::{Cli, ExecutionContext}; | ||
| use humility::{msg, warn}; | ||
| use humility_cli::{Cli, ExecutionContext, env::Environment}; | ||
|
|
||
| /// Main CLI entry point | ||
| /// | ||
|
|
@@ -35,21 +38,195 @@ fn main() -> std::process::ExitCode { | |
| return std::process::ExitCode::FAILURE; | ||
| } | ||
| }; | ||
| let OuterCli { cli, cmd } = outer_cli; | ||
| if let Some(s) = version(&cli) { | ||
| println!("{s}"); | ||
| let OuterCli { mut cli, cmd } = outer_cli; | ||
|
|
||
| // The --version (-V) and --list-targets operations are implemented as | ||
| // flags, rather than subcommands, so we'll special-case them here. | ||
| if cli.version { | ||
| println!( | ||
| "{} {} {}", | ||
| env!("CARGO_BIN_NAME"), | ||
| env!("CARGO_PKG_VERSION"), | ||
| PROBES_ENABLED | ||
| ); | ||
| if cmd.is_some() { | ||
| warn!("ignoring subcommand after printing version"); | ||
| } | ||
| return std::process::ExitCode::SUCCESS; | ||
| }; | ||
| } else if cli.list_targets { | ||
| let env = cli.environment.as_ref().expect("checked by clap"); | ||
| let targets = match Environment::targets(env) { | ||
| Ok(targets) => targets, | ||
| Err(err) => { | ||
| msg!("failed to parse environment: {err:?}"); | ||
| return std::process::ExitCode::FAILURE; | ||
| } | ||
| }; | ||
|
|
||
| if cli.terse { | ||
| println!( | ||
| "{}", | ||
| targets | ||
| .iter() | ||
| .map(|(t, _)| &**t) | ||
| .collect::<Vec<_>>() | ||
| .join(", ") | ||
| ); | ||
| } else { | ||
| println!("{:15} DESCRIPTION", "TARGET"); | ||
|
|
||
| for (target, description) in &targets { | ||
| println!( | ||
| "{:15} {}", | ||
| target, | ||
| match description { | ||
| Some(d) => d, | ||
| None => "-", | ||
| } | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if cmd.is_some() { | ||
| warn!("ignoring subcommand after listing targets"); | ||
| } | ||
| return std::process::ExitCode::SUCCESS; | ||
| } | ||
|
|
||
| let log_level = if cli.verbose { "trace" } else { "warn" }; | ||
| let mut context = match ExecutionContext::new(cli, &m) { | ||
| Ok(ctx) => ctx, | ||
| Err(e) => { | ||
| eprintln!("failed to build execution context: {e:#}"); | ||
| return std::process::ExitCode::FAILURE; | ||
|
|
||
| // Before we do our processing, we need to check for the presence of | ||
| // environment variables on our mutually exclusive attach options | ||
| // (i.e., dump/probe/ip/target). Clap has support for options being | ||
| // set as environment variables, so why are we doing it here? We do | ||
| // this manually because Clap's behavior when an option has a | ||
| // specified environment variable is to treat a variable that is in | ||
| // the environment just as if the option had been set on the command | ||
| // line -- but that isn't actually what we want here: if an option is | ||
| // set on the command line, we want to ignore the fact that a | ||
| // different mutually exclusive option may be in the environment. So | ||
| // we only look at the environment if none of these is set (Clap | ||
| // assures that at most one is set on the command line), and we | ||
| // examine the environment in an order of precence: probe, ip, target, | ||
| // dump. If the user has specified no command line options but | ||
| // multiple of these in the environment, they will get the first in | ||
| // this ordering that we find. | ||
| if cli.dump.is_none() | ||
| && cli.probe.is_none() | ||
| && cli.ip.is_none() | ||
| && cli.target.is_none() | ||
| { | ||
| use std::env; | ||
|
|
||
| if let Ok(e) = env::var("HUMILITY_PROBE") { | ||
| cli.probe = Some(e); | ||
| } else if let Ok(e) = env::var("HUMILITY_IP") { | ||
| cli.ip = Some(e.parse().unwrap_or_else(|x| match x {})); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait how is parse returning Infallible here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Over in #668, we made IP parsing lazier – the parse captures a |
||
| } else if let Ok(e) = env::var("HUMILITY_TARGET") { | ||
| cli.target = Some(e); | ||
| } else if let Ok(e) = env::var("HUMILITY_DUMP") { | ||
| cli.dump = Some(e); | ||
| } | ||
| } | ||
|
|
||
| let environment = match (&cli.environment, &cli.target) { | ||
| (Some(env), Some(target)) => { | ||
| let env = match Environment::from_file(env, target) { | ||
| Ok(e) => e, | ||
| Err(err) => { | ||
| msg!("failed to match environment: {err:?}"); | ||
| return std::process::ExitCode::FAILURE; | ||
| } | ||
| }; | ||
|
|
||
| // Cannot specify a dump/probe/IP address and also an | ||
| // environment and target (enforced by clap) | ||
| assert!(cli.dump.is_none()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe turn these asserts into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutual exclusion between these options is enforced by |
||
| assert!(cli.probe.is_none()); | ||
| assert!(cli.ip.is_none()); | ||
|
|
||
| cli.probe = Some(env.probe.clone()); | ||
|
|
||
| // | ||
| // If we have an archive on the command-line or in an environment | ||
| // variable, we want to prefer that over whatever is in the | ||
| // environment file -- but we also want to warn the user about | ||
| // what is going on. | ||
| // | ||
| if cli.archive.is_some() { | ||
| let msg = if m.value_source("archive") | ||
| == Some(clap::parser::ValueSource::CommandLine) | ||
| { | ||
| "archive on command-line" | ||
| } else { | ||
| "archive in environment variable" | ||
| }; | ||
|
|
||
| warn!("{msg} overriding archive in environment file"); | ||
| } else { | ||
| cli.archive = match env.archive(&cli.archive_name) { | ||
| Ok(a) => Some(a), | ||
| Err(e) => { | ||
| msg!("Failed to get archive: {e}"); | ||
| return std::process::ExitCode::FAILURE; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Some(env) | ||
| } | ||
|
|
||
| (Some(env), None) => { | ||
| if let Err(err) = Environment::validate(env) { | ||
| eprintln!("failed to parse environment: {err:?}"); | ||
| return std::process::ExitCode::FAILURE; | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
|
||
| _ => None, | ||
| }; | ||
|
|
||
| // | ||
| // Check to see if we have both a dump and an archive. Because these | ||
| // conflict with one another but because we allow both of them to be | ||
| // set with an environment variable, we need to manually resolve this: | ||
| // we want to allow an explicitly set value (that is, via the command | ||
| // line) to win the conflict. | ||
| // | ||
| if cli.dump.is_some() && cli.archive.is_some() { | ||
| match ( | ||
| m.value_source("dump") == Some(ValueSource::CommandLine), | ||
| m.value_source("archive") == Some(ValueSource::CommandLine), | ||
| ) { | ||
| (true, true) => { | ||
| msg!("cannot specify both a dump and an archive"); | ||
| return std::process::ExitCode::FAILURE; | ||
| } | ||
|
|
||
| (false, false) => { | ||
| msg!( | ||
| "both dump and archive have been set via environment \ | ||
| variables; unset one of them, or use a command-line \ | ||
| option to override" | ||
| ); | ||
| return std::process::ExitCode::FAILURE; | ||
| } | ||
|
|
||
| (true, false) => { | ||
| warn!("dump on command-line overriding archive in environment"); | ||
| cli.archive = None; | ||
| } | ||
|
|
||
| (false, true) => { | ||
| warn!("archive on command-line overriding dump in environment"); | ||
| cli.dump = None; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut context = ExecutionContext { environment, cli }; | ||
| let Some(cmd) = cmd else { | ||
| eprintln!("humility failed: subcommand expected (--help to list)"); | ||
| return std::process::ExitCode::FAILURE; | ||
|
|
@@ -74,19 +251,6 @@ static PROBES_ENABLED: &str = ""; | |
| #[cfg(not(feature = "probes"))] | ||
| static PROBES_ENABLED: &str = "probeless"; | ||
|
|
||
| pub(crate) fn version(cli: &Cli) -> Option<String> { | ||
| if cli.version { | ||
| Some(format!( | ||
| "{} {} {}", | ||
| env!("CARGO_BIN_NAME"), | ||
| env!("CARGO_PKG_VERSION"), | ||
| PROBES_ENABLED | ||
| )) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn validate_clap() { | ||
| OuterCli::command().debug_assert() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a note, but it's probably not worth fixating on because this isn't new code:
It feels like there should be a nicer way to specify this, or at least, the semantics of what we've chosen as "the acceptable set of configuration combinations" feels overly complicated. Especially the repeated "is_some", matches, and asserts. I feel like I need a flowchart to follow this now :D
I have no calibration for "necessary complexity" vs "incidental complexity" here, but just leaving a note as an uninformed newcomer that "whew, this feels like a lot". It might be worth having a couple of functions for each acceptable source/combo of options to make it easier to hang documentation off of, so folks know what is/isn't allowed.