Skip to content

Close-GH 15685: improve proc_open error reporting on Windows #15687

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 3 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 1, 2024

While similar errors are already reported via strerror() on other platforms, this has apparently overlooked for Windows, where only the error code has been reported so far.

While similar errors are already reported via `strerror()` on other
platforms, this has apparently overlooked for Windows, where only the
error code has been reported so far.
@cmb69
Copy link
Member Author

cmb69 commented Sep 1, 2024

These error messages are localized on Windows, so there is the general question what to do about that in tests:

  1. use a placeholder (%s) for the error message
  2. assume en_US, and let the tests fail for other locales (e.g. ext/odbc/tests/bug73448.phpt)
  3. skip the test if the locale is not en_US (e.g. ext/com_dotnet/tests/variants_x64.phpt)

I prefer (1), but see valid arguments for (3); (2) is the best for controlled environments (like CI), but I consider it generally doubtful.

@cmb69 cmb69 linked an issue Sep 1, 2024 that may be closed by this pull request
We go with option (2), fow now.
@cmb69 cmb69 marked this pull request as ready for review September 1, 2024 11:34
@cmb69 cmb69 requested a review from bukka as a code owner September 1, 2024 11:34
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

This looks much better indeed.

@bukka
Copy link
Member

bukka commented Sep 2, 2024

In terms of tests, I would probably go for 3.

Unfortunately, there is no PHP userland function which allows us to
get the current system locale, so we work around.
@cmb69 cmb69 closed this in 3892529 Sep 3, 2024
@cmb69
Copy link
Member Author

cmb69 commented Sep 3, 2024

In terms of tests, I would probably go for 3.

Did this.

@cmb69 cmb69 deleted the cmb/gh15685 branch September 3, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve proc_open error reporting on Windows
2 participants