GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@nanjekyejoannah

@nanjekyejoannah nanjekyejoannah commented Jan 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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
All reactions
  • 👍 1 reaction
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 18, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jan 18, 2019
@nanjekyejoannah 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
vstinner requested changes Jan 18, 2019
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

All reactions
Comment thread Misc/NEWS.d/next/Library/2019-01-18-13-44-13.bpo-35537.R1lbTl.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Jan 18, 2019

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Jan 18, 2019
izbyshev
izbyshev reviewed Jan 18, 2019
View reviewed changes
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@nanjekyejoannah 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
pablogsal reviewed Jan 20, 2019
View reviewed changes
Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner requested changes Jan 27, 2019
View reviewed changes
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_posix.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/Library/2019-01-18-13-44-13.bpo-35537.R1lbTl.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner requested changes Jan 28, 2019
View reviewed changes
Comment thread Lib/test/test_posix.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_posix.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Outdated

@vstinner vstinner Jan 28, 2019

Copy link
Copy Markdown
Member

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.

All reactions
Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

izbyshev
izbyshev reviewed Jan 30, 2019
View reviewed changes
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_posix.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/posixmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@nanjekyejoannah
nanjekyejoannah force-pushed the setsid branch from a34eedc to 225ff47 Compare January 31, 2019 11:00
@nanjekyejoannah
Add setsid parameter to os.posix_spawn() and os.posix_spawnp()
6a93484
@nanjekyejoannah
nanjekyejoannah force-pushed the setsid branch from 225ff47 to 6a93484 Compare January 31, 2019 11:23
@nanjekyejoannah

nanjekyejoannah commented Feb 1, 2019

Copy link
Copy Markdown
Contributor Author

@vstinner I made some changes . PTAL.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit 80c5dfe into python:master Feb 1, 2019
@bedevere-bot bedevere-bot removed the awaiting changes label Feb 1, 2019
@izbyshev

izbyshev commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 1, 2019

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Feb 1, 2019

Copy link
Copy Markdown

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

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 80c5dfe.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (80c5dfe) or if it is a false positive.
  5. 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Feb 1, 2019

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Feb 1, 2019

Copy link
Copy Markdown

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

Hi! The buildbot AMD64 Ubuntu Shared 3.x has failed when building commit 80c5dfe.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (80c5dfe) or if it is a false positive.
  5. 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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@izbyshev

izbyshev commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Feb 1, 2019

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal left review comments
@vstinner vstinner vstinner requested changes
+1 more reviewer
@izbyshev izbyshev izbyshev left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

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.

6 participants

@nanjekyejoannah @bedevere-bot @izbyshev @vstinner @pablogsal @the-knights-who-say-ni
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.