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

meson fixups #2746

Merged
merged 3 commits into from
Aug 23, 2021
Merged

meson fixups #2746

merged 3 commits into from
Aug 23, 2021

Conversation

eli-schwartz
Copy link
Contributor

Two small changes:

  • with regard to make / cmake inconsistencies #2261, fix linking libzstd.so with private symbol visibility, emulating the Makefile build with zstd-dll, which currently only "works" because the meson.build incorrectly failed to set private visibility
  • use more idiomatic, warning-free ways to configure meson to use -Wextra -pedantic -Werror

@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Aug 9, 2021

-Werror fails because make clangbuild fails but isn't tested in CI. I guess assuming that use of -Werror in the other build systems is worth emulating for parity, was a bad assumption...

EDIT: also this is generally impossible if you build the tests too.

…ple binaries

This saves repeatedly compiling them, resulting in 16 fewer compile
steps, in exchange for one `ar` step.
meson prefers that project-level options for Wall/Wextra/pedantic be
used, rather than hardcoding raw flags in add_project_arguments. If you
do the latter anyway, it raises a meson warning.

Set the default options for the project to use all this.

Also move the -Werror comment to the project default options with
appropriate format, but leave it commented out since it does not work.
@eli-schwartz
Copy link
Contributor Author

In the end I had to kind of rip out the current shared library linkage in tests/ because I don't think this was ever going to work. So all the test binaries now link to libtestcommon.a, a convenience library shared between test programs which also includes a copy of all .o files from libzstd. Since they are .o files, the fact that they're built with -fvisibility=hidden doesn't matter, and the private symbols successfully link.

@eli-schwartz
Copy link
Contributor Author

New in this force-push is commit ee3355d, which should actually slightly speed up building by sharing some object files which are built identically for multiple test binaries, and can now be shared.

This matches the Makefile build. Due to one private xxhash symbol in use
by the program, it recompiles a private copy of xxhash.

Due to the test binaries making extensive (?) use of private symbols, it
doesn't even attempt to link to shared libzstd, and instead, all of the
original object files are added to libtestcommon itself for private
linkage. This, too, matches the Makefile build.

Ref. facebook#2261
@eli-schwartz
Copy link
Contributor Author

Please let this be the last push... if pthread is a dependency of libzstd, it needs to be added to the convenience library too. But this worked flawlessly locally... because I'm running a development version of glibc which has moved libpthread into libc.so and is no longer required to be separately linked, lol.

@Cyan4973 Cyan4973 merged commit be82a0a into facebook:dev Aug 23, 2021
@eli-schwartz eli-schwartz deleted the meson-fixup branch August 23, 2021 23:00
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request Jan 31, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request Apr 19, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request Apr 19, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request Apr 28, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request May 27, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request Dec 1, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
eli-schwartz added a commit to mesonbuild/wrapdb that referenced this pull request Dec 14, 2022
These are the same changes I applied upstream at facebook/zstd#2746

But we need it here too for our forked meson.build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants