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

use jansson >= 2.6 and document JSON payload functions #884

Merged
merged 5 commits into from Oct 27, 2016

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Oct 27, 2016

The jansson json_pack() and json_unpack() functions that were used to build flux_rpcf() and related functions have some ambiguities in accepted variable arguments depending on what version of jansson is used, and the capabilities of the underlying machine. This PR is an attempt to address that by:

  • Requiring flux to build against jansson >= 2.6, thereby reducing some API variability
  • Documenting the format specifiers that work in 2.6
  • Requring json_int_t to be 64 bits internally, and documenting the %I type as int64_t externally.

garlick added some commits Oct 27, 2016

build: ensure jansson's json_int_t is 64 bits
jansson pack/unpack define the %I format token to correspond
to json_int_t, and json_int_t is either defined as long long
or long.  If the underlying system supports them, this will
always(?) be a 64 bit integer.

Rather than define a flux-specific type for json integers,
we make the guarantee in our API that %I is a 64 bit integer,
and catch it at build time if this likely assumption turns out
to be false.
build: require jansson >= 2.6
Jansson version 2.6 is a good compromise between availability
on distros and API features.  (Except on travis-ci where version
2.2 seems to be the available packaged version).
doc: add json_pack/unpack sections where needed
Create shared asciidoc sections for json_pack and json_unpack
format specifiers.  Descriptions were borrowed pretty much verbatim
from the jansson 2.6 API manual (with citation).

Include these in the docs that introduced json_pack/unpack based
API functions.

Move Jansson API reference citations to the shared sections and
explicitly reference version 2.6.  Hopefully this will avoid confusion
about which jansson features can be used in Flux API functions built on
json_pack()/json_unpack().
travis-ci: download jansson-2.9
Now that flux-core requires jansson 2.6 or newer, the
jansson-2.2 package available in travis is too old.
Download the tarball and build a side-installed version.

@garlick garlick added the review label Oct 27, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage increased (+0.04%) to 75.863% when pulling 4114682 on garlick:jansson_clarify into f79d5cc on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 27, 2016

Nicely done this is great!

We should do the same in flux-sched, since it requires flux-core anyway, and we can remove some of the #ifdef'd code checking for JANSSON_VERSION_HEX of anything < 2.6.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 27, 2016

Current coverage is 72.19% (diff: 100%)

Merging #884 into master will decrease coverage by 0.01%

@@             master       #884   diff @@
==========================================
  Files           156        156          
  Lines         26947      26945     -2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19457      19452     -5   
- Misses         7490       7493     +3   
  Partials          0          0          
Diff Coverage File Path
•••••••••• 100% src/common/libflux/message.c
•••••••••• 100% src/common/libflux/module.c
•••••••••• 100% src/common/libflux/attr.c

Powered by Codecov. Last update f79d5cc...535d799

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 27, 2016

we can remove some of the #ifdef'd code checking for JANSSON_VERSION_HEX of anything < 2.6

Oh right I'll add that here.

libflux: drop conditional code for jansson < 2.6
We can go ahead and use json_array_foreach() unconditionally,
now that jansson 2.6 or greater is required.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.009%) to 75.811% when pulling 535d799 on garlick:jansson_clarify into f79d5cc on flux-framework:master.

@grondo grondo merged commit e98c4aa into flux-framework:master Oct 27, 2016

4 checks passed

codecov/patch Coverage not affected when comparing f79d5cc...535d799
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +27.79% compared to f79d5cc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 75.811%
Details

@grondo grondo removed the review label Oct 27, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 27, 2016

Already submitted pr - thanks!

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 27, 2016

Hm travis failed with a new one for me:

not ok 71 - stat watcher invoked once for size chnage
#   Failed test 'stat watcher invoked once for size chnage'
#   at ../../../../src/common/libflux/test/reactor.c line 644.

My guess is that it is transient error somehow so I'll restart the build.

@garlick garlick deleted the garlick:jansson_clarify branch May 24, 2017

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.