[Process] Enhance compatiblity with --enable-sigchild #16915
Conversation
There was a problem hiding this comment.
because there are too much tightened to implementation details, and in fact do not work when testing with a sighchild enabled php
nicolas-grekas
force-pushed
the
fixsigchild
branch
from
b9395dc to
deab96b
Compare
There was a problem hiding this comment.
we are now able to retrieve it when enhancing compat, but can we also without enhancing it ?
There was a problem hiding this comment.
knowing "if" it has been signaled works with or without enhanced mode thanks to https://github.com/symfony/symfony/pull/16915/files#diff-f9f2411040cda7b73402481facf3e4ddR1133
knowing which signal is more tricky and requires enhanced mode
There was a problem hiding this comment.
in fact, I was wrong, knowing "if" it has been signaled doesn't work without the enhanced mode, fixed
nicolas-grekas
force-pushed
the
fixsigchild
branch
2 times, most recently
from
14d13ee to
7b11f1f
Compare
|
Comments addressed, ready for a second round of review :) |
|
Sorry not able to review in depth right now but I sure hope it won't blow up anything. Then again I think sigchild usage is probably less and less of a problem the more time passes. |
nicolas-grekas
force-pushed
the
fixsigchild
branch
7 times, most recently
from
8cadae4 to
e36b1b4
Compare
|
I added a special build of php with sigchild enabled on travis so that we can test, verify, prove and enforce that this really works :) |
nicolas-grekas
force-pushed
the
fixsigchild
branch
from
e36b1b4 to
36fb047
Compare
There was a problem hiding this comment.
updateStatus is already called by requireProcessIsTerminated
nicolas-grekas
force-pushed
the
fixsigchild
branch
2 times, most recently
from
b2d9c18 to
bd38abd
Compare
There was a problem hiding this comment.
I assume that the version will have to change for the 3.0 branch ?
There was a problem hiding this comment.
yes, I propose 5.5.9 there
|
@nicolas-grekas have you tried running tests when forcing the sighchild compat layer to be disabled ? |
|
@stof yes. Nothing interesting here: tests either fail because of one of those exceptions about --enable-sigchild, or pass. |
|
@nicolas-grekas but it needs to be done to ensure we don't miss some of the sigchild checks |
nicolas-grekas
force-pushed
the
fixsigchild
branch
from
bd38abd to
24bd3a2
Compare
|
Sigchild is now tested with and without the enhanced mode.
|
nicolas-grekas
force-pushed
the
fixsigchild
branch
from
24bd3a2 to
db6239d
Compare
|
Looks good to me |
nicolas-grekas
force-pushed
the
fixsigchild
branch
from
070bf65 to
db6239d
Compare
nicolas-grekas
force-pushed
the
fixsigchild
branch
from
db6239d to
e7cc4aa
Compare
There was a problem hiding this comment.
aren't you missing a I in the name ?
There was a problem hiding this comment.
SIGCHLD is the name of the said signal, so it was intentional...
|
except for the comment I made, 👍 |
…olas-grekas) This PR was merged into the 2.3 branch. Discussion ---------- [Process] Enhance compatiblity with --enable-sigchild | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16888 | License | MIT | Doc PR | - This is complete rewrite of the fallback `--enable-sigchild` handling in the Process class. It removes most of the differences between this and a non-sigchild-enabled php. Which means the test suite doesn't need anymore to be replayed 3 times (which is how I started this PR, looking for a way to test this component in less time). I validated this with a locally compiled php, sigchild-enabled. Green. Changes affect only this special-mode php. Ping @romainneutron and @Seldaek (original writer of the sigchild support) Submitted on 2.3 as bugfix, which it is to me. Commits ------- e7cc4aa [Process] Enhance compatiblity with --enable-sigchild
There was a problem hiding this comment.
A comment explaining what's done in this commandline, with a link to the stackoverflow issue would be more than welcome.
I doubt anybody could read this without error in two years (thinking about the first implementation was years ago)
5 participants
This is complete rewrite of the fallback
--enable-sigchildhandling in the Process class.It removes most of the differences between this and a non-sigchild-enabled php.
Which means the test suite doesn't need anymore to be replayed 3 times (which is how I started this PR, looking for a way to test this component in less time).
I validated this with a locally compiled php, sigchild-enabled. Green.
Changes affect only this special-mode php.
Ping @romainneutron and @Seldaek (original writer of the sigchild support)
Submitted on 2.3 as bugfix, which it is to me.