A few days ago I have posted a patch[1] that addresses an issue
introduced when a meson check was dropped but some uses of the
corresponding WITH_ macro were not removed at the same time.

That got me thinking about what we can do to prevent such scenarios
from happening again in the future. I have come up with something
that I think would be effective, but since applying the approach
throughout the entire codebase would require a non-trivial amount of
work, I figured I'd ask for feedback before embarking on it.

The idea is that there are two types of macros we can use for
conditional compilation: external ones, coming from the OS or other
libraries, and internal ones, which are the result of meson tests.

The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only
defined if they apply, so it is correct to check for their presence
with #ifdef. Using #if will also work, as undefined macros evaluate
to zero, but it's not good practice to use them that way. If -Wundef
has been passed to the compiler, those incorrect uses will be
reported (only on platforms where they are not defined, of course).

The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar,
but in this case we control their definition. This means that using
means that the feature is not available on the machine we're building
on, but it could also mean that we've removed the meson check and
forgot to update all users of the macro. In this case, -Wundef would
work 100% reliably to detect the issue: if the meson check doesn't
exist, neither will the macro, regardless of what platform we're
building on.

So the approach I'm suggesting is to use a syntax-check rule to
ensure that internal macros are only ever checked with #if instead of

Of course this requires a full sweep to fix all cases in which we're
not already doing things according to the proposal. Should be fairly
easy, if annoying. A couple of examples are included here for
demonstration purposes.

The bigger impact is going to be on the build system. Right now we
generally only define WITH_ macros if the check passed, but that will
have to change and the result is going to be quite a bit of
additional meson code I'm afraid.

Thoughts?

