◐ Shell
reader mode source ↗
Skip to content

bpo-20104: Add flag capabilities to posix_spawn#6693

Merged
pablogsal merged 11 commits into
python:masterfrom
pablogsal:complete_posix_spawn
Sep 7, 2018
Merged

bpo-20104: Add flag capabilities to posix_spawn#6693
pablogsal merged 11 commits into
python:masterfrom
pablogsal:complete_posix_spawn

Conversation

@pablogsal

@pablogsal pablogsal commented May 2, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@pablogsal pablogsal force-pushed the complete_posix_spawn branch 3 times, most recently from b838fd6 to eec6215 Compare May 2, 2018 11:07
@pablogsal

pablogsal commented May 2, 2018

Copy link
Copy Markdown
Member Author

@vstinner @serhiy-storchaka Please, review and once we are happy with the interface and implementation we can add new tests for the flags in this PR.

@pablogsal pablogsal changed the title bpo-20104: Add flag capabilities to posix_spawn May 2, 2018
@pablogsal

pablogsal commented May 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

This implementation is missing setting the POSIX_SPAWN_SETSCHEDPARAM flag implementation because I am not sure what is the best way to actually implement the interface to set the scheduler parameters unsing posix_spawnattr_setschedparam. Any ideas?

@pablogsal pablogsal force-pushed the complete_posix_spawn branch 11 times, most recently from 6946cd3 to 55d1b3d Compare May 2, 2018 18:04
@gpshead gpshead requested a review from serhiy-storchaka May 2, 2018 21:49
@pablogsal

pablogsal commented May 5, 2018

Copy link
Copy Markdown
Member Author

@gpshead @serhiy-storchaka I have implemented the existing flags as keyword-only arguments as suggested in the issue. The question of what to do with POSIX_SPAWN_SETSCHEDPARAM remains.

@pablogsal pablogsal force-pushed the complete_posix_spawn branch 2 times, most recently from 395e7af to 457ea58 Compare May 5, 2018 19:22
@pablogsal pablogsal force-pushed the complete_posix_spawn branch 4 times, most recently from 31bb4bc to 98e005a Compare May 6, 2018 00:18
116 hidden items Load more…
@pablogsal pablogsal force-pushed the complete_posix_spawn branch from f0bd16d to dcf9ffc Compare May 14, 2018 13:17
@serhiy-storchaka

serhiy-storchaka commented Jun 15, 2018

Copy link
Copy Markdown
Member

Please regenerate the clinic file.

@pablogsal pablogsal force-pushed the complete_posix_spawn branch from dcf9ffc to e605b01 Compare June 15, 2018 16:33
@pablogsal

pablogsal commented Jun 15, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka I had to rebase onto the latest master to pick up the latest changes and solve the merge conflict of the clinic file. I have done that and regenerated it.

@serhiy-storchaka

serhiy-storchaka commented Jul 15, 2018

Copy link
Copy Markdown
Member

Please merge with the master again and regenerate the clinic file.

@pablogsal pablogsal force-pushed the complete_posix_spawn branch from e605b01 to 8fb0389 Compare July 20, 2018 09:57
@pablogsal

pablogsal commented Jul 24, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka Done!

@bedevere-bot

bedevere-bot commented Jul 24, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner

vstinner commented Jul 24, 2018

Copy link
Copy Markdown
Member

The overall change LGTM, but I have a few questions/suggestions.

@vstinner vstinner mentioned this pull request Aug 2, 2018
Merged
@pablogsal

pablogsal commented Aug 31, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Aug 31, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner, @serhiy-storchaka: please review the changes made to this pull request.

vstinner
vstinner approved these changes Sep 3, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM.

@vstinner

vstinner commented Sep 7, 2018

Copy link
Copy Markdown
Member

Yeah! It's now time to see how to use it in subprocess :-)

@serhiy-storchaka

serhiy-storchaka commented Sep 8, 2018

Copy link
Copy Markdown
Member

I don't see a use case for it in the stdlib.

@vstinner

vstinner commented Sep 9, 2018 via email

Copy link
Copy Markdown
Member

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

Reviewers

@vadmium vadmium

@pitrou pitrou

@serhiy-storchaka serhiy-storchaka

@vstinner vstinner vstinner approved these changes

@gpshead gpshead Awaiting requested review from gpshead

@benjaminp benjaminp Awaiting requested review from benjaminp

+1 more reviewer

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@pablogsal @bedevere-bot @serhiy-storchaka @vstinner @gpshead @vadmium @pitrou @ZackerySpytz @the-knights-who-say-ni