refactor: pid1-exe

[polarathene]

This PR is only applying changes to the pid1-exe package, I've avoided touching the library until feedback of this PR is received.

• Originally, I just wanted to correct some typos, and improve the --help output. I tidied up the CLI code a bit along the way, it should not introduce any regressions.
• Commit history is reasonably clean, but not ideal. Given PR history in the project thus far, if you're comfortable with the PR being squashed to a single commit with a reference to this PR for context, please use "Squash" as the merge strategy (rather than the default "Merge commit").

Functional changes (beyond --help output)

• -- is now optional, the clap parser won't swallow options from the RHS of the command positional arg.
• Refuse to compile on Windows (or any other non-unix platform).

  • Cargo.toml now restricts the pid1 dep to unix platforms only. As the other deps are cross-platform, it's best practice AFAIK to leave those without a constraint (thus they are downloaded/compiled), even though the CLI itself will fail to compile.

  • Compilation for Windows now fails like so:

    $ cargo +stable zigbuild --quiet --bin pid1 --release --target x86_64-pc-windows-gnu
    
    Compiling pid1-exe v0.1.6 (/opt/pid1-rs/pid1-exe)
    error: `pid1` is only compatible with Unix-like operating systems.
     --> pid1-exe/src/main.rs:9:3
      |
    9 |   compile_error!("`pid1` is only compatible with Unix-like operating systems.");
      |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
    error: could not compile `pid1-exe` (bin "pid1") due to 1 previous error

  • The prior "implementation" seemed rather misleading? For which no context was available on related PRs where these additions slipped in (presumably the intent was to avoid noisy compile errors?):

    • Implement builder pattern #7 (lib)
    • Fix potential race condition #11 (bin)

--help output comparison

0.1.6:

Usage:

Arguments:
  <COMMAND>  Process to run
  [ARGS]...  Arguments to the process

Options:
  -w, --workdir <DIR>        Specify working direcory
  -t, --timeout <TIMEOUT>    Timeout (in seconds) to wait for child proess to exit [default: 2]
  -v, --verbose              Turn on verbose output
  -e, --env <ENV>            Override environment variables. Can specify multiple times
  -u, --user-id <USER_ID>    Run command with user ID
  -g, --group-id <GROUP_ID>  Run command with group ID
  -h, --help                 Print help

Revised via this PR:

A Unix PID1 process wrapper for signal handling and zombie reaping

Usage: pid1 [OPTIONS] <COMMAND> [ARGS]...

Arguments:
  <COMMAND>  Process to run
  [ARGS]...  Arguments to that process

Options:
  -w, --workdir <DIR>        Specify working directory
  -t, --timeout <SECONDS>    Grace period for stopping before escalating to SIGKILL [default: 2]
  -v, --verbose              Turn on verbose output
  -e, --env <KEY=VALUE>      Override environment variables. Can specify multiple times
  -u, --user-id <USER ID>    Run command with user ID
  -g, --group-id <GROUP ID>  Run command with group ID
  -h, --help                 Print help
  -V, --version              Print version

Diff from 0.1.6:

