libsubprocess: invert SETPGRP flag logic

[chu11]

Problem: Remote subprocesses default to always set the SETPGRP flag. This leads to inconsistent behavior where local subprocesses don't have the flag set but remote ones do.

We could remove this default and require remote subprocess execution to set this flag as needed. However, it ends up that the vast majority of code in flux-core sets the SETPGRP flag anyways, leading to an excess amount of code change to make local vs remote subprocesses consistent.

Solution: Invert the SETPGRP flag logic into NO_SETPGRP. Now, by default, have both local and remote subprocesses call setpgrp(2). The NO_SETPGRP flag will then disable the call to setpgrp(2). Update all callers to libsubprocess accordingly.

Note that we leave the new NO_SETPGRP flag to be for local subprocesses only. If there comes a time for it to be supported for remote subprocesses, that will be for another day.

[garlick]

We could remove this default and require remote subprocess execution to set this flag as needed. However, it ends up that the vast majority of code in flux-core sets the SETPGRP flag anyways, leading to an excess amount of code change to make local vs remote subprocesses consistent.

API wise, NO_SETPGRP seems vaguely wrong to me, but I see where you're coming from about updating all the tests n stuff. @grondo any thoughts?

[chu11]

API wise, NO_SETPGRP seems vaguely wrong to me, but I see where you're coming from about updating all the tests n stuff. @grondo any thoughts?

Updating everything to set the SETPGRP flag is certainly ok and fine too, we could do that.

It's mostly to get this consistent, b/c the follow up will be to allow subprocess flags to be passed to remote subprocesses.

[grondo]

Yeah, I was struggling with NO_SETPGRP too, but the simplicity is indeed appealing. I think this PR is probably fine as is.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.30%. Comparing base (7cf4359) to head (9fe507b). Report is 465 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6082      +/-   ##
==========================================
- Coverage   83.31%   83.30%   -0.02%     
==========================================
  Files         523      523              
  Lines       86194    86199       +5     
==========================================
- Hits        71811    71806       -5     
- Misses      14383    14393      +10     

Files with missing lines	Coverage Δ
src/broker/runat.c	79.13% <ø> (-0.07%)	:arrow_down:
src/cmd/builtin/proxy.c	78.24% <100.00%> (+0.30%)	:arrow_up:
src/cmd/flux-start.c	83.74% <100.00%> (+0.08%)	:arrow_up:
src/common/libsubprocess/fork.c	76.74% <100.00%> (+1.55%)	:arrow_up:
src/common/libsubprocess/posix_spawn.c	88.40% <100.00%> (ø)
src/common/libsubprocess/server.c	78.73% <100.00%> (ø)
src/common/libsubprocess/subprocess.c	88.90% <100.00%> (ø)
src/shell/task.c	82.03% <100.00%> (ø)

... and 7 files with indirect coverage changes