[pacman-dev,2/5] meson.build: Fix detection of symbols

Message ID 20210417034524.204755-3-mark.weiman@markzz.com
State New
Headers show
Series Various fixes for FreeBSD (and perhaps others) | expand

Commit Message

Mark Weiman April 17, 2021, 3:45 a.m. UTC
This patch changes the behavior of meson to define configuration options
*only* when the symbol checked is present. Currently, it defines all of
them in config.h whether the symbol exists or not and the code that
looks for it doesn't check the macro's value, but whether it's defined.

Signed-off-by: Mark Weiman <mark.weiman@markzz.com>
---
 meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Allan McRae April 19, 2021, 7:10 a.m. UTC | #1
On 17/4/21 1:45 pm, Mark Weiman wrote:
> This patch changes the behavior of meson to define configuration options
> *only* when the symbol checked is present. Currently, it defines all of
> them in config.h whether the symbol exists or not and the code that
> looks for it doesn't check the macro's value, but whether it's defined.
> 

Remember back when we used autotools and all this just worked!  :D

Patch looks good to me.

Allan

> Signed-off-by: Mark Weiman <mark.weiman@markzz.com>
> ---
>  meson.build | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 483c4fbd..14b3381a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -146,7 +146,9 @@ foreach sym : [
>      'tcflush',
>    ]
>    have = cc.has_function(sym, args : '-D_GNU_SOURCE')
> -  conf.set10('HAVE_' + sym.to_upper(), have)
> +  if have
> +    conf.set10('HAVE_' + sym.to_upper(), have)
> +  endif
>  endforeach
>  
>  foreach member : [
>
Emil Velikov April 19, 2021, 11:34 a.m. UTC | #2
On Mon, 19 Apr 2021 at 08:10, Allan McRae <allan@archlinux.org> wrote:
>
> On 17/4/21 1:45 pm, Mark Weiman wrote:
> > This patch changes the behavior of meson to define configuration options
> > *only* when the symbol checked is present. Currently, it defines all of
> > them in config.h whether the symbol exists or not and the code that
> > looks for it doesn't check the macro's value, but whether it's defined.
> >
>
> Remember back when we used autotools and all this just worked!  :D
>
> Patch looks good to me.
>
Food for thought:

Usually the more robust approach is to always set the respective
defines to 0/1 and evaluate them directly (aka #if HAVE_).
In addition one could set -Werror=undef in the build to catch any issues.

This will produce clear traces with potential issues, while #if
defined will silently fallback to the "other" path.
If people are OK with the above, I will follow-up with some patches.

Note: above suggestion is not meant to dismiss the original patch.

-Emil
Allan McRae April 19, 2021, 2:05 p.m. UTC | #3
On 19/4/21 9:34 pm, Emil Velikov wrote:
> On Mon, 19 Apr 2021 at 08:10, Allan McRae <allan@archlinux.org> wrote:
>>
>> On 17/4/21 1:45 pm, Mark Weiman wrote:
>>> This patch changes the behavior of meson to define configuration options
>>> *only* when the symbol checked is present. Currently, it defines all of
>>> them in config.h whether the symbol exists or not and the code that
>>> looks for it doesn't check the macro's value, but whether it's defined.
>>>
>>
>> Remember back when we used autotools and all this just worked!  :D
>>
>> Patch looks good to me.
>>
> Food for thought:
> 
> Usually the more robust approach is to always set the respective
> defines to 0/1 and evaluate them directly (aka #if HAVE_).
> In addition one could set -Werror=undef in the build to catch any issues.
> 
> This will produce clear traces with potential issues, while #if
> defined will silently fallback to the "other" path.
> If people are OK with the above, I will follow-up with some patches.
> 
> Note: above suggestion is not meant to dismiss the original patch.
> 


I still live in the good old autotools days where there were lots of
#undef in config.h...

It appears some of that still happens.  This is my build/config.h:

#undef HAVE_STRUCT_STATFS_F_FLAGS
#define HAVE_STRUCT_STATVFS_F_FLAG

But it is not consistent.


I'm not sure whether the better solution is to add more #undef, or to go
to the #if 0/1 style.

Allan
Mark Weiman April 19, 2021, 2:28 p.m. UTC | #4
On Tue, 2021-04-20 at 00:05 +1000, Allan McRae wrote:
> On 19/4/21 9:34 pm, Emil Velikov wrote:
> > On Mon, 19 Apr 2021 at 08:10, Allan McRae <allan@archlinux.org>
> > wrote:
> > > 
> > > On 17/4/21 1:45 pm, Mark Weiman wrote:
> > > > This patch changes the behavior of meson to define
> > > > configuration options
> > > > *only* when the symbol checked is present. Currently, it
> > > > defines all of
> > > > them in config.h whether the symbol exists or not and the code
> > > > that
> > > > looks for it doesn't check the macro's value, but whether it's
> > > > defined.
> > > > 
> > > 
> > > Remember back when we used autotools and all this just worked! 
> > > :D
> > > 
> > > Patch looks good to me.
> > > 
> > Food for thought:
> > 
> > Usually the more robust approach is to always set the respective
> > defines to 0/1 and evaluate them directly (aka #if HAVE_).
> > In addition one could set -Werror=undef in the build to catch any
> > issues.
> > 
> > This will produce clear traces with potential issues, while #if
> > defined will silently fallback to the "other" path.
> > If people are OK with the above, I will follow-up with some
> > patches.
> > 
> > Note: above suggestion is not meant to dismiss the original patch.
> > 
> 
> 
> I still live in the good old autotools days where there were lots of
> #undef in config.h...
> 
> It appears some of that still happens.  This is my build/config.h:
> 
> #undef HAVE_STRUCT_STATFS_F_FLAGS
> #define HAVE_STRUCT_STATVFS_F_FLAG
> 
> But it is not consistent.
> 

This can be easily done by not using set10, but instead setting the
values to true/false with set. That's why HAVE_STRUCT_STATFS_F_FLAGS is
undef'd and those other ones that use set10 don't. I can modify this to
mimick the old autotools behavior.

> 
> I'm not sure whether the better solution is to add more #undef, or to
> go
> to the #if 0/1 style.
> 
> Allan

Mark
Emil Velikov April 19, 2021, 2:46 p.m. UTC | #5
On Mon, 19 Apr 2021 at 15:05, Allan McRae <allan@archlinux.org> wrote:
>
> On 19/4/21 9:34 pm, Emil Velikov wrote:
> > On Mon, 19 Apr 2021 at 08:10, Allan McRae <allan@archlinux.org> wrote:
> >>
> >> On 17/4/21 1:45 pm, Mark Weiman wrote:
> >>> This patch changes the behavior of meson to define configuration options
> >>> *only* when the symbol checked is present. Currently, it defines all of
> >>> them in config.h whether the symbol exists or not and the code that
> >>> looks for it doesn't check the macro's value, but whether it's defined.
> >>>
> >>
> >> Remember back when we used autotools and all this just worked!  :D
> >>
> >> Patch looks good to me.
> >>
> > Food for thought:
> >
> > Usually the more robust approach is to always set the respective
> > defines to 0/1 and evaluate them directly (aka #if HAVE_).
> > In addition one could set -Werror=undef in the build to catch any issues.
> >
> > This will produce clear traces with potential issues, while #if
> > defined will silently fallback to the "other" path.
> > If people are OK with the above, I will follow-up with some patches.
> >
> > Note: above suggestion is not meant to dismiss the original patch.
> >
>
>
> I still live in the good old autotools days where there were lots of
> #undef in config.h...
>
> It appears some of that still happens.  This is my build/config.h:
>
> #undef HAVE_STRUCT_STATFS_F_FLAGS
> #define HAVE_STRUCT_STATVFS_F_FLAG
>
> But it is not consistent.
>
Speaking of consistency - here is a quick grep from an old-ish checkout:

$ git grep HAVE_LIBGPGME
...
lib/libalpm/be_sync.c:#ifndef HAVE_LIBGPGME // not defined, ok
...
lib/libalpm/sync.c:#ifdef HAVE_LIBGPGME // is defined, ok
meson.build:conf.set('HAVE_LIBGPGME', gpgme.found()) // hmm we always
define it, right?

Looking at the above one would say - we need CI to catch it. But we do
- see the arch-no-gpg config, still has gpgme installed :-\
Since the headers are in the standard location, they end up used. Even
if the final binary is missing the DT_NEEDED on gpgme.
Mind you that link thing is likely meson just adding a linker flag
(don't recall which one) to drop unneeded libraries from the DT_NEEDED
list.

Disclaimer: above is an educated guess, didn't stare at the build logs (yet).

Hmm HAVE_LIBSSL, ENABLE_NLS seems similar.

>
> I'm not sure whether the better solution is to add more #undef, or to go
> to the #if 0/1 style.
>
IMHO the above example/braindump illustrates why I'm in favour of 0/1
style. In addition, we had far too many bugs in Mesa and X (Xorg or
otherwise) so we gradually switched to 0/1.

-Emil
Mark Weiman April 19, 2021, 3:50 p.m. UTC | #6
On Mon, 2021-04-19 at 15:46 +0100, Emil Velikov wrote:
> On Mon, 19 Apr 2021 at 15:05, Allan McRae <allan@archlinux.org>
> wrote:
> > 
> > On 19/4/21 9:34 pm, Emil Velikov wrote:
> > > On Mon, 19 Apr 2021 at 08:10, Allan McRae <allan@archlinux.org>
> > > wrote:
> > > > 
> > > > On 17/4/21 1:45 pm, Mark Weiman wrote:
> > > > > This patch changes the behavior of meson to define
> > > > > configuration options
> > > > > *only* when the symbol checked is present. Currently, it
> > > > > defines all of
> > > > > them in config.h whether the symbol exists or not and the
> > > > > code that
> > > > > looks for it doesn't check the macro's value, but whether
> > > > > it's defined.
> > > > > 
> > > > 
> > > > Remember back when we used autotools and all this just worked! 
> > > > :D
> > > > 
> > > > Patch looks good to me.
> > > > 
> > > Food for thought:
> > > 
> > > Usually the more robust approach is to always set the respective
> > > defines to 0/1 and evaluate them directly (aka #if HAVE_).
> > > In addition one could set -Werror=undef in the build to catch any
> > > issues.
> > > 
> > > This will produce clear traces with potential issues, while #if
> > > defined will silently fallback to the "other" path.
> > > If people are OK with the above, I will follow-up with some
> > > patches.
> > > 
> > > Note: above suggestion is not meant to dismiss the original
> > > patch.
> > > 
> > 
> > 
> > I still live in the good old autotools days where there were lots
> > of
> > #undef in config.h...
> > 
> > It appears some of that still happens.  This is my build/config.h:
> > 
> > #undef HAVE_STRUCT_STATFS_F_FLAGS
> > #define HAVE_STRUCT_STATVFS_F_FLAG
> > 
> > But it is not consistent.
> > 
> Speaking of consistency - here is a quick grep from an old-ish
> checkout:
> 
> $ git grep HAVE_LIBGPGME
> ...
> lib/libalpm/be_sync.c:#ifndef HAVE_LIBGPGME // not defined, ok
> ...
> lib/libalpm/sync.c:#ifdef HAVE_LIBGPGME // is defined, ok
> meson.build:conf.set('HAVE_LIBGPGME', gpgme.found()) // hmm we always
> define it, right?
> 

From my testing to write these patches, if gpgme.found() is false, an
`#undef HAVE_LIBGPGME` will be set in config.h. This is where my note
in one of my other emails came from. I would need to test this further,
but a random "oh no, I forgot to install gpgme" after building didn't
seem to cause any errors on FreeBSD.

> Looking at the above one would say - we need CI to catch it. But we
> do
> - see the arch-no-gpg config, still has gpgme installed :-\
> Since the headers are in the standard location, they end up used.
> Even
> if the final binary is missing the DT_NEEDED on gpgme.
> Mind you that link thing is likely meson just adding a linker flag
> (don't recall which one) to drop unneeded libraries from the
> DT_NEEDED
> list.
> 
> Disclaimer: above is an educated guess, didn't stare at the build
> logs (yet).
> 
> Hmm HAVE_LIBSSL, ENABLE_NLS seems similar.
> 
> > 
> > I'm not sure whether the better solution is to add more #undef, or
> > to go
> > to the #if 0/1 style.
> > 
> IMHO the above example/braindump illustrates why I'm in favour of 0/1
> style. In addition, we had far too many bugs in Mesa and X (Xorg or
> otherwise) so we gradually switched to 0/1.
> 
> -Emil

Mark
Allan McRae April 20, 2021, 1:24 a.m. UTC | #7
On 20/4/21 12:28 am, Mark Weiman wrote:
> On Tue, 2021-04-20 at 00:05 +1000, Allan McRae wrote:
>> On 19/4/21 9:34 pm, Emil Velikov wrote:
>>> On Mon, 19 Apr 2021 at 08:10, Allan McRae <allan@archlinux.org>
>>> wrote:
>>>>
>>>> On 17/4/21 1:45 pm, Mark Weiman wrote:
>>>>> This patch changes the behavior of meson to define
>>>>> configuration options
>>>>> *only* when the symbol checked is present. Currently, it
>>>>> defines all of
>>>>> them in config.h whether the symbol exists or not and the code
>>>>> that
>>>>> looks for it doesn't check the macro's value, but whether it's
>>>>> defined.
>>>>>
>>>>
>>>> Remember back when we used autotools and all this just worked! 
>>>> :D
>>>>
>>>> Patch looks good to me.
>>>>
>>> Food for thought:
>>>
>>> Usually the more robust approach is to always set the respective
>>> defines to 0/1 and evaluate them directly (aka #if HAVE_).
>>> In addition one could set -Werror=undef in the build to catch any
>>> issues.
>>>
>>> This will produce clear traces with potential issues, while #if
>>> defined will silently fallback to the "other" path.
>>> If people are OK with the above, I will follow-up with some
>>> patches.
>>>
>>> Note: above suggestion is not meant to dismiss the original patch.
>>>
>>
>>
>> I still live in the good old autotools days where there were lots of
>> #undef in config.h...
>>
>> It appears some of that still happens.  This is my build/config.h:
>>
>> #undef HAVE_STRUCT_STATFS_F_FLAGS
>> #define HAVE_STRUCT_STATVFS_F_FLAG
>>
>> But it is not consistent.
>>
> 
> This can be easily done by not using set10, but instead setting the
> values to true/false with set. That's why HAVE_STRUCT_STATFS_F_FLAGS is
> undef'd and those other ones that use set10 don't. I can modify this to
> mimick the old autotools behavior.
> 

I have pulled the patch as-is, because it improves the current state.
So any further changes can happen on top of that.  (currently sitting in
my patchqueue branch).

I have no strong opinion whether the changes should have #undef, or just
always defining as 0/1 and then using #if.   It is clear that one of
these needs to happen so we have consistency.

Allan

Patch

diff --git a/meson.build b/meson.build
index 483c4fbd..14b3381a 100644
--- a/meson.build
+++ b/meson.build
@@ -146,7 +146,9 @@  foreach sym : [
     'tcflush',
   ]
   have = cc.has_function(sym, args : '-D_GNU_SOURCE')
-  conf.set10('HAVE_' + sym.to_upper(), have)
+  if have
+    conf.set10('HAVE_' + sym.to_upper(), have)
+  endif
 endforeach
 
 foreach member : [