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 :|
