bpo-40094: Add os.waitstatus_to_exitcode()#19201
bpo-40094: Add os.waitstatus_to_exitcode()#19201vstinner merged 3 commits intopython:masterfrom vstinner:wait_status_to_returncode
Conversation
|
asyncio.unix_events._compute_returncode() can be replaced with os._wait_status_to_returncode() , but I prefer to do that in a separated PR since Lib/test/test_asyncio/test_unix_events.py has extensive tests on calls to Well, I prefer to keep this PR short to ease its review. |
|
I updated my PR to rename the function os.status_to_exitcode() and make it public. TODO: Add unit tests by spawning a child process (maybe using fork?) and use waitpid() to get a status. |
|
I modified my PR to add Windows support. On Windows, os.waitpid() status also requires an operation (shif right by 8 bits) to get an exitcode from the waitpid status. So IMO it's worth it to add it to Windows as well, which makes the function even more useful ;-) |
There was a problem hiding this comment.
This is my opinion about the API decision:
I 've searched usecases of this function and it will be helpful to them.
- Name looks good! I like it.
- Publicize API?:
I am not +1 or -1 on this decision.
But if the API is provided by the public API, it would help to handling status code
through standard library suggest. :)
corona10
left a comment
There was a problem hiding this comment.
But the Windows CI should be checked.
Fixed. I also added an unit test for signals. I didn't know that os.spawnv() uses a shell. Apparently, Windows shell doesn't like spaces. I changed the test to use a filename instead. |
I'm not sure that I understood this point. Do you mean that the stdlib should use the new function? My PR modify 3 modules to use use: os, multiprocessing and subprocess. And also many tests. |
|
Hum, maybe run_cgi() http.server log should be modified in a separated PR to use the function: IMO logging an exit code rather than an exit status is more convenient. |
|
Tools/scripts/which.py may use this function as well to display an exit code rather than a raw waitpid exit status: But it changes the behavior, so I prefer to do it in a separated PR. mailcap.test() may also uses it for: |
|
I renamed the function to os.waitstatus_to_exitcode(). IMHO this name avoids any risk of confusion and it's short enough. I moved the function documentation at the right place: close to waitpid() documentation. I also rebased my PR and squashed commits to be able to modify the commit message. |
|
(Oops, "make regen-all" failed: it's now fixed.) |
|
@pablogsal, @methane, @gpshead, @pitrou: Hi. Do you think that it would be an useful addition to the stdlib? IMO it is worth it: I was able to use the new function in 3 modules of the stdlib (os, multiprocessing, subprocess), but also _bootsubprocess, and even more tests. |
|
Oh, another possible usage of waitstatus_to_exitcode(): |
Add os.waitstatus_to_exitcode() function to convert a wait status to an exitcode. Suggest waitstatus_to_exitcode() usage in the documentation when appropriate. Use waitstatus_to_exitcode(): * multiprocessing, os, subprocess and _bootsubprocess modules. * test.support.wait_process() * Many tests * setup.py: run_command()
|
I pushed as much changes as possible in https://bugs.python.org/issue40094 (non-controversial changes) to make this PR as small as possible. So remaining changes should really be focused on the value of the addition of the os.waitstatus_to_exitcode() function. I rebased my PR on top of this changes and I modify popen().close() documentation and test to use waitstatus_to_exitcode(). |
Add os.waitstatus_to_exitcode() function to convert a wait status to an
exitcode.
https://bugs.python.org/issue40094