bpo-31904: Add subprocess module support for VxWorks RTOS by pxinwr · Pull Request #12157 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits7 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is the successive PR after #11968. This PR enables subprocess module support for VxWorks RTOS. VxWorks doesn't support fork()/exec(). Instead, we use rtpSpawn() to spawn new processes. So a new extension module named _vxwapi is added to offer the method rtp_spawn, which will be used in subprocess module to create new processes.
More and full support on modules for VxWorks will continuously be added by the coming PRs.
VxWorks is a product developed and owned by Wind River. For VxWorks introduction or more details, go to https://www.windriver.com/products/vxworks/
Wind River will have a dedicated engineering team to contribute to the support as maintainers.
We already have a working buildbot worker internally, but has not bound to master. We will check the process for the buildbot, then add it.

https://bugs.python.org/issue31904

@@ -1589,6 +1611,8 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
errpipe_read, errpipe_write,
restore_signals, start_new_session, preexec_fn)
self._child_created = True

Choose a reason for hiding this comment

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

Best not to ever allow this to be set to True; if an asynchronous event happened and raised an exception (KeyboardInterrupt) here, the cleanup code would see this as True even though you meant it to be False.

The logic should probably look like:

if _vxworks: if self.pid != -1: self._child_created = True else: self._child_created = True

Choose a reason for hiding this comment

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

All this said... why is _vxwapi.rtp_spawn not raising an exception rather than returning -1? An exception is expected when a child process could not be created. Not a silent return with pid == -1.

Choose a reason for hiding this comment

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

more exception information raises in _vxwapi.rtp_spawn, since VxWorks has not used fork/execv, so only return -1

@mock.patch("subprocess._posixsubprocess.fork_exec")
def test_exception_errpipe_normal(self, fork_exec):
@mock.patch(mock_modname)
def test_exception_errpipe_normal(self, mock_func):

Choose a reason for hiding this comment

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

by abstracting the function name away from this code and going with the opaque mock_XXX names, it is no longer obvious what this does. what function is stubbed out, what it does, etc. It'd be good to keep this parameter named mock_fork_exec_or_spawn just to keep that clear to the reader without bouncing around the code to find definitions.

similar mock_modname would be better off called mock_fork_exec_or_spawn_fn_name for readability.

@@ -1709,7 +1732,7 @@ def raise_it():
preexec_fn=raise_it)
except subprocess.SubprocessError as e:
self.assertTrue(
subprocess._posixsubprocess,
mock_submod,

Choose a reason for hiding this comment

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

given the skipIf I think this could be left as subprocess._posixsubprocess. mock_submod would be better named platform_specific_extension_module at which point i'd object less to it being used here.

@@ -2738,7 +2770,7 @@ def __int__(self):
with self.assertRaises(
ValueError,
msg='fds_to_keep={}'.format(fds_to_keep)) as c:
_posixsubprocess.fork_exec(
mock_func(

Choose a reason for hiding this comment

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

similar naming comment. platform_specific_fork_exec_or_spawn would read better. But this does feel odd, the platform specific functions do not have to have the same function signature. how are all of these arguments even being passed into the vxworks platform specific function which appears to require fewer arguments?

there isn't a good reason to keep platform specific internal function signatures in sync.

return NULL;
}

if (preexec_fn != Py_None) {

Choose a reason for hiding this comment

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

is there a benefit to attempting to match the _posixsubprocess.fork_exec() function signature? It has a negative side effect on maintenance for everyone: When we want to add parameters to that, this code on a platform virtually nobody has access to would also need to be updated.

I'd like to keep the need to update the platform specific implementations isolated.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

Close it. No response for long time and also need some rebase actions.