Skip to content

Fix run-tests.php to propagate status code on Windows - #15378

Closed
cmb69 wants to merge 1 commit into
php:PHP-8.3from
cmb69:cmb/termsig
Closed

cmb69 wants to merge 1 commit into
php:PHP-8.3from
cmb69:cmb/termsig

Conversation

@cmb69

@cmb69 cmb69 commented Aug 13, 2024

Copy link
Copy Markdown
Member

There is now a workaround in system_with_timeout() to avoid issues with quotes and spaces in the filenames of the executable by using start[1]. However, calling start will not propagate the process status of the actual process. Thus, calling proc_get_status() is pretty meaningless, and especially Microsoft errors cannot be detected (typically, access violations etc.), and as such no "Termsig" message is output.

We fix this by executing exit after the started command has finished.

[1]

php-src/run-tests.php

Lines 1157 to 1162 in a6d7d52

// when proc_open cmd is passed as a string (without bypass_shell=true option) the cmd goes thru shell
// and on Windows quotes are discarded, this is a fix to honor the quotes and allow values containing
// spaces like '"C:\Program Files\PHP\php.exe"' to be passed as 1 argument correctly
if (IS_WINDOWS) {
$commandline = 'start "" /b /wait ' . $commandline;
}


Given the proc_open() escaping fixes that have been done in the meantime, it may also be possible to drop the start workaround on Windows altogether; a quick test worked as expected, but I haven't done extensive testing.

There is now a workaround in `system_with_timeout()` to avoid issues
with quotes and spaces in the filenames of the executable by using
`start`[1].  However, calling `start` will not propagate the process
status of the actual process.  Thus, calling `proc_get_status()`
is pretty meaningless, and especially Microsoft errors cannot be
detected (typically, access violations etc.), and as such no "Termsig"
message is output.

We fix this by executing `exit` after the started command has finished.

[1] <https://github.com/php/php-src/blob/a6d7d5234b05582d3a333c0f2646fdeae44b4728/run-tests.php#L1157-L1162>
@cmb69

cmb69 commented Sep 1, 2024

Copy link
Copy Markdown
Member Author

Sorry to bother you, @nielsdos, but you've written that code, and I think fixing the issue (namely that segfaults in tests may go unnoticed on Windows) is pretty important. I'm not sure if the & exit hack is preferable, or whether we should drop the whole start /b /wait wrapping.

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, wasn't aware tbh that this would interfere with the access violations on Windows.
proc_open seems to be the better option but we can delay that.

@cmb69 cmb69 closed this in 71b9087 Sep 1, 2024
@cmb69
cmb69 deleted the cmb/termsig branch September 1, 2024 22:54
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

Successfully merging this pull request may close these issues.

2 participants