Fixup meson build #1064
Fixup meson build #1064
Conversation
|
@Cyan4973 Here is what I got regarding fixing Meson. The only thing that I didn't try to port were the tests. I compiled the executables, but the tests are so entrenched in the Makefile that I just lost motivation. I see we don't even run the tests on the Meson build, so I don't think it will be an issue. |
|
Wondering if I should do something like this: https://github.com/hse-project/hse/blob/master/subprojects/packagefiles/lz4/lib/meson.build#L5. That is my custom lz4 build. |
|
Wondering if we could forego an lz4 release and just create a new release in wrapdb with the new Meson build. That would solve my issues. |
|
I also left the original copyright headers in place although the code is vastly different. Do you want me to add myself to the contributor headers? |
sounds fair |
I don't think is necessary. I ran a test to see if an executable would link without lz4 APIs going through the PLT with a static lz4 library, and it seemed good to go. |
|
Anyways I feel really good about this PR and I am happy to mark it off my list of things to do. |
|
mesonbuild/wrapdb#179 (comment) hmmm... |
| '@0@_manual.html'.format(mp), | ||
| build_by_default: true, | ||
| input: lz4_source_root / 'lib/@0@.h'.format(mp), | ||
| output: '@0@_manual.html'.format(mp), |
There was a problem hiding this comment.
The use of @INPUT@ and the input kwarg is a definite improvement.
| lz4 = executable( | ||
| 'lz4', | ||
| sources, | ||
| objects: liblz4.extract_all_objects(recursive: true), |
There was a problem hiding this comment.
Why do you need to extract all objects here? Guessing because the build never used gnu symbol visibility before, and the executable is relying on symbols that aren't exported with LZ4LIB_API?
There was a problem hiding this comment.
Exactly. @Cyan4973 any thoughts on this?
There was a problem hiding this comment.
I don't remember the details, but I know that the CLI executable is designed to be linked to the static variant of the library, meaning it is not restricted to "public" symbols. One such example is the xxhash library, which is not exposed on the public side of liblz4, but is employed internally to calculate checksums. The same function is used either as a random number generator or as a checksum for different purposes within the CLI.
There was a problem hiding this comment.
The most elegant solution here, IMHO, is to do a bit of project restucturing until you end up with:
libcommon.a: a static library containing internal symbols shared between the library and the CLIliblz4.so/liblz4.a: a shared/static library containing public symbols of the library only. It links to libcommonlz4: a CLI executable that links to both libcommon and to liblz4
So, the CLI executable would be free to link to the shared variant of the library, and it is restricted to public symbols, but any time you decide you need a private symbol, you just move the private symbol to libcommon. This also has a neat side effect: you get to see which functions are broken out into this internal library, and which presumably represent weak points in the liblz4.so API that don't fully meet the needs of your own CLI. Who knows -- those might turn out to be things that merit polishing and stabilization.
The CLI executable then prefers to link to public symbols from liblz4, which depending on the vendor's choice may be static or shared, because some vendors have decided for some reason they like shared libraries ;)
But, for any non-public symbols, the CLI executable just falls back to statically linking them from the convenience library libcommon.
You get the best of both worlds. The downside is it requires more work.
There was a problem hiding this comment.
I want to copy essentially what upstream is currently doing. So for me I see two options other than the re-org suggested by @eli-schwartz. Link against liblz4_internal_dep which is an internal static library or continue to do what I am doing currently which is just extracting objects from the static library.
|
Alright hopefully that is the last push. |
|
@Cyan4973 anything else you need from me on this PR? |
|
It would be great if the |
|
Whoops. My apologies. Will look into it. |
The meson build had gotten a little out of hand. It needed to be cleaned up and have its errors fixed. This should enable lz4 to switch to Meson at any time should the need ever arise.
|
I just pushed a fix and have raised an internal issue with Meson, but that shouldn't affect lz4 in any way |
|
Cool the CI is now all good to go as far as this PR is concerned |
|
@vtorri isn't |
|
@Cyan4973 any more feedback for this PR? I got @eli-schwartz to approve since he is a maintainer of Meson and it's wrapdb, so he would be fairly authoritative on this matter. |
|
Thanks for the |
|
You are welcome. I'll definitely be giving this a go as soon as we get a new release on wrapdb. I may come back with more feedback, but I think this is a much better base than what we had before. |
|
So @eli-schwartz informed me that I can't see the results of my labor easily unless there is a 1.9.4 release of lz4. Would this be possible? Otherwise I can wait until the next release that fits within your schedule. |
|
I don't have any availability to work on a new release of |
|
That's ok. No big deal. Look forward to the next release |
|
Hey @Cyan4973. Wondering if there was anything on your roadmap for a release including this work any time soon. I know Q2 isn't over just yet though |
|
There is no release planned soon. |
The meson build had gotten a little out of hand. It needed to be cleaned
up and have its errors fixed. This should enable lz4 to switch to Meson
at any time should the need ever arise.