Skip to content

Commit

Permalink
Fix stringification of abnormal Process::Status on Windows [fixup #…
Browse files Browse the repository at this point in the history
…15255] (#15267)

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.

```cr
# 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.
  • Loading branch information
straight-shoota authored Dec 14, 2024
1 parent 5915e3b commit 8f12767
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 15 deletions.
16 changes: 16 additions & 0 deletions spec/std/process/status_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,14 @@ describe Process::Status do
assert_prints Process::Status.new(exit_status(255)).to_s, "255"
end

it "on abnormal exit" do
{% if flag?(:win32) %}
assert_prints status_for(:interrupted).to_s, "3221225786"
{% else %}
assert_prints status_for(:interrupted).to_s, "INT"
{% end %}
end

{% if flag?(:unix) && !flag?(:wasi) %}
it "with exit signal" do
assert_prints Process::Status.new(Signal::HUP.value).to_s, "HUP"
Expand All @@ -254,6 +262,14 @@ describe Process::Status do
assert_prints Process::Status.new(exit_status(255)).inspect, "Process::Status[255]"
end

it "on abnormal exit" do
{% if flag?(:win32) %}
assert_prints status_for(:interrupted).inspect, "Process::Status[3221225786]"
{% else %}
assert_prints status_for(:interrupted).inspect, "Process::Status[Signal::INT]"
{% end %}
end

{% if flag?(:unix) && !flag?(:wasi) %}
it "with exit signal" do
assert_prints Process::Status.new(Signal::HUP.value).inspect, "Process::Status[Signal::HUP]"
Expand Down
42 changes: 27 additions & 15 deletions src/process/status.cr
Original file line number Diff line number Diff line change
Expand Up @@ -275,11 +275,15 @@ class Process::Status
# `Process::Status[Signal::HUP]`.
def inspect(io : IO) : Nil
io << "Process::Status["
if normal_exit?
exit_code.inspect(io)
else
exit_signal.inspect(io)
end
{% if flag?(:win32) %}
@exit_status.to_s(io)
{% else %}
if normal_exit?
exit_code.inspect(io)
else
exit_signal.inspect(io)
end
{% end %}
io << "]"
end

Expand All @@ -288,22 +292,30 @@ class Process::Status
# A normal exit status prints the numerical value (`0`, `1` etc).
# A signal exit status prints the name of the `Signal` member (`HUP`, `INT`, etc.).
def to_s(io : IO) : Nil
if normal_exit?
io << exit_code
else
io << exit_signal
end
{% if flag?(:win32) %}
@exit_status.to_s(io)
{% else %}
if normal_exit?
io << exit_code
else
io << exit_signal
end
{% end %}
end

# Returns a textual representation of the process status.
#
# A normal exit status prints the numerical value (`0`, `1` etc).
# A signal exit status prints the name of the `Signal` member (`HUP`, `INT`, etc.).
def to_s : String
if normal_exit?
exit_code.to_s
else
exit_signal.to_s
end
{% if flag?(:win32) %}
@exit_status.to_s
{% else %}
if normal_exit?
exit_code.to_s
else
exit_signal.to_s
end
{% end %}
end
end

0 comments on commit 8f12767

Please sign in to comment.