Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: meson build #120
base: master
Are you sure you want to change the base?
WIP: meson build #120
Changes from 1 commit
63e93f5
fc52493
72f696e
8f0731e
7056bbc
d823821
3f29010
070a65d
132cde1
fc9655e
0ffc20d
aca1aa2
ae73fa4
0f2859d
96e2d96
f51a11f
be2a23a
8a7b9b1
a83a638
859f821
fb6b458
24ad34e
efcb02e
d45e486
7105a71
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eli-schwartz: Elsewhere there is
gnu_symbol_visibility: 'hidden'
. Is it possible to use the result of that here somehow? I remember that on some platforms the attributes are accepted but produce compiler warnings as they aren't truly supported. I don't know if that is a case on any modern platform though (Cygwin was like that many years ago but I haven't tested in years).Gnulib's visibility.m4 checks that both attribute and compiler option are supported using
-Werror
to enable visibility support. Meson's documentation doesn't tell if compiler.has_function_attribute() accepts the attribute if using it produces warnings but compiling still succeeds.It's also a bit unfortunate if visibility support requires an option in both
library
and a separate check for the attribute support. Perhaps thegnu_symbol_visibility
option could insert preprocessor macros likeMESON_GNU_SYMBOL_VISIBILITY_HIDDEN __attribute__((__visibility("hidden")))
which the C/C++ code could use. I'm not sure what is the best way as perhaps this idea has downsides. And modern compilers have__has_attribute
anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meson internally handles -Werror for exactly that reason:
.https://github.com/mesonbuild/meson/blob/7d28ff29396f9d7043204de8ddc52226b9903811/mesonbuild/compilers/mixins/clike.py#L1309-L1324
Most projects I have seen, actually check for
__GNUC__
instead of using visibility.m4 even with autotools. This is kind of necessary anyway since you need preprocessor logic to figure out whether Microsoft dllexport/dllimport is viable, and visibility.m4 is ultimately not all that helpful overall.Certain versions of... SunPro, if I recall correctly, are the only commonly known compiler that handles visibility but isn't either
__GNUC__
orMSC_VER
, and they use a third syntax instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Documenting it could be nice to give more confidence. Or perhaps it's that coming from other build systems one is constantly suspecting that corner cases aren't handled well enough, sorry.
liblzma checks in this order in the C code:
#if defined(_WIN32) || defined(__CYGWIN__)
then use__declspec(dllexport)
.#if HAVE_VISIBILITY
to use GNU C attributes. Checking for__GNUC__
wouldn't work as it can be defined on platforms that don't support the visibility attributes; one has to know if the attribute is actually supported.Otherwise assume no visibility support.
My note in b0d1422 says that SunPro (or whatever its name is this year) supports the GNU C attribute since 2007. At least modern releases support the
-fvisibility
option too. I didn't research when the command line option was added (it would make sense to add it when the attribute was but I don't know if that happened). The original option is-xldscope
but I guess there's no point to add support for it when-fvisibility
works too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
results in a warning with GCC 14.1.1 on x86-64 GNU/Linux:
But
cc.has_function_attribute('dllexport')
is still true. So-Werror
isn't used or Meson shortcircuits the attribute check somehow. Thus I fear it might do the same with the visibility attribute as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I'm wrong. I just fell into the exact trap of
#undef
vs.#define
being too easy to mix in config.h. So the previous message is noise. Sorry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense, because pthread_dep doesn't specify
required: false
and therefore it cannot ever fail to be found (regardless of whether meson always finds it successfully).So the if block is nonsense and should just be unconditional.
have_threads is therefore true if get_option('threads') is.