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

[cleanup] partial migration from json-c to jansson #1501

Merged
merged 15 commits into from May 3, 2018

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented May 3, 2018

This PR converts a number of residual users of json-c over to jansson. This is mostly the small stuff.

I skipped wreck/job.c because I think @grondo has already fixed that one offline.

The remaining areas in flux-core that still use json-c are:

  • ./src/modules/wreck/wrexecd.c
  • ./src/modules/aggregator/aggregator.c
  • ./src/common/libjsc/jstatctl.c
  • ./src/bindings/lua/flux-lua.c
  • ./src/bindings/lua/json-lua.c

@garlick garlick force-pushed the garlick:nix_jsonc branch from ba430a7 to 852d53b May 3, 2018

garlick added some commits May 1, 2018

test/rpc: stop using json-c
Problem: t/rpc/rpc.c still uses json-c.

Convert to jansson.
test/mrpc: stop using json-c
Problem: t/rpc/mrpc.c still uses json-c.

Use flux_respond_pack() in that one instance.
test/request: [cleanup] fix trailing whitespace
Problem: t/request/req.c conntains lines with trailing
white space which makes vim/git sad.

Delete trailing white space.
test/request: stop using json-c (server)
Problem: t/request/req.c still uses json-c.

Convert to jansson and the "pack" RPC functions.
test/request: stop using json-c (client)
Problem: t/request/treq.c is still using jsonc-c.

Convert to jansson.
cmd/flux: drop shortjson.h
Problem: cmd/flux includes shortjson.h but doesn't use it.

Drop the extraneous include.
libutil/tstat: drop shortjson.h
Problem: libutil/tstat includes shortjson.h but doesn't
use it.

Drop the extraneous include.
libflux/test: drop shortjson.h from module test
Problem: module test includes shortjson.h but doesn't use it.

Drop extraneous include
man/test: stop using json-c in treduce.c
Problem: doc/man3/treduce.c is still using json-c.

Convert to rpc pack functions.
man/test: stop using json-c in tmrpc_then.c
Problem: doc/man3/tmrpc_then.c is still using json-c.
cmd/cmdhelp: stop using json-c
Problem: cmdhelp is still using json-c.

Convert to jansson.
cmd/flux-comms: drop shortjson.h
Problem: flux-comms was including shortjson.h but not using it.

Drop extraneous include.
cmd/flux-jstat: stop using json-c
Problem: flux-jstat is still using json-c.

Convert to jansson.
libjsc: use rpc_pack for kvspath RPC
Simplify code in lwj_kvs_path() by using flux_rpc_pack()
and flux_rpc_get_unpack() instead of "hand parsing"
the JSON payloads.
libsubprocess: stop using json-c
Problem: libsubprocess library still uses json-c.

Convert to jansson.

Drop extraneous json-c includes from subprocess/zio
unit tests as well.
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 3, 2018

Coverage Status

Coverage decreased (-0.02%) to 79.061% when pulling 852d53b on garlick:nix_jsonc into 12a1880 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 3, 2018

Codecov Report

Merging #1501 into master will decrease coverage by 0.04%.
The diff coverage is 76.25%.

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   78.78%   78.74%   -0.05%     
==========================================
  Files         164      164              
  Lines       30706    30680      -26     
==========================================
- Hits        24192    24158      -34     
- Misses       6514     6522       +8
Impacted Files Coverage Δ
src/cmd/flux-comms.c 55% <ø> (ø) ⬆️
src/common/libutil/tstat.c 100% <ø> (ø) ⬆️
src/cmd/flux.c 84.15% <ø> (ø) ⬆️
src/common/libflux/rpc.c 92.37% <100%> (-1.02%) ⬇️
src/common/libjsc/jstatctl.c 72.35% <100%> (-0.01%) ⬇️
src/cmd/cmdhelp.c 85.26% <64.28%> (+0.91%) ⬆️
src/common/libsubprocess/subprocess.c 81.12% <64.28%> (-0.74%) ⬇️
src/cmd/flux-jstat.c 81.57% <75%> (-4.27%) ⬇️
src/common/libflux/future.c 87.5% <83.33%> (-1.29%) ⬇️
src/broker/modservice.c 79.61% <0%> (-1.95%) ⬇️
... and 9 more
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented on src/cmd/flux-jstat.c in 5e7726e May 3, 2018

Just a quick note, wouldn't it be a bit safer if we check null pointer before free and jason decref?

if (jcbstr_pretty)
    free (jcbstr_pretty);

for example?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented May 3, 2018

Just looked at jsc and jstat. Thank you for the long desired jansson conversion! I just have one inlined comment above.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

json_decref() handles a NULL pointer, and POSIX free() does too, so I think we're safe, or so I have been supposing!

http://pubs.opengroup.org/onlinepubs/009695399/functions/free.html

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 3, 2018

@dongahn, I did not convert jsc, just one RPC that was an easy one. It's a lot of work and may be replaced anyway so I punted.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

Nice cleanup!

Sorry about the Lua bindings, now I feel really bad :-(

Should I push my commit for wreck/job.c here or keep it in my current devel branch?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 3, 2018

Actually this is nice cleanup. I think it should be merged now.

@grondo grondo merged commit 3ac89a8 into flux-framework:master May 3, 2018

2 of 4 checks passed

codecov/patch 70% of diff hit (target 78.78%)
Details
codecov/project 78.76% (-0.03%) compared to 12a1880
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 79.061%
Details

@garlick garlick deleted the garlick:nix_jsonc branch May 3, 2018

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.