Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stringification of abnormal Process::Status on Windows [fixup #15255] #15267

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

straight-shoota
Copy link
Member

The change in #15255 broke Process::Status#to_s and #inspect on Windows due to the redefinition of #normal_exit?. Unfortunately we were missing specs for this, so it did go unnoticed.

This patch adds specs and restablishes the previous behaviour with the small improvement of treating the exit_status value as UInt32 instead of Int32, which results in positive numbers.

# Crystal 1.14.0
Process::Status.new(LibC::STATUS_CONTROL_C_EXIT)).inspect # => "Process::Status[-1073741510]"

# master
Process::Status.new(LibC::STATUS_CONTROL_C_EXIT)).inspect # NotImplementedError: Process::Status#exit_signal

# this patch
Process::Status.new(LibC::STATUS_CONTROL_C_EXIT)).inspect # => "Process::Status[3221225786]"

There's still room for improvement, for example map the values to names and/or use base 16 numerals as is custom for error statuses on Windows), but I'll leave that for a follow-up.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:system kind:regression Something that used to correctly work but no longer works labels Dec 11, 2024
@straight-shoota straight-shoota self-assigned this Dec 11, 2024
@straight-shoota straight-shoota added this to the 1.15.0 milestone Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants