Skip to content

Export exit codes to JSON output#371

Merged
sharkdp merged 3 commits intosharkdp:masterfrom
JordiChauzi:export_exit_codes_to_json
May 10, 2021
Merged

Export exit codes to JSON output#371
sharkdp merged 3 commits intosharkdp:masterfrom
JordiChauzi:export_exit_codes_to_json

Conversation

@JordiChauzi
Copy link
Copy Markdown
Contributor

Fix #348.

I added the field exit_codes: Vec<i32> to the BenchmarkResult struct.

However, on unix, the returned ExitStatus can be None, when the process is terminated by a signal. In that case, I used the signal method result (from ExitStatusExt for unix) as exit code. This part is located in the extract_exit_code function in benchmark.rs. I tried to document the unwrap uses (I found other instances of unwrap in the library so I assumed it was ok to use it here).

Comment on lines +217 to +220
/* 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.
*/
Copy link
Copy Markdown
Owner

@sharkdp sharkdp Apr 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JordiChauzi Are you interested in finishing this? Otherwise, that's also fine! We can find someone else then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I like the null solution better. You are right. Thanks!

@sharkdp
Copy link
Copy Markdown
Owner

sharkdp commented Apr 3, 2021

Thank you very much for your contribution!

This looks great. I added one inline comment.

"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())
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! I could not easily find a constant in libc or in the std library, so I hardcoded 128 with a comment.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sharkdp sharkdp merged commit 7e23087 into sharkdp:master May 10, 2021
@sharkdp
Copy link
Copy Markdown
Owner

sharkdp commented Oct 17, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export exit codes to JSON

2 participants