GeezerVison is showing github.com — bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp(). by nanjekyejoannah · Pull Request #11608 · python/cpython
Conversation
I have added a new setsid parameter to os.posix_spawnp() and os.posix_spawn().
I have not yet added a test for this.
@vstinner we can discuss this once you think this patch is good enough.
https://bugs.python.org/issue35537
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
jab reacted with thumbs up emoji
nanjekyejoannah
changed the title
Add setsid parameter to os.posix_spawn() and os.posix_spawn()
bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawn().
Jan 18, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a test. You can copy and adapt the test_subprocess test:
def test_start_new_session(self):
# For code coverage of calling setsid(). We don't care if we get an
# EPERM error from it depending on the test execution environment, that
# still indicates that it was called.
try:
output = subprocess.check_output(
[sys.executable, "-c",
"import os; print(os.getpgid(os.getpid()))"],
start_new_session=True)
except OSError as e:
if e.errno != errno.EPERM:
raise
else:
parent_pgid = os.getpgid(os.getpid())
child_pgid = int(output)
self.assertNotEqual(parent_pgid, child_pgid)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
nanjekyejoannah
changed the title
bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawn().
bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp().
Jan 18, 2019
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's wrong: parse_posix_spawn_flags() is called from py_posix_spawnp(), so function_name should be "posix_spawnp" in that case. You have to add a parameter to parse_posix_spawn_flags().
nitpick: I suggest to rename the variable to "func_name".
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@nanjekyejoannah @vstinner Um, some of my comments were marked as resolved without addressing them, in particular, about shadowing the standard function with setsid parameter and usage of os.getpgid() instead of os.getsid() in the test. I'd prefer to have a discussion if there is an objection. Thanks.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
@nanjekyejoannah @vstinner Um, some of my comments were marked as resolved without addressing them, in particular, about shadowing the standard function with setsid parameter and usage of os.getpgid() instead of os.getsid() in the test. I'd prefer to have a discussion if there is an objection. Thanks.
Oh, I didn't notice that. According to the GitHub UI, I was the only one who "requested changes" (voted -1 on the PR).
I proposed to @nanjekyejoannah to reuse test_start_new_session() from test_subprocess. The comment of this test mentions setsid(). The 'start_new_session' parameter of subprocess.Popen becomes the 'call_setsid' parameter of _posixsubprocess.fork_exec() which then calls setsid(). If os.getpgid() is not appropriate: test_subprocess should be fixed as well.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️
Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 80c5dfe.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/103/builds/2023) and take a look at the build logs.
- Check if the failure is related to this commit (80c5dfe) or if it is a false positive.
- If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.
You can take a look at the buildbot page here:
https://buildbot.python.org/all/#builders/103/builds/2023
Click to see traceback logs
From https://github.com/python/cpython
* branch master -> FETCH_HEAD
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
Reset branch 'master'
Python/ast.c: In function ‘handle_keywordonly_args.isra.21’:
Python/ast.c:1415:35: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
arg->type_comment = NEW_TYPE_COMMENT(ch);
^
Python/ast.c: In function ‘ast_for_arguments’:
Python/ast.c:1619:35: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
arg->type_comment = NEW_TYPE_COMMENT(ch);
^
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_classdef’:
./Include/Python-ast.h:495:58: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~~~~~~
Python/ast.c:4340:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:495:58: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~~~~~~
Python/ast.c:4340:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_stmt’:
./Include/Python-ast.h:545:49: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define Try(a0, a1, a2, a3, a4, a5, a6, a7, a8) _Py_Try(a0, a1, a2, a3, a4, a5, a6, a7, a8)
^~~~~~~
Python/ast.c:4189:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset, n_except = (nch - 3)/3;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:545:49: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define Try(a0, a1, a2, a3, a4, a5, a6, a7, a8) _Py_Try(a0, a1, a2, a3, a4, a5, a6, a7, a8)
^~~~~~~
Python/ast.c:4189:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset, n_except = (nch - 3)/3;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:681:55: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7) _Py_ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~~~~~~~~~
Python/ast.c:4128:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:681:55: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7) _Py_ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~~~~~~~~~
Python/ast.c:4128:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:526:47: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define While(a0, a1, a2, a3, a4, a5, a6, a7) _Py_While(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~
Python/ast.c:4016:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:526:47: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define While(a0, a1, a2, a3, a4, a5, a6, a7) _Py_While(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~
Python/ast.c:4016:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:530:44: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define If(a0, a1, a2, a3, a4, a5, a6, a7) _Py_If(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~
Python/ast.c:3886:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:530:44: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define If(a0, a1, a2, a3, a4, a5, a6, a7) _Py_If(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~
Python/ast.c:3886:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_funcdef_impl’:
./Include/Python-ast.h:484:66: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) _Py_FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10)
^~~~~~~~~~~~~~~
Python/ast.c:1738:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:484:66: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) _Py_FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10)
^~~~~~~~~~~~~~~
Python/ast.c:1738:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_for_stmt’:
./Include/Python-ast.h:518:53: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~
Python/ast.c:4065:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:518:53: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~
Python/ast.c:4065:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;
^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_with_stmt’:
./Include/Python-ast.h:534:46: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define With(a0, a1, a2, a3, a4, a5, a6, a7) _Py_With(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~
Python/ast.c:4292:67: note: ‘end_col_offset’ was declared here
int i, n_items, nch_minus_type, has_type_comment, end_lineno, end_col_offset;
^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:534:46: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define With(a0, a1, a2, a3, a4, a5, a6, a7) _Py_With(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~
Python/ast.c:4292:55: note: ‘end_lineno’ was declared here
int i, n_items, nch_minus_type, has_type_comment, end_lineno, end_col_offset;
^~~~~~~~~~
Python/pystate.c: In function ‘_long_shared’:
Python/pystate.c:1483:18: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
data->data = (void *)value;
^
Python/symtable.c: In function ‘PySymtable_BuildObject’:
Python/symtable.c:289:5: warning: enumeration value ‘FunctionType_kind’ not handled in switch [-Wswitch]
switch (mod->kind) {
^~~~~~
./Modules/posixmodule.c: In function ‘parse_posix_spawn_flags’:
./Modules/posixmodule.c:5173:17: warning: unused variable ‘func_name’ [-Wunused-variable]
const char *func_name = "posix_spawnp";
^~~~~~~~~
/buildbot/tmp/tmpdir/tmp4xs5o00b/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:3: SyntaxWarning: invalid escape sequence \o
/buildbot/tmp/tmpdir/tmp4xs5o00b/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:3: SyntaxWarning: invalid escape sequence \o
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks and congrats @nanjekyejoannah, I merged your PR!
I saw that almost all my requests have been addressed exception of 'func_name': you added func_name but its value is hardcoded to "posix_spawnp". I started to write a fix, but then I saw other minor issues in error messages. So I wrote PR #11719 directly instead. I'm fine with working step by step, not having the perfect commit directly.
Sadly, when I testing my "func_name" change, I noticed that "test_posix" fails locally. Oh. Why Travis CI didn't detect it? Oh. Now I recall that Travis CI lacks the required constant and so the test is skipped (NotImplementedError...).
I wrote PR #11718 just to skip the test, to have more time to fix the test.
This PR was awaiting for 2 weeks and got many reviews. I chose to merge it to make progress ;-)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️
Hi! The buildbot AMD64 Ubuntu Shared 3.x has failed when building commit 80c5dfe.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/141/builds/1142) and take a look at the build logs.
- Check if the failure is related to this commit (80c5dfe) or if it is a false positive.
- If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.
You can take a look at the buildbot page here:
https://buildbot.python.org/all/#builders/141/builds/1142
Click to see traceback logs
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid = int(output)
NameError: name 'output' is not defined
======================================================================
ERROR: test_start_new_session (test.test_posix.TestPosixSpawnP)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid = int(output)
NameError: name 'output' is not defined
----------------------------------------------------------------------
Ran 139 tests in 4.228s
FAILED (errors=2, skipped=12)
Warning -- reap_children() reaped child process 2567
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid = int(output)
NameError: name 'output' is not defined
======================================================================
ERROR: test_start_new_session (test.test_posix.TestPosixSpawnP)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid = int(output)
NameError: name 'output' is not defined
----------------------------------------------------------------------
Ran 139 tests in 1.949s
FAILED (errors=2, skipped=12)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Oh, I didn't notice that. According to the GitHub UI, I was the only one who "requested changes" (voted -1 on the PR).
Ah, I see. I prefer to leave simple comments so that they are not mixed with reviews by core devs, but my approach backfired in this case.
If os.getpgid() is not appropriate: test_subprocess should be fixed as well.
While I think that technically the existing os.getpgid check works (because if the session is changed, the process group must be changed as well), os.posix_spawn also has setpgroup parameter, so it's kind of confusing to use the same check in both cases. (Note that the existing test for setpgroup doesn't really check the process group at all).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|