bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp(). by nanjekyejoannah · Pull Request #11608 · python/cpython · GitHub

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

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)

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.

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

@vstinner I made some changes . PTAL.

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

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

⚠️⚠️⚠️ 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

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

⚠️⚠️⚠️ 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)

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

@vstinner I opened https://bugs.python.org/issue35876 after seeing the buildbot failures to track the failing test fixes.

the-knights-who-say-ni added the CLA signed label Jan 18, 2019

bedevere-bot added the awaiting review label Jan 18, 2019

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

vstinner requested changes Jan 18, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 18, 2019

izbyshev reviewed Jan 18, 2019

View reviewed changes

Comment thread Doc/library/os.rst Outdated

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

pablogsal reviewed Jan 20, 2019

View reviewed changes

Comment thread Modules/posixmodule.c Outdated

Comment thread Modules/posixmodule.c Outdated

vstinner requested changes Jan 27, 2019

View reviewed changes

vstinner requested changes Jan 28, 2019

View reviewed changes

izbyshev reviewed Jan 30, 2019

View reviewed changes

Comment thread Doc/library/os.rst Outdated

Comment thread Doc/library/os.rst Outdated

Comment thread Lib/test/test_posix.py Outdated

Comment thread Modules/posixmodule.c Outdated

nanjekyejoannah force-pushed the setsid branch from a34eedc to 225ff47 Compare January 31, 2019 11:00

Add setsid parameter to os.posix_spawn() and os.posix_spawnp()

6a93484

nanjekyejoannah force-pushed the setsid branch from 225ff47 to 6a93484 Compare January 31, 2019 11:23

vstinner merged commit 80c5dfe into python:master Feb 1, 2019

bedevere-bot removed the awaiting changes label Feb 1, 2019

bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp(). by nanjekyejoannah · Pull Request #11608 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp(). - #11608

bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp().#11608vstinner merged 1 commit intopython:masterpython/cpython:masterfrom nanjekyejoannah:setsidnanjekyejoannah/cpython:setsidCopy head branch name to clipboard

Conversation

nanjekyejoannah commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jan 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nanjekyejoannah commented Feb 1, 2019

Uh oh!

izbyshev commented Feb 1, 2019

Uh oh!

vstinner commented Feb 1, 2019

Uh oh!

bedevere-bot commented Feb 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

vstinner commented Feb 1, 2019

Uh oh!

bedevere-bot commented Feb 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

izbyshev commented Feb 1, 2019

Uh oh!

pablogsal commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp().#11608
vstinner merged 1 commit into
python:masterfrom
nanjekyejoannah:setsid

nanjekyejoannah commented Jan 18, 2019 •
edited
Loading