Construct `ExecutionContext` in `humility-bin` by mkeeter · Pull Request #677 · oxidecomputer/humility

mkeeter · 2026-05-21T20:41:21Z

(staged on #666)

ExecutionContext::new is a weird function: it usually builds an ExecutionContext, but sometimes bails out unceremoniously with std::process::exit. This is because it's also handling a final set of subcommands-as-cli-flags, e.g. --list-targets.

In this PR, we move all of that logic into humility-bin, which is already designed to exit with a std::process::ExitCode (instead of calling exit(..) directly).

Logic should be the same, but this moves --list-targets handling earlier (which should fix #597).

jamesmunns

A lot of pedantic nits, feel free to take them or leave them :)

jamesmunns · 2026-05-22T11:33:38Z

+        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 {}));


Wait how is parse returning Infallible here?

Over in #668, we made IP parsing lazier – the parse captures a Result<..> which is only checked if we're actually using the IP address.

jamesmunns · 2026-05-22T11:41:13Z

+    // 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()


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.

As noted in the rework RFD, i would like to get rid of environments and just have other tooling set HUMILITY_PROBE and HUMILITY_ARCHIVE as appropriate. Environments solved an actual problem for us but I do not think the belong in a humility binary.

jamesmunns · 2026-05-22T11:42:13Z

+            // Cannot specify a dump/probe/IP address and also an
+            // environment and target
+            //
+            assert!(cli.dump.is_none());


Maybe turn these asserts into ExitCode::FAILUREs too, to be polite?

Mutual exclusion between these options is enforced by clap; I'll add a comment.

jamesmunns · 2026-05-22T11:43:28Z

+            PROBES_ENABLED
+        );
+        if cmd.is_some() {
+            humility::warn!("ignoring subcommand after printing version");


Already in scope

Suggested change

humility::warn!("ignoring subcommand after printing version");

warn!("ignoring subcommand after printing version");

Good catch, it wasn't in scope in the previous file that I copied from

jamesmunns · 2026-05-22T11:43:30Z

+    // The --version (-V) and --list-targets operations are implemented as
+    // flags, rather than subcommands, so we'll special-case them here.
+    if cli.version {
+        eprintln!(


When do we use humility::warn!, humility::msg!, and eprintln!?

Right now, it's purely based on vibes! We'll want to standardize on log, but that's further down the line.

labbott

Part of me really wants to have a separate binary for dealing with humility dumps simply so the logic here isn't so complicated. I have convinced myself this is fine but it should probably be spot checked against our lab machines for good measure.

labbott · 2026-05-27T19:15:05Z

+    // 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()


As noted in the rework RFD, i would like to get rid of environments and just have other tooling set HUMILITY_PROBE and HUMILITY_ARCHIVE as appropriate. Environments solved an actual problem for us but I do not think the belong in a humility binary.

mkeeter requested review from hawkw and jamesmunns May 21, 2026 20:41

mkeeter force-pushed the mkeeter/context-building branch from b85f4b5 to 92ebd5d Compare May 21, 2026 20:50

mkeeter force-pushed the mkeeter/subcommand-enum branch from f49aa94 to 20d913b Compare May 21, 2026 20:50

jamesmunns reviewed May 22, 2026

View reviewed changes

mkeeter force-pushed the mkeeter/context-building branch 4 times, most recently from 3a25b11 to bbb641c Compare May 26, 2026 14:20

mkeeter force-pushed the mkeeter/subcommand-enum branch 2 times, most recently from 52f87ad to fa8ee6b Compare May 27, 2026 17:18

mkeeter force-pushed the mkeeter/context-building branch from bbb641c to c1ffe04 Compare May 27, 2026 17:18

labbott approved these changes May 27, 2026

View reviewed changes

mkeeter force-pushed the mkeeter/context-building branch from c1ffe04 to 17f9a92 Compare May 27, 2026 19:30

mkeeter force-pushed the mkeeter/subcommand-enum branch from fa8ee6b to 2b6fcbb Compare May 27, 2026 19:30

mkeeter force-pushed the mkeeter/context-building branch 2 times, most recently from a32ff10 to 840972b Compare May 27, 2026 19:48

Construct ExecutionContext in humility-bin

f1ce3d7

Base automatically changed from mkeeter/subcommand-enum to master May 27, 2026 20:59

mkeeter force-pushed the mkeeter/context-building branch from 840972b to f1ce3d7 Compare May 27, 2026 21:01

	humility::warn!("ignoring subcommand after printing version");
	warn!("ignoring subcommand after printing version");

Conversation

mkeeter commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesmunns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

labbott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkeeter commented May 21, 2026 •

edited

Loading