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

consolidate installed libraries and source tree cleanup #1095

Merged
merged 4 commits into from Jun 28, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jun 27, 2017

As discussed in #1084, this PR moves libkvs, libkz, and libjsc from src/modules to src/common and folds libkvs and libjsc into libflux-core.so. libkz is not publicly exported so it's folded into libflux-common.la. There are no actual API or code changes proposed here, just moving things around internally and conslidating external libraries.

Having separate installed shared libraries for those interfaces that are all listed in flux-core.pc was a bit silly. We got there by trying to co-locate comms modules and their APIs together in the source tree, but I think the consensus in hindsight is that this is not worth it.

The reason for kicking this off now was to make it easier to enumerate the library interfaces that are licensable under LGPL; however, I'm not sure that's actually needed. We could just say "in addition Flux libraries may be licensed..." and not actually name them. So just consider this cleanup.

One loose end is libflux-optparse.so. I left that one separate because it's fairly standalone. It seems like either it should be folded into libflux-core.so like the others, or have it's own pc file. Thoughts?

garlick added some commits Jun 27, 2017

libkvs: move from kvs module directory
Relocate the KVS API from src/modules/kvs to src/common/libkvs.
This is built as a static library libkvs.la which is included
in the installed dynamic library libflux-core.so.  Temporarily
unmask symbols with kvs_*, kvsdir_*, and kvsitr_* prefixes
in that library.

Include proto.[ch] and json_dirent.[ch] in libkvs.la.
Since these are used by the KVS module also, but are not
exported via the public libflux-core.so, the KVS module
links statically with libkvs.la to obtain access.

Various Makefile.am and include paths are updated.
libkz: move from module directory
Relocate libkz from src/modules to src/common.

This is built as part of libflux-internal.la and is not
part of the public API.  It was located in the modules
directory because of its dependency on the KVS API, which
is now built with libflux-core.

Various Makefile.am and include paths are updated.
libjsc: move from module directory
Relocate libjsc from src/modules to src/common.
This is built as a static library libmsc.la which is included
in the installed dynamic library libflux-core.so.  Temporarily
unmask symbols with jsc_* prefix in that library.

Various Makefile.am and include paths are updated.
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 27, 2017

One loose end is libflux-optparse.so.

It probably should not go into libflux -- is it installed publicly just because other projects might want to use it? (i.e. any reason it can't also be included as convenience library only) If it needs to be installed I agree it should have its own .pc file.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 27, 2017

Everything "just worked" for me testing on hype. Nice job.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage increased (+0.04%) to 78.275% when pulling 013f305 on garlick:libflux_lgpl into c5f839d on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 27, 2017

Codecov Report

Merging #1095 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1095      +/-   ##
==========================================
+ Coverage   77.97%   78.01%   +0.04%     
==========================================
  Files         151      151              
  Lines       26115    26115              
==========================================
+ Hits        20362    20374      +12     
+ Misses       5753     5741      -12
Impacted Files Coverage Δ
src/modules/resource-hwloc/resource.c 71.33% <ø> (ø) ⬆️
src/common/libkvs/kvs.c 75.49% <ø> (ø)
src/common/libkvs/proto.c 75% <ø> (ø)
src/bindings/lua/kvs-lua.c 77.3% <ø> (ø) ⬆️
src/common/libjsc/jstatctl.c 78.56% <ø> (ø)
src/common/libkz/kz.c 71.42% <ø> (ø)
src/common/libkvs/json_dirent.c 97.72% <ø> (ø)
src/modules/kvs/kvs.c 79.79% <ø> (ø) ⬆️
src/modules/kvs/lookup.c 92.33% <ø> (ø) ⬆️
src/bindings/lua/flux-lua.c 81.58% <ø> (-0.1%) ⬇️
... and 17 more
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jun 28, 2017

Thanks. optparse was exported so that projects outside of core could provide utilities that work in a uniform way. I'll push another commit that adds a pc file.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jun 28, 2017

Note that sched doesn't appear to use optparse, so I think this PR won't dismember any cute furry animals like before.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 28, 2017

Thanks!

One build failed with a hang in those cron faketime tests. I just restarted.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 28, 2017

Coverage Status

Coverage increased (+0.04%) to 78.275% when pulling 50585e3 on garlick:libflux_lgpl into c5f839d on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 28, 2017

Looks good, thanks.

@grondo grondo merged commit 48408d5 into flux-framework:master Jun 28, 2017

4 checks passed

codecov/patch Coverage not affected when comparing c5f839d...50585e3
Details
codecov/project 78.01% (+0.04%) compared to c5f839d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 78.275%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.