Skip to content

bpo-40094: Add os.waitstatus_to_exitcode() #19201

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

Merged
merged 3 commits into from
Apr 1, 2020
Merged

bpo-40094: Add os.waitstatus_to_exitcode() #19201

merged 3 commits into from
Apr 1, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 27, 2020

Add os.waitstatus_to_exitcode() function to convert a wait status to an
exitcode.

https://bugs.python.org/issue40094

@vstinner
Copy link
Member Author

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 os.W*() functions using waitpid_mocks() helper.

Well, I prefer to keep this PR short to ease its review.

@vstinner vstinner changed the title bpo-40094: Add os._wait_status_to_returncode() bpo-40094: Add os.status_to_exitcode() Mar 28, 2020
@vstinner
Copy link
Member Author

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.

@vstinner
Copy link
Member Author

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 ;-)

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

This is my opinion about the API decision:
I 've searched usecases of this function and it will be helpful to them.

  1. Name looks good! I like it.
  2. 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. :)

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

But the Windows CI should be checked.

@vstinner
Copy link
Member Author

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.

@vstinner
Copy link
Member Author

But if the API is provided by the public API, it would help to handling status code through standard library suggest. :)

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.

@vstinner
Copy link
Member Author

Hum, maybe run_cgi() http.server log should be modified in a separated PR to use the function:

pid, sts = os.waitpid(pid, 0)
...
if sts:
    self.log_error("CGI script exit status %#x", sts)

IMO logging an exit code rather than an exit status is more convenient.

@vstinner
Copy link
Member Author

Tools/scripts/which.py may use this function as well to display an exit code rather than a raw waitpid exit status:

sts = os.system('ls ' + longlist + ' ' + filename)
if sts: msg('"ls -l" exit status: ' + repr(sts))

But it changes the behavior, so I prefer to do it in a separated PR.

mailcap.test() may also uses it for:

print("Executing:", command)
sts = os.system(command)
if sts:
    print("Exit status:", sts)

@vstinner vstinner changed the title bpo-40094: Add os.status_to_exitcode() bpo-40094: Add os.waitstatus_to_exitcode() Mar 30, 2020
@vstinner
Copy link
Member Author

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.

@vstinner
Copy link
Member Author

(Oops, "make regen-all" failed: it's now fixed.)

@vstinner
Copy link
Member Author

@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.

@vstinner
Copy link
Member Author

Oh, another possible usage of waitstatus_to_exitcode(): os.popen().close() result. Example from test_popen.py:

    def test_return_code(self):
        self.assertEqual(os.popen("exit 0").close(), None)
        if os.name == 'nt':
            self.assertEqual(os.popen("exit 42").close(), 42)
        else:
            self.assertEqual(os.popen("exit 42").close(), 42 << 8)

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()
@vstinner
Copy link
Member Author

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().

@vstinner vstinner merged commit 65a796e into python:master Apr 1, 2020
@vstinner vstinner deleted the wait_status_to_returncode branch April 1, 2020 16:49
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.

4 participants