Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-82616: Add process_group support to subprocess.Popen #23930

Merged
merged 18 commits into from
May 5, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Dec 25, 2020

Adds a process_group= parameter to subprocess APIs to help us deprecate
uses of preexec_fn.

https://bugs.python.org/issue38435

Adds a setpgid parameter to subprocess APIs to help us deprecate
uses of preexec_fn.
@gpshead gpshead added the type-feature A feature request or enhancement label Dec 25, 2020
@gpshead gpshead changed the title Add setpgid support to subprocess.POpen bpo-38435: Add setpgid support to subprocess.POpen Dec 25, 2020
@gpshead gpshead self-assigned this Dec 25, 2020
@gpshead gpshead marked this pull request as draft December 25, 2020 04:42
@gpshead gpshead marked this pull request as ready for review December 25, 2020 04:51
@gpshead gpshead requested a review from vstinner December 25, 2020 05:23
Copy link
Contributor

@ZackerySpytz ZackerySpytz left a comment

Choose a reason for hiding this comment

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

It is subprocess.Popen, not subprocess.POpen. Please fix the title and the news entry.

@serhiy-storchaka serhiy-storchaka changed the title bpo-38435: Add setpgid support to subprocess.POpen bpo-38435: Add setpgid support to subprocess.Popen Dec 29, 2020
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 29, 2021
@gpshead gpshead added stdlib Python modules in the Lib dir extension-modules C modules in the Modules dir and removed stale Stale PR or inactive for long period of time. labels May 5, 2022
@gpshead gpshead changed the title bpo-38435: Add setpgid support to subprocess.Popen gh-82616: Add setpgid support to subprocess.Popen May 5, 2022
@gpshead gpshead changed the title gh-82616: Add setpgid support to subprocess.Popen gh-82616: Add process_group support to subprocess.Popen May 5, 2022
@gpshead
Copy link
Member Author

gpshead commented May 5, 2022

The Address sanitizer failure blocking this makes no logical sense. I cannot reproduce it on my systems because an address sanitizer build doesn't even work at all.

Update: Zach Ware pointed out that I also needed to export ASAN_OPTIONS=detect_leaks=0:allocator_may_return_null=1:handle_segv=0 for that run. With that I can get a functioning ASAN build at least, but am still unable to reproduce the issue.
Update 2: You need an ubuntu 20.04 gcc 9.4 based system to reproduce it. anything more modern does not. regardless, the asan failure didn't make sense. I'm having those tests skipped under asan.

@gpshead gpshead requested review from 1st1 and asvetlov as code owners May 5, 2022 22:15
@gpshead gpshead merged commit f6dd14c into python:main May 5, 2022
@gpshead gpshead deleted the subprocess-issue38435 branch May 5, 2022 23:22
@vstinner
Copy link
Member

vstinner commented May 5, 2022

(post-merge review ;-)) LGTM.

It might help to add a build check (C11 static_assert()) that the pid_t type is unsigned. A macro like that can be used:

#define IS_TYPE_UNSIGNED(type) (((type)0 - 1) > 0)

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request May 7, 2022
Most recent one in python/cpython#23930. This makes me wish we
could have had type evaluation functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants