| Message ID | 20210417034524.204755-3-mark.weiman@markzz.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Various fixes for FreeBSD (and perhaps others) | expand |
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 : [ >
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
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
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
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
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
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
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 : [
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(-)