isolate and handle unix error

[seun-ja]

Aims to Fix #3161

[itowlson]

This conflates a process exit code with an operating system signal number, and won't help with the original problem - it will just make it appear as if the subprocess has exited with a weird exit code.

A possible way forward is to make ExitStatusError an enum e.g.

pub enum ExitStatusError {
    ExitCode(i32),
    Signal(i32),
    Unknown,
}

Then the message can report which case was in play, distinguishing the signal case from the process exit case.

(I am not sure if we can translate the i32 signal number into a meaningful-ish string - that would be great if we could.) (ETA from light googling this looks like it would be a pain but maybe someone knows better)

[lann]

nit: Just in case someone wants to try compiling spin to wasm some day we could make things slightly easier for them. 🙂

Suggested change

	#[cfg(windows)]
	#[cfg(not(unix))]

[lann]

It would be good to at least try to avoid confusion between exit codes and signals, even though its only slightly within our control here.

https://tldp.org/LDP/abs/html/exitcodes.html

128+n Fatal error signal "n"
255 Exit status out of range

Suggested change

	match self {
	Self::ExitCode(code) => *code,
	Self::Signal(signal) => *signal,
	Self::Unknown => 1,
	}
	// Exit code conventions: https://tldp.org/LDP/abs/html/exitcodes.html
	match self {
	Self::ExitCode(code) => *code,
	Self::Signal(signal) => 128 + *signal,
	Self::Unknown => 1,
	}.min(255)

[seun-ja]

Thanks for sharing the docs, I've been curious to know why the unknown was initially set at 1. Makes more sense now!

[lann]

We should expose the new detail we have:

Suggested change

	Self::Signal(signal) => writeln!(f, "{signal}"),
	Self::Signal(signal) => writeln!(f, "signal {signal}"),

[itowlson]

I separated out the Zig CI stuff into a single-purpose PR, so please revert all GH workflow changes out of this one - thanks!

But the signals stuff looks great - thank you for improving this! Happy to approve once the CI stuff rolls back.

[itowlson]

This is extremely nitty but status: signal 7 reads a wee bit oddly to me. How would you feel about e.g. "subprocess exited with exit code 5", "subprocess exited with signal 7", "subprocess exited with unknown status"? Something like

let _ = write!(f, "subprocess exited with ");
match self {
    Self::ExitCode(code) => write!(f, "exit code {code}"),
    Self::Signal(signal) => write!(f, "signal {signal}"),
    Self::ExitCode(code) => write!(f, "unknown status"),
}

Definitely not a blocker though and there may be others who prefer to leave as is!

[seun-ja]

Yes, this is better. Would go for this, thanks!

[itowlson]

There shouldn't be any changes needed in the build.yml I think?

[seun-ja]

It's only formatting. My IDE auto-formatted the file when I worked on it. The CI still works.

Should I revert the changes?

[itowlson]

Yes please. We mostly try to avoid PRs containing unrelated changes to different files, even formatting, because it makes the file history harder to follow.

[itowlson]

Thanks - looks good!