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

miscellaneous cleanup #902

Merged
merged 10 commits into from Nov 14, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Nov 14, 2016

My patch series in #898 was getting long so decided to split out a number of minor miscellaneous cleanups into this PR. It also includes some misc cleanup I had sitting off in another branch. Some whitespace & typo fixes included, so code coverage may fail.

@garlick garlick added the review label Nov 14, 2016

chu11 added some commits Nov 3, 2016

src/bindings/lua/wreck/io.lua: Whitespace cleanup
Fix tabbing in several locations
Add vi tab settings into file
doc/flux_rpc(3): Update return values
Add return value information for flux_rpcf() and flux_rpc_getf().
doc/flux_rpc_then(3): Update with jansson info
Document that flux_rpc_then() and flux_rpc_check() also relate
to flux_rpc_getf().
libutil/tstat: remove tstat_json()
Remove tstat_json() so tstat utility library is not dependent on
libjson-c.

@chu11 chu11 force-pushed the chu11:misccleanup3 branch from 119b6c7 to e463251 Nov 14, 2016

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 14, 2016

This all looks good - thanks! Will merge when travis passes.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2016

Huh ... all the travis builds failed. Is there any way to get to the config.log in travis?

checking for JANSSON... yes
checking json_int_t is 64 bits... no
configure: error: json_int_t must be 64 bits for flux to be built
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 14, 2016

Not that I know of.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 14, 2016

Huh ... all the travis builds failed. Is there any way to get to the config.log in travis?

Temporarily edit your .travis.yml file in this branch to echo the config.log to stdout when configure fails (in fact, that might be useful in general and a commit we could keep)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 14, 2016

Instead of using AC_TRY_COMPILE to get sizeof json_int_t could you use AC_CHECK_SIZEOF? Perhaps then we wouldn't need a new X_AC_JANSSON macro.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2016

Instead of using AC_TRY_COMPILE to get sizeof json_int_t could you use AC_CHECK_SIZEOF? Perhaps then we wouldn't need a new X_AC_JANSSON macro.

That seems like a good idea. I did somewhat blindly copy from the previous check. I am still curious how this check failed though.

@chu11 chu11 force-pushed the chu11:misccleanup3 branch 2 times, most recently from b44652e to aba9ed1 Nov 14, 2016

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2016

Ends up the check can't find jansson.h, leading to error. I suppose it's not installed in the normal location, probably just need to do some CFLAGS hacks. With such hacks, maybe need to keep X_AC_JANSSON macro.

@chu11 chu11 force-pushed the chu11:misccleanup3 branch from b10db0c to 9ea40e5 Nov 14, 2016

chu11 added some commits Nov 12, 2016

build: Add check for json_int_t being 64 bits
Check was previously in libflux/message.c, but since jansson
usage now crosses many files, move check into configure.  Also
simply by using AX_COMPILE_CHECK_SIZEOF.

@chu11 chu11 force-pushed the chu11:misccleanup3 branch from 9ea40e5 to a04e8cc Nov 14, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 14, 2016

Current coverage is 72.37% (diff: 100%)

Merging #902 into master will increase coverage by 0.02%

@@             master       #902   diff @@
==========================================
  Files           157        157          
  Lines         27043      27043          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19566      19572     +6   
+ Misses         7477       7471     -6   
  Partials          0          0          
Diff Coverage File Path
•••••••••• 100% src/modules/kvs/kvs.c
•••••••••• 100% src/common/libutil/tstat.c
•••••••••• 100% src/common/libflux/message.c
•••••••••• 100% src/common/libflux/rpc.c

Powered by Codecov. Last update 87f14cd...a04e8cc

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 14, 2016

Coverage Status

Coverage increased (+0.02%) to 75.978% when pulling a04e8cc on chu11:misccleanup3 into 87f14cd on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 14, 2016

Just did a push and things seem to work now. Per @grondo suggestion used size check macro instead of AC_TRY_COMPILE. Also added output of config.log into the travis output if a failure occurs. Admittedly don't know much about travis, but I think I made the change in the right place :-)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 14, 2016

Yeah, use of after_failure to display the config.log seems correct to me, thanks!

This looks good now. Thanks and merging.

@grondo grondo merged commit 4b9e43b into flux-framework:master Nov 14, 2016

4 checks passed

codecov/patch 100% of diff hit (target 72.35%)
Details
codecov/project 72.37% (+0.02%) compared to 87f14cd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 75.978%
Details

@grondo grondo removed the review label Nov 14, 2016

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.