• Added program description and usage syntax
• --workdir description typo direcory => directory
• --timeout description typo ~~proess=>process`~~ (EDIT: Revised description entirely)
• --env option value format communicates expected structure
• Added --version option

Full change overview

The diff should be fairly straight-forward, but if additional context for anything is needed, hopefully this documents my reasoning well enough 😅

• Removed cfg attributes for conditional compilation (along with windows compilation "support").
  • Only unix platforms are actually supported, make that explicit with an compilation failure on unsupported platforms. compile_error! is used for a friendly failure message.
  • #[cfg(target_family = "unix")] shortened to alias #[cfg(unix)]
• The clap:usage crate feature was added for generating the usage in --help output, previously this was blank. Alternatively it could be set with a hard-coded string if not adding the usage feature.
• Pid1App refactored:
  • Corrected typos in doc comments and introduced white-space for improved readability.
  • When user_id/group_id were added, they were excluded from pub(crate) visibility already established on other fields in the struct. Added for consistency.
  • The command attribute annotating the struct adds a --version option and about (-h/--help) that sources description from Cargo.toml. The existing description in Cargo.toml was not accurate, I iterated through a few variations before settling on the one committed, though I'm not sure how well "signal handling" maps to what is effectively only support for graceful exit? (SIGCHLD aside for zombie reaping)
  • --timeout description revised to more clearly communicate it's a grace period that escalates to SIGKILL, rather than just a timeout on the child process exiting without context to an outcome.
  • arg attributes adjusted to remove implicit defaults. Additionally --env + --timeout have more helpful contextual value_name set, while --user-id/--group-id remove the _ (the previous values here were implicit already from the field name).
  • The command field arg attribute now has trailing_var_arg = true.
    • The -- separator is now optional between the wrapped command and pid1-exe CLI options.
    • Previously without --, clap would treat anything other than positional args on the RHS of command parsed as part of the pid1-exe options (eg: --help).
    • --env removed the value_parser in favor of a impl FromStr and wrapper type KeyValue(OsString, OsString) instead of (OsString, OsString).
      • The main benefit for this is simpler logic )(and as a bonus, simpler interoperability with other CLI crates such as bpaf).
      • NOTE: Adapted the KeyValue type from the wasmi project clap CLI (which in an earlier iteration had a familiar looking error string).
• Pid1App::run() refactored:
  • Introduced white-space formatting for better readability / maintainability.
  • child => cmd for the let binding that's actually the configured/parsed command to execute. There's also no need for the rebinding directly after to set the args. The args can use the Vec type as-is instead of slice syntax (the vec is casted implicitly to a slice).
  • Expanded on the signal install comment, I was a bit curious about it but the only context from git blame was potential race condition without any other details.
    • This could technically be shifted to occur prior to the pid != 1 condition, but registering the signals is only relevant when pid == 1, so would be redundant (given the exec/exit paths).
    • Personally since this logic has a variation in the pid1 lib, it seems more appropriate to have a convenience in the lib for a CLI to call, simplifying any CLI implementation.
  • Some control flow was adjusted to reduce the nested indentation.
    • pid != 1 conditional does not need to wrap subsequent logic in an else branch, as it already performs exec into the command, with process exit if that fails.
      • I considered adding a comment for clarity that when not running as PID1 that zombie reaping and signal propagation from pid1 lib was of no value, but could not express such tersely without language that was already similarly conveyed in the small logic for that branch 😅 (thus assumed reader is familiar with why)
      • I somewhat expected a log when --verbose was used to highlight when not PID1, I think tini/dumb-init can do such. I could add a follow-up PR addressing that.
    • There's no need for the double let child binding with .spawn() + match { ... }? Simplified to using .spawn().unwrap_or_else( ... ).

[psibi]

Refuse to compile on Windows (or any other non-unix platform)

We want this to compile on Windows platform as this is often a dependency on our projects which can be used on Windows platforms.

[polarathene]

TL;DR: You can still support windows fine, but as pid1 isn't actually relevant to that platform, it's 2 extra lines to clearly communicate that in your project's source code whilst avoiding any compilation concerns with the target.

---

We want this to compile on Windows platform as this is often a dependency on our projects which can be used on Windows platforms.

Uhh... then that is a mistake on your end? You clearly have no Windows support in the CLI project, so compiling it for Windows achieves nothing.

For the library, it's the same concern and integration should properly communicate that.

Presently your lib docs and README advise the following:

use pid1::Pid1Settings;
use std::time::Duration;

fn main() {
    Pid1Settings::new()
        .enable_log(true) // Optional: for debugging
        .timeout(Duration::from_secs(2)) // Optional: timeout for graceful shutdown
        .launch()
        .expect("Launch failed");

    // Rest of the logic...
    println!("Hello world");
}

When your actual project is cross-platform and wants to integrate pid1 for unix platforms, but otherwise not use it for the others, you'd do something similar to what I've done in this PR but without the compiler error.

It's literally 2 lines?:

Cargo.toml: (don't download/compile the pid1 crate for targets that aren't unix platforms)

+[target.'cfg(unix)'.dependencies]
pid1 = "0.1.6"

main.rs: (conditionally use pid1 only when the target is a unix platform):

fn main() {
+   #[cfg(unix)]
    pid1::Pid1Settings::new()
        .enable_log(true) // Optional: for debugging
        .timeout(std::time::Duration::from_secs(2)) // Optional: timeout for graceful shutdown
        .launch()
        .expect("Launch failed");

    // Rest of the logic...
    println!("Hello world");
}

That's sufficient no?

---

Presently your Windows platform support for compilation with the library crate is just:

pid1-rs/pid1/src/lib.rs

Lines 26 to 35 in 2e8ec76

	/// Relaunch process as PID with default value of [`Pid1Settings`]
	#[cfg(target_family = "unix")]
	pub fn relaunch_if_pid1() -> Result<(), Error> {
	Pid1Settings::default().launch()
	}
	#[cfg(target_family = "windows")]
	pub fn relaunch_if_pid1() -> Result<(), Error> {
	Ok(())
	}

pid1-rs/pid1/src/lib.rs

Lines 106 to 112 in 2e8ec76

	#[cfg(target_family = "windows")]
	pub fn launch(self) -> Result<(), Error> {
	if self.log {
	eprintln!("pid1-rs: PID1 capability not supported for Windows");
	}
	Ok(())
	}

So you're building the pid1 crate on Windows to what... avoid the two changes on a project using the library that explicitly communicate "this is only relevant to unix"?

Why would you want to hide that detail for such a minor convenience? (it's not exactly helpful to imply Windows support at a glance, but then realize that's not the case at all, far better to be explicit)

---

FWIW, besides pid1-exe, there are only 2 public dependents listed at crates.io:

• watchexec uses a pid1 crate feature for conditional compilation (probably because this project doesn't make it clear on the unix only constraint? Their Cargo.toml doesn't have a platform constraint, and the pid1 feature is enabled by default)
• blendwerk has no cfg attribute used (Cargo.toml is a plain dependency, not marked optional)

Again, the changes in this PR are not breaking for these consumers of the pid1 lib, I've not adjusted anything to do with the library crate in this PR, only the binary crate.

Making the change to the library (via a separate PR) would require a semver bump to 0.2.0 as it would otherwise break existing projects if they target non-unix platforms 👍

[polarathene]

If the improvement to handling platforms correctly is still an issue despite my feedback, let me know and I can remove that from this PR.

Are the other changes welcomed?

[psibi]

Overall LGTM, I believe the CI should pass.

[psibi]

Any reason why we are removing required = true ?

[polarathene]

It can be added back if you prefer it for clarity.

I removed it as it is implicitly true as a default (for a positional arg). Similarly a positional arg of Vec is implicitly required = false.

[polarathene]

It is late here, I'll look into the test failures tomorrow. Not quite sure from the failure outputs on Ubuntu runners why two of the tests failed from these PR changes.

[polarathene]

I don't see any relationship between the pid1 tests in sanity.rs that are failing, and a dependence upon pid1-exe? The changes in this PR likely haven't introduced any regression to those tests but an issue with the test suite itself?

Consulting Gemini as I'm not familiar with your test suite:

• It suggests that it may be racey with the parallel container approach, and that you should consider something like cargo test -- --test-threads=1 to run the tests synchronously instead.
• It's other suggestion was related to the -t / --tty option invoked with docker exec, and an issue related to that when running multiple such commands in parallel across threads from a single process (cargo test), with the stdout/stderr pipes and some other context (that sounded like gibberish / hallucination).

Unfortunately with that context, and my lack of expertise in this scenario, I could not deduce how much of this advice from Gemini was legitimate.

• I do maintain a project with a test suite that does run containers in parallel fine, with some exceptions where we need to ensure serial execution of test cases.

• I recall the usage of docker run with -t/-tty caused a weird discrepancy in a test case years ago, but it was instead related to appending a CRLF to output 🤔

  # `CR` (`\r`) is introduced by the `-t` / `--tty` option:
  $ docker run --rm -it alpine ash -c 'echo foo' > bar; bat --plain --show-all bar
  foo␍␊
  
  # The expected LF line-ending instead of CRLF:
  $ docker run --rm alpine ash -c 'echo foo' > bar; bat --plain --show-all bar
  foo␊
  
  # Running with `-t` but `bar` receives stdout within the container:
  # - For context `foo` output is actually displayed with bold weight and `␊` is coloured red,
  #   these adjustments apply to all other outputs within this snippet.
  # - While those visual styles can't be displayed in this snippet...
  #   if not using the `-t`/`--tty` CLI flag, the `bat` command output changes based on:
  #   - Run inside the container: Plain unstylized text (No TTY detected)
  #   - Run outside the container: Visual styles apply (The terminal itself has it's own TTY)
  $ docker run --rm -it alpine ash -c 'apk add bat; echo "foo" > bar; bat --plain --show-all bar'
  foo␊
  
  # Again, but this time without `-i` / `--interactive`:
  # This exits with the terminal receiving input (as if typed / pasted) of:
  # `10;rgb:cccc/cccc/cccc11;rgb:0c0c/0c0c/0c0c61;4;6;7;14;21;22;23;24;28;32;42;52c`
  $ docker run --rm -t alpine ash -c 'apk add bat; echo "foo" > bar; bat --plain --show-all bar'
  foo␊0;rgb:cccc/cccc/cccc^[\^[]11;rgb:0c0c/0c0c/0c0c^[\^[[?61;4;6;7;14;21;22;23;24;28;32;42;52c

  Apparently this distinction is expected from a TTY, it informs the terminal to display output correctly as intended, but as shown above with the bat program revealing the line-ending invisibles changing, that can result in the bar file produced not matching expectations (which in that linked project was a problem causing a test failure).

  I'm not quite sure about the final output and behaviour from lack of -i, presumably bat is detecting a TTY and sending all that to interact with the TTY but the lack of -i results in that spilling out to the actual terminal STDIN, whereas with -i/--interactive on the docker CLI options it'd receive that to manipulate the pseudo-TTY?

---

I looked at our test suite and we specifically:

• Only call docker exec without the -t/--tty flag.
• Allocate a TTY to a new container instance (via docker create / docker run). I've not verified if we need that (and if so why it'd be needed for tests)

I'm not sure exactly when you'd need -t alone, but I know for interactive use locally in a terminal -it is a useful pair. However for running in CI perhaps it does cause problems here in the tests (and might explain why there was no issue noticed with -t usage when running the tests locally via a terminal app?)

Other notes:

• Github Actions has an issue about not providing a TTY (although Docker can apparently workaround that and emulate one, it might only apply to docker run/docker create?)
• Docker Compose differs with it's implicit -it on exec when available, but since Docker Compose V2, --no-TTY/-T should be implicit in environments not compatible with -it.

[polarathene]

I've gone ahead an made the changes to drop -t / --tty from the docker run / docker exec commands, as they're not necessary and likely causing issues in CI as per my prior comment above.

I've also revised your Development.md docs.

---

The output from 2023 is from a different simple.rs program that was calling date, which no longer happens:

2023:

pid1-rs/examples/simple.rs

Lines 15 to 23 in a416936

	for _ in 0..4 {
	std::thread::sleep(std::time::Duration::from_secs(2));
	std::process::Command::new("date").spawn().unwrap();
	}
	if args.len() > 1 {
	println!("Going to sleep 500 seconds");
	std::thread::sleep(std::time::Duration::from_secs(500));
	}

2026:

pid1-rs/pid1/examples/simple.rs

Lines 15 to 31 in 2e8ec76

	if args.iter().any(|arg| arg == "--create-grandchildren") {
	println!("Spawning 3 grandchildren processes that will be orphaned.");
	for _ in 0..3 {
	// These children will be orphaned when `simple` exits, and then
	// adopted and reaped by pid1.
	std::process::Command::new("sleep").arg("1").spawn()?;
	}
	}
	if let Some(pos) = args.iter().position(|r| r == "--sleep") {
	if let Some(duration_str) = args.get(pos + 1) {
	if let Ok(duration) = duration_str.parse::<u64>() {
	println!("Going to sleep {duration} seconds");
	std::thread::sleep(std::time::Duration::from_secs(duration));
	}
	}
	}

I'm not even sure how relevant the information is in this document now that you've got what looks to be the equivalent tests covered in your sanity.rs?

The justfile itself doesn't appear to provide a means to setup a container running in the background, so it's unclear how the reader is meant to use it as a convenience with the current guidance it provides.

• In the original 2023 ref above the just test recipe invokes the container to run without any args, /sleep is implicit and performs several date calls like shown above, but 2026 variant explicitly requires --sleep with a duration or it exits much sooner.
• In the 2023 justfile, the run-image recipe task provides --sleep, which adds a 500 sec sleep. The 2026 equivalent recipe provides --sleep 20, but either way this is done in the foreground, so another terminal would need to be opened.

Log for process not running as PID 1 was dropped since too:

pid1-rs/src/lib.rs

Lines 43 to 57 in a416936

	pub fn relaunch_if_pid1(option: Pid1Settings) -> Result<(), Error> {
	let pid = std::process::id();
	if pid == 1 {
	let child = relaunch()?;
	if option.log {
	eprintln!("pid1-rs: Process running as PID 1");
	}
	pid1_handling(option, child)
	} else {
	if option.log {
	eprintln!("pid1-rs: Process not running as Pid 1: PID {pid}");
	}
	Ok(())
	}
	}

pid1-rs/pid1/src/lib.rs

Lines 92 to 105 in 2e8ec76

	pub fn launch(self) -> Result<(), Error> {
	let pid = std::process::id();
	if pid == 1 {
	// Install signal handles before we launch child process
	let signals = Signals::new([SIGTERM, SIGINT, SIGCHLD]).unwrap();
	let child = relaunch()?;
	if self.log {
	eprintln!("pid1-rs: Process running as PID 1");
	}
	pid1_handling(self, signals, child)
	} else {
	Ok(())
	}
	}

Logs can appear a bit racey for the immediate exit, where it appears to reap before the logs in this case, but can equally output as reaped after:

$ docker run --rm --name pid1rs pid1rstest

pid1-rs: Process running as PID 1
pid1-rs: Reaped PID 7
In the simple process, going to sleep. Process ID is 7
Args: ["/simple"]

[polarathene]

@psibi Could you please run the workflows again to confirm tests in CI pass? Thank you