[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/SKJ2APPUTJEDDQZRMDNMUE2IX6QMJYCG/

Andrea Bolognani (4):
  configmake: Check for WIN32 correctly
  meson: Always define WITH_*_DECL macros
  syntax-check: Ensure WITH_ macros are used correctly
  meson: Enable -Wundef

 build-aux/syntax-check.mk  |  5 +++++
 configmake.h.in            |  2 +-
 meson.build                |  3 +++
 tests/virmockstathelpers.c | 28 ++++++++++++++--------------
 4 files changed, 23 insertions(+), 15 deletions(-)

-- 
2.43.2
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On a Thursday in 2024, Andrea Bolognani wrote:
>A few days ago I have posted a patch[1] that addresses an issue
>introduced when a meson check was dropped but some uses of the
>corresponding WITH_ macro were not removed at the same time.
>
>That got me thinking about what we can do to prevent such scenarios
>from happening again in the future. I have come up with something
>that I think would be effective, but since applying the approach
>throughout the entire codebase would require a non-trivial amount of
>work, I figured I'd ask for feedback before embarking on it.
>
>The idea is that there are two types of macros we can use for
>conditional compilation: external ones, coming from the OS or other
>libraries, and internal ones, which are the result of meson tests.
>
>The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only
>defined if they apply, so it is correct to check for their presence
>with #ifdef. Using #if will also work, as undefined macros evaluate
>to zero, but it's not good practice to use them that way. If -Wundef
>has been passed to the compiler, those incorrect uses will be
>reported (only on platforms where they are not defined, of course).
>
>The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar,
>but in this case we control their definition. This means that using
>means that the feature is not available on the machine we're building
>on, but it could also mean that we've removed the meson check and
>forgot to update all users of the macro. In this case, -Wundef would
>work 100% reliably to detect the issue: if the meson check doesn't
>exist, neither will the macro, regardless of what platform we're
>building on.
>
>So the approach I'm suggesting is to use a syntax-check rule to
>ensure that internal macros are only ever checked with #if instead of
>
>Of course this requires a full sweep to fix all cases in which we're
>not already doing things according to the proposal. Should be fairly
>easy, if annoying. A couple of examples are included here for
>demonstration purposes.
>
>The bigger impact is going to be on the build system. Right now we
>generally only define WITH_ macros if the check passed, but that will
>have to change and the result is going to be quite a bit of
>additional meson code I'm afraid.
>
>Thoughts?

I don't like all the extra additional meson code. Can it be somehow
simplified?

Calling:
   foreach function : functions
     conf.set('WITH_@0@'.format(function.to_upper()), cc.has_function(function))
   endforeach
does not seem to work.

Jano

>
>[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/SKJ2APPUTJEDDQZRMDNMUE2IX6QMJYCG/
>
>Andrea Bolognani (4):
>  configmake: Check for WIN32 correctly
>  meson: Always define WITH_*_DECL macros
>  syntax-check: Ensure WITH_ macros are used correctly
>  meson: Enable -Wundef
>
> build-aux/syntax-check.mk  |  5 +++++
> configmake.h.in            |  2 +-
> meson.build                |  3 +++
> tests/virmockstathelpers.c | 28 ++++++++++++++--------------
> 4 files changed, 23 insertions(+), 15 deletions(-)
>
>-- 
>2.43.2
>_______________________________________________
>Devel mailing list -- devel@lists.libvirt.org
>To unsubscribe send an email to devel-leave@lists.libvirt.org

On 2/26/25 18:02, Ján Tomko wrote:
> On a Thursday in 2024, Andrea Bolognani wrote:
>> A few days ago I have posted a patch[1] that addresses an issue
>> introduced when a meson check was dropped but some uses of the
>> corresponding WITH_ macro were not removed at the same time.
>>
>> That got me thinking about what we can do to prevent such scenarios
>> from happening again in the future. I have come up with something
>> that I think would be effective, but since applying the approach
>> throughout the entire codebase would require a non-trivial amount of
>> work, I figured I'd ask for feedback before embarking on it.
>>
>> The idea is that there are two types of macros we can use for
>> conditional compilation: external ones, coming from the OS or other
>> libraries, and internal ones, which are the result of meson tests.
>>
>> The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only
>> defined if they apply, so it is correct to check for their presence
>> with #ifdef. Using #if will also work, as undefined macros evaluate
>> to zero, but it's not good practice to use them that way. If -Wundef
>> has been passed to the compiler, those incorrect uses will be
>> reported (only on platforms where they are not defined, of course).
>>
>> The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar,
>> but in this case we control their definition. This means that using
>> means that the feature is not available on the machine we're building
>> on, but it could also mean that we've removed the meson check and
>> forgot to update all users of the macro. In this case, -Wundef would
>> work 100% reliably to detect the issue: if the meson check doesn't
>> exist, neither will the macro, regardless of what platform we're
>> building on.
>>
>> So the approach I'm suggesting is to use a syntax-check rule to
>> ensure that internal macros are only ever checked with #if instead of
>>
>> Of course this requires a full sweep to fix all cases in which we're
>> not already doing things according to the proposal. Should be fairly
>> easy, if annoying. A couple of examples are included here for
>> demonstration purposes.
>>
>> The bigger impact is going to be on the build system. Right now we
>> generally only define WITH_ macros if the check passed, but that will
>> have to change and the result is going to be quite a bit of
>> additional meson code I'm afraid.
>>
>> Thoughts?
> 
> I don't like all the extra additional meson code. Can it be somehow
> simplified?
> 
> Calling:
>   foreach function : functions
>     conf.set('WITH_@0@'.format(function.to_upper()),
> cc.has_function(function))
>   endforeach
> does not seem to work.

That's because conf.set(varname, value) behaves differently for
different types of $value:

if $value is boolean then "#define/#undef $varname" is generated,
if $value is int then "#define $varname $value" is generated,
and if $value is string then "#define $varname $value" is generated.

Long story short, we want .to_int():

foreach function : stat_functions
  conf.set('WITH_@0@_DECL'.format(function.to_upper()),
cc.has_header_symbol('sys/stat.h', function).to_int())
endforeach


But this patch series fixes only a portion of the problem (that where
compiler complained about -Wundef). Similar pattern exists for other
$functions, e,g:

tests/virmock.h:#ifndef WITH___OPEN_2_DECL


I suggest turning at least those two foreach loops above and below too.
This still leaves us with plenty of:

  if condition
    conf.set('WITH_SOMETHING', 1)
  endif

but well, who said this is trivial task?

Michal

On Thu, Feb 27, 2025 at 10:48:43AM +0100, Michal Prívozník wrote:
> On 2/26/25 18:02, Ján Tomko wrote:
> > On a Thursday in 2024, Andrea Bolognani wrote:
> >> A few days ago I have posted a patch[1] that addresses an issue
> >> introduced when a meson check was dropped but some uses of the
> >> corresponding WITH_ macro were not removed at the same time.
> >>
> >> That got me thinking about what we can do to prevent such scenarios
> >> from happening again in the future. I have come up with something
> >> that I think would be effective, but since applying the approach
> >> throughout the entire codebase would require a non-trivial amount of
> >> work, I figured I'd ask for feedback before embarking on it.
> >>
> >> The idea is that there are two types of macros we can use for
> >> conditional compilation: external ones, coming from the OS or other
> >> libraries, and internal ones, which are the result of meson tests.
> >>
> >> The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only
> >> defined if they apply, so it is correct to check for their presence
> >> with #ifdef. Using #if will also work, as undefined macros evaluate
> >> to zero, but it's not good practice to use them that way. If -Wundef
> >> has been passed to the compiler, those incorrect uses will be
> >> reported (only on platforms where they are not defined, of course).
> >>
> >> The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar,
> >> but in this case we control their definition. This means that using
> >> means that the feature is not available on the machine we're building
> >> on, but it could also mean that we've removed the meson check and
> >> forgot to update all users of the macro. In this case, -Wundef would
> >> work 100% reliably to detect the issue: if the meson check doesn't
> >> exist, neither will the macro, regardless of what platform we're
> >> building on.
> >>
> >> So the approach I'm suggesting is to use a syntax-check rule to
> >> ensure that internal macros are only ever checked with #if instead of
> >>
> >> Of course this requires a full sweep to fix all cases in which we're
> >> not already doing things according to the proposal. Should be fairly
> >> easy, if annoying. A couple of examples are included here for
> >> demonstration purposes.
> >>
> >> The bigger impact is going to be on the build system. Right now we
> >> generally only define WITH_ macros if the check passed, but that will
> >> have to change and the result is going to be quite a bit of
> >> additional meson code I'm afraid.
> >>
> >> Thoughts?
> > 
> > I don't like all the extra additional meson code. Can it be somehow
> > simplified?
> > 
> > Calling:
> >   foreach function : functions
> >     conf.set('WITH_@0@'.format(function.to_upper()),
> > cc.has_function(function))
> >   endforeach
> > does not seem to work.
> 
> That's because conf.set(varname, value) behaves differently for
> different types of $value:
> 
> if $value is boolean then "#define/#undef $varname" is generated,
> if $value is int then "#define $varname $value" is generated,
> and if $value is string then "#define $varname $value" is generated.
> 
> Long story short, we want .to_int():
> 
> foreach function : stat_functions
>   conf.set('WITH_@0@_DECL'.format(function.to_upper()),
> cc.has_header_symbol('sys/stat.h', function).to_int())
> endforeach
> 
> 
> But this patch series fixes only a portion of the problem (that where
> compiler complained about -Wundef). Similar pattern exists for other
> $functions, e,g:
> 
> tests/virmock.h:#ifndef WITH___OPEN_2_DECL
> 
> 
> I suggest turning at least those two foreach loops above and below too.
> This still leaves us with plenty of:
> 
>   if condition
>     conf.set('WITH_SOMETHING', 1)
>   endif
> 
> but well, who said this is trivial task?

This all sounds like a large amount of churn, which risks introducing
regressions, for a very small amount of potential future benefit....
Not convinced it is worth it.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|