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

libsubprocess: Remove STREAM_STOP option on remote subprocesses #2333

Merged
merged 3 commits into from Aug 26, 2019

Conversation

@chu11
Copy link
Contributor

commented Aug 23, 2019

I realized that the "STREAM_STOP" cmd option could propagate to a remote subprocess. Support some internal functions for removing any options in a cmd object that cannot work on remote subprocesses. In the future if there are options that only work on remote subprocesses, it'll be easy to remove them for local subprocesses.

chu11 added 2 commits Aug 23, 2019
Add comment that STREAM_STOP option only applies to local subprocesses.
Add internal function flux_cmd_delete_opts() so that opts that do
not apply to certain situations can be easily removed.
@chu11 chu11 force-pushed the chu11:libsubprocess_remove_opts branch from ad55307 to 702de4b Aug 23, 2019
@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

LGTM!

It might be clearer and more user friendly if there were a flux_cmd_unsetopt() that took a single opt to unset, instead of the function that takes an argv-style array. (e.g. like setenv, getenv, and unsetenv) Though you may have had something in mind that I missed, so I apologize if so.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@grondo, the flux_cmd_delete_opts() function is internal, not user facing, so hopefully only needed to be understood by libsubprocess :P

The main reason I wrote it this way is b/c we can't be sure what option names the user will set. Since option names can also apply to channels, the option names set by the user can be almost anything "PMI_BUFFER_SIZE", "FOOBAR_LINE_BUFFER", etc. Thus I coded it up searching for substrings.

On remote subprocesses, ignore the STREAM_STOP option and remove
it from the internal copy of the cmd passed in by users.
@chu11 chu11 force-pushed the chu11:libsubprocess_remove_opts branch from 702de4b to 72c0175 Aug 26, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

re-pushed, fixing my typo

@codecov-io

This comment has been minimized.

Copy link

commented Aug 26, 2019

Codecov Report

Merging #2333 into master will decrease coverage by <.01%.
The diff coverage is 88.88%.

@@            Coverage Diff            @@
##           master   #2333      +/-   ##
=========================================
- Coverage   80.81%   80.8%   -0.01%     
=========================================
  Files         215     215              
  Lines       34238   34256      +18     
=========================================
+ Hits        27668   27681      +13     
- Misses       6570    6575       +5
Impacted Files Coverage Δ
src/common/libsubprocess/subprocess.c 88.5% <100%> (+0.05%) ⬆️
src/common/libsubprocess/command.c 71.14% <86.66%> (+0.68%) ⬆️
src/modules/connector-local/local.c 73.26% <0%> (-1.16%) ⬇️
src/common/libflux/message.c 81% <0%> (+0.12%) ⬆️
src/modules/job-ingest/worker.c 74.12% <0%> (+1.39%) ⬆️
src/common/libutil/aux.c 94.44% <0%> (+3.7%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@grondo, the flux_cmd_delete_opts() function is internal, not user facing, so hopefully only needed to be understood by libsubprocess :P

Oh, duh, sorry! I wasn't reading clearly and thought it was public.

@grondo grondo merged commit 5378e4c into flux-framework:master Aug 26, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 88.88% of diff hit (target 80.81%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +8.07% compared to cb1bbde
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.