g_type_init is deprecated from GLib 2.35.1, which causes build failures when building against latest versions of GLib. Although GLIB_DISABLE_DEPRECATION_WARNINGS is defined in config.h, some files do not include it. (I believe 2.34.x is the latest version that we can depend on, so removing the g_type_init is not an option yet.)
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.