Bug 59165

I would suggest including the config.h then add
AC_DEFINE(GLIB_VERSION_MIN_REQUIRED, GLIB_VERSION_2_34, [Ignore post 2.34 deprecations])

or s/34/30/ if that's what logger depends on atm.

Created attachment 72717 [details] [review]
Include config.h to silence deprecation warnings from g_type_init

The other option would be to "#if !GLIB_CHECK_VERSION (2, 35, 1)" the g_type_inits. Since most files already use config.h, I decided to go this way for the sake of consistency.

(In reply to comment #1)
> I would suggest including the config.h then add
> AC_DEFINE(GLIB_VERSION_MIN_REQUIRED, GLIB_VERSION_2_34, [Ignore post 2.34
> deprecations])
> 
> or s/34/30/ if that's what logger depends on atm.


The reason for having this:
AC_DEFINE(GLIB_DISABLE_DEPRECATION_WARNINGS, 1, [Build with GLib deprecated])
... is GValueArray which was deprecated in 2.31.x. So we would still need to ignore pre-2.34 warnings.

Or did I misunderstand you?

Oh, so the logger depends on 2.25.11. So we can replace it with:
AC_DEFINE(GLIB_VERSION_MIN_REQUIRED, GLIB_VERSION_2_25).

This is getting confusing.

GLIB_VERSION_MIN_REQUIRED is available since 2.32, but that is also when GValueArray got deprecated.

Which means our minimum GLib version will be 2.32 if we use GLIB_VERSION_MIN_REQUIRED, but ignoring post 2.32 deprecations won't help because of GValueArray.

Created attachment 72719 [details] [review]
Bump minimum GLib version to 2.28

(In reply to comment #5)
> This is getting confusing.
> 
> GLIB_VERSION_MIN_REQUIRED is available since 2.32, but that is also when
> GValueArray got deprecated.

With an older GLib, the autoconf runes will expand to "#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_30" (or whatever) in config.h. That's fine, even though older GLib doesn't define GLIB_VERSION_2_30, because nothing in those versions will expand GLIB_VERSION_MIN_REQUIRED either :-)

> Which means our minimum GLib version will be 2.32 if we use
> GLIB_VERSION_MIN_REQUIRED, but ignoring post 2.32 deprecations won't help
> because of GValueArray.

Depending on 2.32, but ignoring post-2.30 deprecations, is fine. However, we can probably depend on 2.30 or even older.

telepathy-glib currently depends on 2.30 so that's effectively our baseline version. I would not object to a dependency on 2.32 either.

> Oh, so the logger depends on 2.25.11. So we can replace it with:
> AC_DEFINE(GLIB_VERSION_MIN_REQUIRED, GLIB_VERSION_2_25).

No, you can't; there are only GLIB_VERSION_X_Y macros for stable branches (actually, stable branches >= 2.26). 2.25.x, 2.33.x etc. are effectively considered to be part of the 2.26, 2.34.x, etc. stable branch that they led to.

Comment on attachment 72717 [details] [review]
Include config.h to silence deprecation warnings from g_type_init

Review of attachment 72717 [details] [review]:
-----------------------------------------------------------------

::: src/test-api.c
@@ +19,4 @@
>   */
>  
>  
> +#include "config.h"

Including config.h as the first thing in every .c file is fine, yes. A better commit message would be "Include config.h as the first thing in every .c file" \n\n "This is Autoconf best-practice, and ensures that the GLib and Telepathy version-selection macros defined by configure.ac work as intended."

::: tests/dbus/test-tpl-log-iter-pidgin.c
@@ +3,4 @@
>  #include "lib/simple-account.h"
>  #include "lib/util.h"
>  
> +#include "telepathy-logger/debug-internal.h"

Why? No reason appears in the commit message.

I think you can set(In reply to comment #5)
> This is getting confusing.
> 
> GLIB_VERSION_MIN_REQUIRED is available since 2.32, but that is also when
> GValueArray got deprecated.
> 
> Which means our minimum GLib version will be 2.32 if we use
> GLIB_VERSION_MIN_REQUIRED, but ignoring post 2.32 deprecations won't help
> because of GValueArray.

I think you can set GLIB_REQUIRED=2.32 and GLIB_VERSION_MIN_REQUIRED=2.30. That's what Empathy 3.6 does: http://git.gnome.org/browse/empathy/tree/configure.ac?h=gnome-3-6#n40

Comment on attachment 72719 [details] [review]
Bump minimum GLib version to 2.28

Review of attachment 72719 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +66,4 @@
>  ])
>  
>  # Minimal version required
> +GLIB_REQUIRED=2.28.0

I'd be inclined to spell this as "2.28" rather than "2.28.0", but this is fine too.

review+ for this patch.

(In reply to comment #8)
> Comment on attachment 72717 [details] [review] [review]
> Include config.h to silence deprecation warnings from g_type_init
> 
> Review of attachment 72717 [details] [review] [review]:
> -----------------------------------------------------------------
> [...]
>
> ::: tests/dbus/test-tpl-log-iter-pidgin.c
> @@ +3,4 @@
> >  #include "lib/simple-account.h"
> >  #include "lib/util.h"
> >  
> > +#include "telepathy-logger/debug-internal.h"
> 
> Why? No reason appears in the commit message.

Because of _tpl_debug_set_flags_from_env

When we include config.h, it turns on ENABLE_DEBUG, which in turn leads to the usage of _tpl_debug_set_flags_from_env.

Do you want me to split it into a separate patch?

(In reply to comment #11)
> When we include config.h, it turns on ENABLE_DEBUG, which in turn leads to
> the usage of _tpl_debug_set_flags_from_env.

OK, that's fine. Please say so in the commit message.

(This was already a bug: code that was meant to be conditionally-compiled was actually never compiled...)

Have you checked that every *.c has either #include "config.h" or #include <config.h> as its first non-comment, non-whitespace line? That's the ideal situation.

(<telepathy.freedesktop.org/wiki/Style>)

(In reply to comment #10)
> Comment on attachment 72719 [details] [review] [review]
> Bump minimum GLib version to 2.28
> 
> Review of attachment 72719 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.ac
> @@ +66,4 @@
> >  ])
> >  
> >  # Minimal version required
> > +GLIB_REQUIRED=2.28.0
> 
> I'd be inclined to spell this as "2.28" rather than "2.28.0", but this is
> fine too.
> 
> review+ for this patch.

Changed it to "2.28" instead of "2.28.0" and pushed to master.

Created attachment 72722 [details] [review]
Include config.h as the first thing in every .c file

(In reply to comment #12)
> (In reply to comment #11)
> > When we include config.h, it turns on ENABLE_DEBUG, which in turn leads to
> > the usage of _tpl_debug_set_flags_from_env.
> 
> OK, that's fine. Please say so in the commit message.

Done.

> Have you checked that every *.c has either #include "config.h" or #include
> <config.h> as its first non-comment, non-whitespace line? That's the ideal
> situation.
> 
> (<telepathy.freedesktop.org/wiki/Style>)

No, I hadn't initially. I went through all the *.c files and have updated the patch accordingly.

Comment on attachment 72722 [details] [review]
Include config.h as the first thing in every .c file

Review of attachment 72722 [details] [review]:
-----------------------------------------------------------------

review+, thanks

Thanks for the review. Pushed to master.