Skip to content

bpo-35537: Rewrite setsid test for os.posix_spawn#11721

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:posix_spawn_setsid_test
Jun 14, 2019
Merged

bpo-35537: Rewrite setsid test for os.posix_spawn#11721
vstinner merged 1 commit intopython:masterfrom
vstinner:posix_spawn_setsid_test

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Feb 1, 2019 •
edited by bedevere-bot
Loading

bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().

https://bugs.python.org/issue35537

@vstinner vstinner requested a review from gpshead as a code owner February 1, 2019 11:16
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Feb 1, 2019
@vstinner
Copy link
Member Author

vstinner commented Feb 1, 2019

@nanjekyejoannah, @izbyshev: Would you mind to review my PR?

Alexey: I also fixed test_subprocess.

izbyshev
izbyshev reviewed Feb 1, 2019
View reviewed changes
Copy link
Contributor

@izbyshev izbyshev left a comment

Choose a reason for hiding this comment

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

LGTM except several small issues.

@vstinner
bpo-35537: Rewrite setsid test for os.posix_spawn
6dbc3a1 
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
@vstinner
Copy link
Member Author

vstinner commented Jun 14, 2019

@izbyshev: Would you mind to review my update PR?

gpshead
gpshead approved these changes Jun 14, 2019
View reviewed changes
Lib/test/test_subprocess.py
if e.errno != errno.EPERM:
raise
else:
parent_pgid = os.getpgid(os.getpid())
Copy link
Member

@gpshead gpshead Jun 14, 2019

Choose a reason for hiding this comment

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

i'm wondering if there is a reason this code was calling os.getpgid() rather than os.getsid() but I honestly do not remember. We added this test 9 years ago with the _posixsubprocess.c internals rewrite, regardless of that calling setsid(). My PGID vs SID knowledge is vague and needs refreshing, they're probably the same in this situation?

Copy link
Member Author

@vstinner vstinner Jun 14, 2019

Choose a reason for hiding this comment

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

I cannot tell you why you wrote this code :-)

@vstinner vstinner merged commit 5884043 into python:master Jun 14, 2019
@vstinner vstinner deleted the posix_spawn_setsid_test branch June 14, 2019 17:31
@miss-islington
Copy link
Contributor

miss-islington commented Jun 14, 2019

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Jun 14, 2019

GH-14093 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Jun 14, 2019
@miss-islington @vstinner
bpo-35537: Rewrite setsid test for os.posix_spawn (GH-11721)
e696b15 
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
(cherry picked from commit 5884043)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@vstinner @lisroach
bpo-35537: Rewrite setsid test for os.posix_spawn (pythonGH-11721)
2f43619 
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@vstinner @DinoV
bpo-35537: Rewrite setsid test for os.posix_spawn (pythonGH-11721)
04c2b60 
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes

+1 more reviewer

@izbyshev izbyshev izbyshev left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @miss-islington @bedevere-bot @gpshead @izbyshev @the-knights-who-say-ni