Skip to content

Add exit code test (GH #19020): #19152

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

Open
wants to merge 1 commit into
base: blead
Choose a base branch
from

Conversation

xsawyerx
Copy link
Member

This tests that when we kill a process through a shell, we get the correct exit code and signal.

Unfortunately, this fails on some versions of Ubuntu and dash, which I believe shows a bug somewhere. This PR cannot be merged until that is resolved one way (like fixing it in the core) or another (like turning this into a TODO or something). I'm hoping to get some smoke tests from this to provide more information.

There's a sleep() included because some systems might delay the signal until the -e'' execution would already finish.

This tests that when we kill a process through a shell, we get the
correct exit code and signal.

Unfortunately, this fails on some versions of Ubuntu and dash, which
I believe shows a bug somewhere.

There's a sleep() included because some systems might delay the
signal until the -e'' execution would already finish.
@xsawyerx
Copy link
Member Author

Smoke test results also spot the issue:

# Failed test 2 - $? (35072) from a KILL signal is 9 at op/kill9_shell_exit.t line 18
t/op/kill9_shell_exit ............................................ FAILED at test 2
#      got "35072"
# expected "9"
# Failed test 3 - OS exit code (shifted) is 0 at op/kill9_shell_exit.t line 19
#      got "137"
# expected "0"
# Failed test 4 - KILL signal (bitwise ANDed exit) is 9 at op/kill9_shell_exit.t line 20
#      got "0"
# expected "9"

@jkeenan jkeenan added the do not merge Don't merge this PR, at least for now label Sep 23, 2021
@jkeenan
Copy link
Contributor

jkeenan commented Jun 27, 2022

There's been no activity in this p.r. since last September.

@xsawyerx, do you intend to pursue this p.r. further?

@xsawyerx
Copy link
Member Author

I'm not sure what's left to pursue. This ticket was left unattended and the discussion on the issue itself seems to have petered off.

  • There is a bug in Debian's dash (which seems like a purposeful change with unintended consequences)
  • It was also inherited by Ubuntu
  • This bug affects Perl users in some narrow situations
  • That narrow situation includes the perl smokers
  • I suggested a few options - the most (and maybe only) practical of which is to document this (should probably be in perlport and under system or backticks in perlfunc) and move on
  • No additional comments were made on the discussion or on this test PR

This PR includes the test to prove the problem. I would suggest this be added as TODO so it wouldn't break the testing suite but developers would still be notified when this issue is no longer relevant (namely, when the affected dash version is not in circulation as much).

I have no intention of pursuing either this PR or the original ticket I opened on it. If no one wishes to act on it (or resolve the discussion on it), you can close it as "ENOFIX".

@demerphq
Copy link
Collaborator

demerphq commented Feb 8, 2023

Anyone want to pick up turning this into a set of TODO tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Don't merge this PR, at least for now hasConflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants