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

wreck: convert wrexecd to jansson #1527

Merged
merged 5 commits into from May 17, 2018

Conversation

Projects
None yet
5 participants
@grondo
Copy link
Contributor

grondo commented May 17, 2018

The main intent of this PR is a quick conversion of wrexecd to jansson.

Along the way, the unused per-task procdesc information was removed. If we need similar functionality in the future we can implement something more scalably. The pdesc JSC tests were removed, but the code from jstatctl.c isn't touched since I know @garlick is working in there.

This was quick work, so could use a good review.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 17, 2018

Hm, getting failures due to exit_status aggregator failures. I'll have to see if this is related to this PR or the aggregator work.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented May 17, 2018

Yeah at some point, we will still need something like this to support legacy parallel debuggers etc.

@grondo grondo force-pushed the grondo:wrexecd-jansson branch from a72aa1a to b154bf2 May 17, 2018

grondo added some commits May 17, 2018

wreck: remove per-task procdesc kvs entry
per-task procdesc is heavyweight item that has no users. Remove
it for now.
wreck: remove questionable use of task.procdesc kvs key
The wreck Lua bindings make one use of the per-task "procdesc"
key to determine if a task is "starting" or "running". Remove
this questionable use case, as it is unclear that the end result
is even useful.
wreck: convert to jansson in wrexecd
Convert the wrexecd executable to jansson instead of json-c.

@grondo grondo force-pushed the grondo:wrexecd-jansson branch from b154bf2 to 0c9a97e May 17, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 17, 2018

Hm, getting failures due to exit_status aggregator failures.

Ugh, hit by json_pack() I vs i again. Fixed now, and rebased against current master.

@garlick
Copy link
Member

garlick left a comment

Just one minor comment - and not strictly related to this PR, just nearby :-)

wlog_err (ctx, "malformed cmdline");
free (*argvp);
return (-1);
}
(*argvp) [i] = strdup (json_object_get_string (ox));
(*argvp) [i] = strdup (json_string_value (ox));

This comment has been minimized.

@garlick

garlick May 17, 2018

Member

use xstrdup() or check result for null

This comment has been minimized.

@grondo

grondo May 17, 2018

Author Contributor

Oh yeah, sorry I missed that.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 17, 2018

Restarted a builder which stalled after t0017-security.t

wreck: wrexecd: fix unchecked return from strdup
Problem: A few calls to strdup() do not check for NULL return.
Check for failed strdup() at all call sites, and exit with
descriptive error message.
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 17, 2018

Went ahead and added a commit to fix up unchecked calls to strdup.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 17, 2018

Coverage Status

Coverage decreased (-0.5%) to 78.43% when pulling f73df10 on grondo:wrexecd-jansson into 3b62c49 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 17, 2018

Codecov Report

Merging #1527 into master will decrease coverage by 0.49%.
The diff coverage is 82.85%.

@@            Coverage Diff            @@
##           master    #1527     +/-   ##
=========================================
- Coverage   78.63%   78.13%   -0.5%     
=========================================
  Files         165      165             
  Lines       30781    30734     -47     
=========================================
- Hits        24204    24015    -189     
- Misses       6577     6719    +142
Impacted Files Coverage Δ
src/modules/wreck/wrexecd.c 74.38% <82.85%> (-0.21%) ⬇️
src/common/libutil/shortjson.h 51.72% <0%> (-39.46%) ⬇️
src/common/libjsc/jstatctl.c 55.82% <0%> (-17.84%) ⬇️
src/common/libflux/message.c 80.94% <0%> (-0.48%) ⬇️
src/broker/overlay.c 73.81% <0%> (-0.32%) ⬇️
src/bindings/lua/flux-lua.c 82.19% <0%> (+0.08%) ⬆️
src/modules/connector-local/local.c 74.59% <0%> (+0.2%) ⬆️
src/common/libkvs/kvs_watch.c 90.12% <0%> (+0.42%) ⬆️
src/common/libflux/future.c 89.86% <0%> (+0.44%) ⬆️
src/broker/modservice.c 80.58% <0%> (+0.97%) ⬆️
... and 1 more
@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 17, 2018

Thanks!

@garlick garlick merged commit c1a649a into flux-framework:master May 17, 2018

4 checks passed

codecov/patch 82.85% of diff hit (target 78.63%)
Details
codecov/project Absolute coverage decreased by -0.49% but relative coverage increased by +4.22% compared to 3b62c49
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.5%) to 78.43%
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 17, 2018

Thanks!

@grondo grondo deleted the grondo:wrexecd-jansson branch May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.