Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Aug 13, 2024

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
Copy link
Member Author

cmb69 commented Sep 1, 2024

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.

Copy link
Member

@nielsdos nielsdos left a comment

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