Created attachment 52963 [details] [review] generate.sh modifications The patches included allow building Gabble for Android, using Android NDK and the androgenizer's tool
Created attachment 52964 [details] [review] Android-specific files
Comment on attachment 52963 [details] [review] generate.sh modifications Review of attachment 52963 [details] [review]: ----------------------------------------------------------------- ::: autogen.sh @@ -21,5 @@ > -then > - echo "+ Setting up submodules" > - git submodule init > -fi > -git submodule update We need this to happen by default, but you can add a way to turn it off if you want (or just use autoreconf -i -f instead of autogen.sh, if that's all you want). @@ -26,5 @@ > - > -# launch Wocky's autogen.sh > -cd lib/ext/wocky > -sh autogen.sh --no-configure > -cd ../../.. We need these to happen in autogen too, unless the submodules are specifically disabled. @@ -34,5 @@ > -sh autogen.sh --no-configure > -cd ../../.. > - > -# Honor NOCONFIGURE for compatibility with gnome-autogen.sh > -if test x"$NOCONFIGURE" = x; then Please don't remove support for NOCONFIGURE, jhbuild uses it. ::: configure.ac @@ -330,4 @@ > > dnl Check for Wocky > # re-enable once Wocky has been released as a lib > -export PKG_CONFIG_PATH=$PKG_CONFIG_PATH:"$ac_top_build_prefix"lib/ext/wocky/wocky We need this feature for non-Android platforms, so we can't just remove it; but you can add a --with-wocky-submodule configure flag that sets the WITH_WOCKY_SUBMODULE variable if you want, and turn off the submodule (--without-wocky-submodule) for Android. Or call it --with-system-wocky and reverse the meaning, if you prefer that. @@ -335,5 @@ > AC_SUBST(WOCKY_CFLAGS) > AC_SUBST(WOCKY_LIBS) > > dnl Check for tp-yell > -export PKG_CONFIG_PATH=$PKG_CONFIG_PATH:"$ac_top_build_prefix"lib/ext/telepathy-yell/telepathy-yell Likewise for --without-yell-submodule. @@ -377,4 @@ > tests/suppressions/Makefile \ > tests/twisted/Makefile \ > lib/Makefile \ > - lib/ext/Makefile \ Similarly, this should be conditional (and everything in that file should also be conditional). ::: lib/ext/Makefile.am @@ -1,1 @@ > -SUBDIRS = lib/ext/
(In reply to comment #2) > > -SUBDIRS = > > lib/ext/ That was meant to say: lib/ext/Makefile.am should all be conditional on the conditions you add to configure.ac, instead of being deleted.
Comment on attachment 52964 [details] [review] Android-specific files Review of attachment 52964 [details] [review]: ----------------------------------------------------------------- ::: src/Makefile.am @@ +283,5 @@ > + -:REL_TOP $(top_srcdir) -:ABS_TOP $(abs_top_srcdir) \ > + -:SOURCES $(libgabble_convenience_la_SOURCES) \ > + $(nodist_libgabble_convenience_la_SOURCES) main.c \ > + -:CFLAGS $(DEFS) $(CFLAGS) $(DEFAULT_INCLUDES) $(INCLUDES) \ > + $(AM_CFLAGS) -DBUILD_AS_ANDROID_SERVICE \ Strictly speaking -D should go in CPPFLAGS. @@ +286,5 @@ > + -:CFLAGS $(DEFS) $(CFLAGS) $(DEFAULT_INCLUDES) $(INCLUDES) \ > + $(AM_CFLAGS) -DBUILD_AS_ANDROID_SERVICE \ > + -:CPPFLAGS $(CPPFLAGS) $(AM_CPPFLAGS) -I../lib \ > + -:LDFLAGS $(libgabble_convenience_la_LIBADD) -lgio-2.0 -ldbus-1 \ > + -lsoup -lgabble_ext -lnice -lgcrypt -lgnutls -lxml2 -lsqlite3 \ It would be much better if we could avoid hard-coding these. ::: src/main.c @@ +27,2 @@ > main (int argc, > +#endif To not screw up syntax highlighting I'd vaguely prefer: int #ifdef ... telepathy_gabble_main #else main #endif (int argc, char **argv) { } or, even better (if this works): #ifdef BUILD_AS_ANDROID_SERVICE #define main telepathy_gabble_main #endif int main (int argc, char **argv) { }
Created attachment 53017 [details] [review] Support for Android patch It supersedes both previous patches, addressing the suggestions made
Comment on attachment 53017 [details] [review] Support for Android patch Review of attachment 53017 [details] [review]: ----------------------------------------------------------------- ::: autogen.sh @@ +19,5 @@ > +#Check if Android build > +run_android_build=false > +for arg in $*; do > + case $arg in > + --enable-android) For what it's worth, --enable-android doesn't actually enable or disable any Android-specific code (as far as I can tell), it just controls whether submodules are used. Perhaps --disable-submodules would be a better name for the flag, and fetch_submodules a better name for the variable. @@ +32,4 @@ > run_configure=true > for arg in $*; do > case $arg in > --no-configure) This code is duplicated in the two branches of this if block. You could pull this out to above 'if test $run_android_build' so you don't need to duplicate it. ::: configure.ac @@ +330,5 @@ > > +dnl Check if Android build > +AC_ARG_ENABLE(android, > + AC_HELP_STRING([--enable-android],[compile Android code]), > + android_build=$enableval, android_build=no ) So this would become AC_ARG_ENABLE(submodules, AC_HELP_STRING([--disable-submodules], [Use system versions of Wocky and Telepathy-Yell, rather than submodules], enable_submodules=$enablevel, enable_submodules=yes)) @@ +336,3 @@ > dnl Check for Wocky > # re-enable once Wocky has been released as a lib > +if test x$android_build = xno; then …and this would be if test x$enable_submodules = xyes; then ::: src/Makefile.am @@ +286,5 @@ > + -:CFLAGS $(DEFS) $(CFLAGS) $(DEFAULT_INCLUDES) $(INCLUDES) \ > + $(AM_CFLAGS) -DBUILD_AS_ANDROID_SERVICE \ > + -:CPPFLAGS $(CPPFLAGS) $(AM_CPPFLAGS) -I../lib \ > + -:LDFLAGS $(libgabble_convenience_la_LIBADD) -lgio-2.0 -ldbus-1 \ > + -lsoup -lgabble_ext -lnice -lgcrypt -lgnutls -lxml2 -lsqlite3 \ Simon's point about this—“It would be much better if we could avoid hard-coding these.”—hasn't been addressed.
(In reply to comment #6) > So this would become > > AC_ARG_ENABLE(submodules, > AC_HELP_STRING([--disable-submodules], which apparently should be spelled AS_HELP_STRING these days.
Created attachment 53415 [details] [review] Support for Android patch Issues addressed.
Comment on attachment 53415 [details] [review] Support for Android patch Review of attachment 53415 [details] [review]: ----------------------------------------------------------------- ::: autogen.sh @@ +32,5 @@ > + # Fetch submodules if needed > + if test ! -f lib/ext/wocky/autogen.sh -o ! -f lib/ext/telepathy-yell/autogen.sh; > + then > + echo "+ Setting up submodules" > + git submodule init This should be indented. ::: configure.ac @@ +329,5 @@ > ac_configure_args=$prev_ac_configure_args > > +dnl Check if Android build > +AC_ARG_ENABLE(submodules, > + AS_HELP_STRING([--disable-submodules],[compile Android code]), The description is still wrong! I suggested wording in my last review… ::: src/Makefile.am @@ +286,5 @@ > + -:CFLAGS $(DEFS) $(CFLAGS) $(DEFAULT_INCLUDES) $(INCLUDES) \ > + $(AM_CFLAGS) -DBUILD_AS_ANDROID_SERVICE \ > + -:CPPFLAGS $(CPPFLAGS) $(AM_CPPFLAGS) $(telepathy_gabble_LDFLAGS) \ > + -:LDFLAGS $(telepathy_gabble_LDADD) $(libgabble_convenience_la_LIBADD) -lgio-2.0 -ldbus-1 \ > + -lsoup -lgabble_ext -lnice -lgcrypt -lgnutls -lxml2 -lsqlite3 \ My point about this—“Simon's point about this—“It would be much better if we could avoid hard-coding these.”—hasn't been addressed.”—hasn't been addressed.
Created attachment 53416 [details] [review] Support for Android patch The previous patch was missing a file
Created attachment 53555 [details] [review] Support for Android patch Solved the hardcoding of libs, indentation, and the forgotten rename of the submodules option diescription
Comment on attachment 53555 [details] [review] Support for Android patch Review of attachment 53555 [details] [review]: ----------------------------------------------------------------- Looks great apart from one nit-pick (sorry...) ::: configure.ac @@ +214,5 @@ > enable_gtk_doc=yes, enable_gtk_doc=no) > AM_CONDITIONAL([ENABLE_GTK_DOC], [test "x$enable_gtk_doc" = xyes]) > > +dnl Check for Gio > +PKG_CHECK_MODULES(GIO, [gio-2.0]) Just add this to the glib block below. gio is part of glib, like gobject and gthread.
Created attachment 53576 [details] [review] Support for Android - fixed Fixed the nitpicking :)
http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/?id=c6e76ac44acd46e8d5a1c9f2cc950548d7a30ead
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.