Export exit codes to JSON output #371
Conversation
src/hyperfine/benchmark.rs
Outdated
| /* From the ExitStatus::code documentation: | ||
| "On Unix, this will return None if the process was terminated by a signal." | ||
| On the other configurations, ExitStatus::code should never return None. | ||
| */ |
There was a problem hiding this comment.
Ok, but I don't really see a guarantee for the latter part ("On the other configurations, ExitStatus::code should never return None.") in the documentation.
I think it might be best to simply return an Option<i32> from this function and later (in the export) convert exit codes to a string that either contains the exit code or an empty string.
This would also allow us to remove all the unwraps.
What do you think?
There was a problem hiding this comment.
@JordiChauzi Are you interested in finishing this? Otherwise, that's also fine! We can find someone else then.
There was a problem hiding this comment.
Sorry about the delay. Last month was a little busy and I kind of forgot about this PR.
Anyway, I will try and implement the changes in the next days.
You are right, there is no real guarantee that a configuration may return None from the ExitStatus::code method (this is currently not the case but again, this is not a guarantee). Printing an empty string in that case makes sense. Do we also want to store None instead of unwrapping when the ExitStatus::signal method returns None?
There was a problem hiding this comment.
The default behavior for serde and outputting json is to serialize None as null. This will create outputs that look like this:
...
"exit_codes": [
0,
0,
0,
1,
null,
null,
42,
0,
0,
0
]
...To me, it looks "good enough". If you still want the empty string behavior tell me so. If this is the case, I think I will have to add a custom serialize method pour the exit_codes field?
There was a problem hiding this comment.
No, I like the null solution better. You are right. Thanks!
|
Thank you very much for your contribution! This looks great. I added one inline comment. |
src/hyperfine/benchmark.rs
Outdated
| "On Unix, this will return None if the process was terminated by a signal." | ||
| In that case, ExitStatusExt::signal should never return None. | ||
| */ | ||
| status.code().or_else(|| status.signal()) |
There was a problem hiding this comment.
I think this should either return the exit code (if it's there) OR the signal n + 128. This is a standard procedure, e.g. in Bash: https://tldp.org/LDP/abs/html/exitcodes.html
Returning just the signal number (e.g. 9 for KILL) could be confused with the actual exit code 9.
There was a problem hiding this comment.
You are right! I could not easily find a constant in libc or in the std library, so I hardcoded 128 with a comment.
Fix #348.
I added the field
exit_codes: Vec<i32>to theBenchmarkResultstruct.However, on
unix, the returnedExitStatuscan beNone, when the process is terminated by a signal. In that case, I used thesignalmethod result (fromExitStatusExtforunix) as exit code. This part is located in theextract_exit_codefunction inbenchmark.rs. I tried to document theunwrapuses (I found other instances ofunwrapin the library so I assumed it was ok to use it here).