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

deps: switch from lzo to lz4 #1740

Merged
merged 5 commits into from Oct 19, 2018

Conversation

Projects
None yet
3 participants
@trws
Copy link
Member

trws commented Oct 16, 2018

This commit switches us from lzo as a vendored dependency to lz4 as an
external dependency. LZ4 tends to win on performance, has a similar
api and similar compression characteristics, but avoids the need to link
GPL code into libflux-core. In support of fixing #1084.

Note that while our discussion in #1084 deemed this unnecessary, minilzo was being linked into libflux-core.so. With this and #1667, I think the only GPL code deps in the repo are base64.c/h.

I thought that could be fixed by just updating to the current upstream, since libmunge is now LGPL, but apparently that's just libmunge and not munged where the base64 files come from. So that begs the question, should we talk with folks about getting those two files LGPLed or switch to using the base64 from libsodium (it has it, and we already depend on it)? Thoughts?

@trws trws force-pushed the trws:remove-vendored-gpl-deps branch from 5451d04 to 8961aa9 Oct 16, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 16, 2018

lz4 sounds like a win.

Regarding base64, we were already talking with @dun about the license, although actually, it wouldn't be a bad thing to switch over to the one in libsodium. The API looks more or less similar to the one we are using:

https://libsodium.gitbook.io/doc/helpers#base64-encoding-decoding

libsodium is only a transitive dependency of flux-core though, which means we would need the "dev" package where we didn't before.

Maybe open an issue on that one? It may be easier to just get the license fixed for now.

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Oct 17, 2018

Makes sense to me @garlick, I'll make a separate issue for that and we can work it out.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 17, 2018

Codecov Report

Merging #1740 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1740      +/-   ##
==========================================
- Coverage   79.34%   79.33%   -0.02%     
==========================================
  Files         184      184              
  Lines       34559    34552       -7     
==========================================
- Hits        27422    27411      -11     
- Misses       7137     7141       +4
Impacted Files Coverage Δ
src/modules/content-sqlite/content-sqlite.c 56.33% <100%> (-0.02%) ⬇️
src/modules/connector-local/local.c 73.16% <0%> (-1.36%) ⬇️
src/common/libflux/message.c 81.06% <0%> (ø) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 18, 2018

Thanks @trws!

Would you mind splitting the removal of libminilzo out of 8961aa9 ?

In general, I think it makes the history easier to deal with when commits that contain substantive changes, like changing the compression alg used in content-sqlite, are isolated from mundane changes, like removal of a library that is no longer used.

@trws trws force-pushed the trws:remove-vendored-gpl-deps branch from b526233 to 0822165 Oct 19, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 19, 2018

Perfect, thanks @trws!

@garlick garlick merged commit 1a5433b into flux-framework:master Oct 19, 2018

3 checks passed

codecov/patch 100% of diff hit (target 79.34%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +20.65% compared to 5680a0b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 23, 2018

Ooops, should have checked - liblz4-dev is not installed on our rhel7 based clusters. I'll open an issue to get that included in the next update.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 23, 2018

I opened https://lc.llnl.gov/jira/browse/TOSS-4343

(private to LLNL)

@trws

This comment has been minimized.

Copy link
Member Author

trws commented Oct 23, 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.