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

Allow additional Popen arguments through run #411

Merged
merged 7 commits into from
Dec 10, 2021
Merged

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Dec 9, 2020

Alternatively, we could pass **kwargs through to the subprocess creation function.

@cottsay cottsay self-assigned this Dec 9, 2020
This is unfortunate, but it appears that we can't pass `None` to get the
"default" behavior for the platform, so we'll need to resort to not
passing it at all.
@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #411 (a93eb41) into master (decc7ce) will decrease coverage by 0.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   81.23%   80.81%   -0.43%     
==========================================
  Files          58       59       +1     
  Lines        3443     3518      +75     
  Branches      645      668      +23     
==========================================
+ Hits         2797     2843      +46     
- Misses        601      630      +29     
  Partials       45       45              
Impacted Files Coverage Δ
colcon_core/subprocess.py 65.89% <100.00%> (+0.26%) ⬆️
colcon_core/verb/build.py 0.00% <0.00%> (ø)
colcon_core/shell/installed_packages.py 100.00% <0.00%> (ø)
colcon_core/shell/__init__.py 98.54% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update decc7ce...a93eb41. Read the comment docs.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Please describe the which led you to create this patch.

Alternatively, we could pass **kwargs through to the subprocess creation function.

That sounds like a more future-proof option. In order to do so it should be checked if all possible keyword arguments should be accepted.

colcon_core/subprocess.py Outdated Show resolved Hide resolved
colcon_core/subprocess.py Outdated Show resolved Hide resolved
@cottsay
Copy link
Member Author

cottsay commented Dec 10, 2020

That sounds like a more future-proof option. In order to do so it should be checked if all possible keyword arguments should be accepted.

Alright, I started that change. Can you elaborate on what you'd like to see regarding the checking of the keyword arguments?

For reference, here is how the behavior is documented on the asyncio functions that we're chaining to: https://docs.python.org/3/library/asyncio-subprocess.html#asyncio.create_subprocess_exec

@cottsay cottsay changed the title Expose close_fds option to subprocess.run Allow additional Popen arguments through run Dec 10, 2020
@cottsay cottsay added the enhancement New feature or request label Dec 1, 2021
@cottsay
Copy link
Member Author

cottsay commented Dec 9, 2021

Please describe the which led you to create this patch.

I've been experimenting with more robust job management regarding GNU Make, and a full jobserver implementation requires sharing open file handles with subordinate processes. I'm still not sure if it's going to pan out, but having the ability to control more of the options is useful even outside of that context.

@mikepurvis
Copy link
Contributor

I'm definitely interested in a full Make jobserver for colcon! It's one of the pain points we have relative to catkin_tools.

@cottsay
Copy link
Member Author

cottsay commented Dec 9, 2021

I'm definitely interested in a full Make jobserver for colcon!

It's encouraging to hear that. I had a working prototype a year ago, but it was a little rough.

@cottsay cottsay requested a review from dirk-thomas December 9, 2021 19:57
Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

The documentation and change are similar to how subprocess.run() describes its own arguments.

The arguments shown above are merely the most common ones, described below in Frequently Used Arguments (hence the use of keyword-only notation in the abbreviated signature). The full function signature is largely the same as that of the Popen constructor - most of the arguments to this function are passed through to that interface. (timeout, input, check, and capture_output are not.)

colcon_core/subprocess.py Outdated Show resolved Hide resolved
@cottsay cottsay merged commit b6c977a into master Dec 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the cottsay/close_fds branch December 10, 2021 22:30
@cottsay cottsay added this to the 0.7.0 milestone Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants