Re: [Qemu-devel] [PATCH v2 2/3] glib: enforce the minimum required version and warn about old APIs

[Daniel P . Berrangé]

On Thu, Jun 07, 2018 at 10:57:56AM +0200, Thomas Huth wrote:
> On 06.06.2018 19:32, Daniel P. Berrangé wrote:
> > There are two useful macros that can be defined before including
> > glib.h that are related to the min required glib version
> > 
> >  - GLIB_VERSION_MIN_REQUIRED
> > 
> >    When this is defined, if code uses an API that was deprecated
> >    in this version, or older, a compiler warning will be emitted.
> >    This alerts maintainers to update their code to whatever new
> >    replacement API is now recommended best practice.
> > 
> >  - GLIB_VERSION_MAX_ALLOWED
> > 
> >    When this is defined, if code uses an API that was introduced
> >    in a version that is newer than the declared version, a compiler
> >    warning will be emitted. This alerts maintainers if new code
> >    accidentally uses functionality that won't be available on some
> >    supported platforms.
> > 
> > The GLIB_VERSION_MAX_ALLOWED constant makes it a bit harder to opt
> > in to using specific new APIs with a GLIB_CHECK_VERSION conditional.
> > To workaround this Pragmas can be used to temporarily turn off the
> > -Wdeprecated-declarations compiler warning, while a static inline
> > compat function is implemented. This workaround is illustrated with the
> > implementation of the g_strv_contains method to satisfy the test suite.
> 
> I wonder whether that's a little bit too much magic already. We could
> also set GLIB_VERSION_MAX_ALLOWED to the version where we've already got
> the work-arounds in place (i.e. to 2.44 here). If we introduce
> additional code that uses other new functions from 2.41 - 2.44, this
> should also be caught by the patchew CI builders?

I don't think that's a good direction to take - if we follow that
through and someone backports a function from 2.56, we'll loose
100% of the benefit of the MAX_ALLOWED checking. Given we only have
a handful of backported functions, that is not a good tradeoff.


> > Signed-off-by: Daniel P. Berrangé <address@hidden>
> > ---
> >  include/glib-compat.h                    | 67 ++++++++++++++++++++++++
> >  tests/docker/dockerfiles/centos6.docker  | 30 -----------
> >  tests/docker/dockerfiles/min-glib.docker |  8 ---
> 
> The docker files are not directly related to this patch, are they? If
> they are, please mention it in the patch description. If not, please
> move this to a separate patch (or maybe into patch 1).

Yeah should go in the patch that bumps the min version really.


> > +/* Ask for warnings for anything that was marked deprecated in
> > + * the defined version, or before. It is a candidate for rewrite.
> > + */
> > +#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_40
> > +
> > +/* Ask for warnings if code tries to use function that did not
> > + * exist in the defined version. These risk breaking builds
> > + */
> > +#define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_40
> > +
> > +_Pragma("GCC diagnostic push")
> > +_Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"")
> 
> Why not "#pragma" ? I think that's a little bit more common?

I tend to always use _Pragma as that offers superset of functionality
of #pramga, because _Pragma can be used in macros.

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