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

IPC::Run::full_results() Windows behavior does not match non-Windows #161

Closed
nmisch opened this issue Aug 22, 2022 · 3 comments
Closed

IPC::Run::full_results() Windows behavior does not match non-Windows #161

nmisch opened this issue Aug 22, 2022 · 3 comments

Comments

@nmisch
Copy link
Collaborator

nmisch commented Aug 22, 2022

This affects the other *result* methods, too. t/result.t fails like this on
Windows:

#   Failed test 'Results of all processes'
#   at t/result.t line 16.
#     Structures begin differing at:
#          $got->[2] = '0'
#     $expected->[2] = '42'

#   Failed test 'First non-zero result'
#   at t/result.t line 17.
#          got: ''
#     expected: '42'

#   Failed test 'Result of the third process'
#   at t/result.t line 20.
#          got: '0'
#     expected: '42'

#   Failed test 'Full results of all processes'
#   at t/result.t line 22.
#     Structures begin differing at:
#          $got->[2] = '42'
#     $expected->[2] = '10752'

#   Failed test 'Full result of the third process'
#   at t/result.t line 26.
#          got: '42'
#     expected: '10752'
# Looks like you failed 5 tests of 10.
[23:28:27] t/result.t ........................... 
Dubious, test returned 5 (wstat 1280, 0x500)
Failed 5/10 subtests 

(We don't see this failure in GitHub Actions, since GITHUB_WINDOWS_TESTING
makes the test a TODO. t/result.t, being absent from MANIFEST, is not part
of the distribution. Hence, we don't see this failure in cpantesters, either.
Users building from git see the failure. I plan to change that.)

The mismatch arises because Win32::Process::GetExitCode() codes aren't
bit-shifted the way wait() status values are. The documentation doesn't settle
the question of what the user should expect. For example, full_results()
documentation says "Returns a list of child exit values as returned by wait".
We don't use wait() on Windows. I could document the mismatch, or I could
change behavior to compensate for the mismatch:

--- a/lib/IPC/Run.pm
+++ b/lib/IPC/Run.pm
@@ -3466,7 +3466,7 @@ sub reap_nb {
                 $? = $kid->{RESULT} = 0x0F;
             }
             else {
-                $? = $kid->{RESULT} << 8;
+                $? = $kid->{RESULT} = $kid->{RESULT} << 8;
             }
         }
         else {

I'd certainly pick the latter in a greenfield, but it is a compatibility break.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/test/perl/PostgreSQL/Test/Utils.pm;h=1ca2cc591708be41c79f02de542e7e64feeba9c5;hb=HEAD#l785
is an example of code compensating for the current mismatch; that would need to
change. Are there other options to consider? What do you prefer?

That addresses the exit 42 part of t/result.t, but it doesn't address the
SIGKILL part. Per https://perldoc.perl.org/perlport#kill, Perl's signal
emulation makes the victim call exit(9). I'll just make t/result.t
recognize the platform-specific outcome. IPC::Run can't recreate a distinction
that Perl signal emulation erased.

@nmisch nmisch changed the title IPC::Run::full_results() Windows behavior does not match non-Unix IPC::Run::full_results() Windows behavior does not match non-Windows Aug 22, 2022
@toddr
Copy link
Member

toddr commented Aug 22, 2022

I continue to wonder if it's a losing battle to expect perl code to behave exactly the same on a non-POSIX system (windows) vs a POSIX system (almost everything else).

Signals have always been a complete consistency mess, especially if you stretch back to windows XP.

At this point IPC::Run has so many inconsistencies, I'm tempted to suggest that if we can break some compatibility but get to a consistent state in a short period of time, then we declare this and proceed. What do you think?

@mohawk2
Copy link
Collaborator

mohawk2 commented Aug 22, 2022

I think consistency is important enough here to justify some breakage.

@nmisch
Copy link
Collaborator Author

nmisch commented Aug 26, 2022

I like accepting a compatibility break here to improve consistency. Given the
Windows compatibility break in the last release, the time is relatively good for
more breaks. Thanks for the feedback.

nmisch added a commit to nmisch/IPC-Run that referenced this issue Nov 7, 2022
For all platforms, a child calling exit(N) will now cause result() to
return N and full_result() to return N << 8.  Programs having
workarounds for the former Windows-specific semantics may need to
condition those workarounds on IPC::Run versions before the next.

Fixes cpan-authors#161
@nmisch nmisch closed this as completed in 894088d Nov 13, 2022
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

No branches or pull requests

3 participants