Bug 44649

cloning since we need to fix the build to compile plugins as dll. still

See also bug 44331, comments 3-5 for background

Removing patch keyword from here. There's no reviewable patch yet :)

(In reply to comment #2)
> See also bug 44331, comments 3-5 for background
> 
> Removing patch keyword from here. There's no reviewable patch yet :)

I"m facing a big problem, plugins depend on gabble_connection_get_type() of connection.c, and if I include connection.c the symbols in this file refers to every other file in the source tree so to include connection.c, I must add all the other files in the source tree.

Any ideas how I can solve this issue ?

You must also move individual symbols such as this one to separate files, not just try to pick existing files to each lib.

Something like connection-plugins.c might be a sensible target in this case.

When moving definitions between source files, you'll also occasionally need to forward declare more stuff they need in internal headers, but that's fine.

Gabble maintainers, any comments?On 11.1.2012 2:36 bugzilla-daemon@freedesktop.org wrote:
https://bugs.freedesktop.org/show_bug.cgi?id=44649

--- Comment #3 from Siraj Razick <siraj.razick@collabora.co.uk> 2012-01-11 02:36:16 EET ---
(In reply to comment #2)
> See also bug 44331, comments 3-5 for background
>
> Removing patch keyword from here. There's no reviewable patch yet :)

I"m facing a big problem, plugins depend on gabble_connection_get_type() of
connection.c, and if I include connection.c the symbols in this file refers to
every other file in the source tree so to include connection.c, I must add all
the other files in the source tree.

Any ideas how I can solve this issue ?

--
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

(In reply to comment #3)
> (In reply to comment #2)
> > See also bug 44331, comments 3-5 for background
> > 
> > Removing patch keyword from here. There's no reviewable patch yet :)
> 
> I"m facing a big problem, plugins depend on gabble_connection_get_type() of
> connection.c, and if I include connection.c the symbols in this file refers to
> every other file in the source tree so to include connection.c, I must add all
> the other files in the source tree.
> 
> Any ideas how I can solve this issue ?

(In reply to comment #4)
> You must also move individual symbols such as this one to separate files, not
> just try to pick existing files to each lib.
> 
> Something like connection-plugins.c might be a sensible target in this case.
> 

I had a discussion with Oggis on irc about this, it's not really easy to factor out gabble_connection_get_type() into
different files.  And currently the connection is a property of the plugins  So oggis proposed that it could be solved by having a GabbleSidecarManager which will hand over the GabbleConnections to plugins and there wouldn't be a gabble/connection.h with the update_sidecar_capabilities, add_sidecar_own_caps functions, but some gabble/sidecar-manager.h declaring those

Any comments or idea's about this approach ?  We are also wondering if there any other workaround which we can do without needing to change the plugin API again .

(In reply to comment #5)
> I had a discussion with Oggis on irc about this, it's not really easy to factor
> out gabble_connection_get_type() into
> different files.  And currently the connection is a property of the plugins  So

Yeah, the plugins currently get passed a GabbleConnection instance in the create_sidecar_async function, and the sidecars generally take a reference to it so that they can call these functions:

http://cgit.freedesktop.org/telepathy/telepathy-gabble/tree/gabble/connection.h

This is not the full GabbleConnection API. However, it's still a sizeable part.

The core of the problem is that the gabble_connection_get_type() implementation which expands out of the G_DEFINE_TYPE_WITH_CODE invocation needs to reference all the interface init symbols for all interfaces GabbleConnection implements. And these in turn recursively depend on most everything in Gabble, so if we pull the current gabble_connection_get_type() into the plugin DLL, we have to pull in about everything else as well.

> oggis proposed that it could be solved by having a GabbleSidecarManager which
> will hand over the GabbleConnections to plugins and there wouldn't be a
> gabble/connection.h with the update_sidecar_capabilities, add_sidecar_own_caps
> functions, but some gabble/sidecar-manager.h declaring those

No sorry, I dunno if I've explained this unclearly. They can't hand over a GabbleConnection; if we want to do any GObject type sanity checks at all, we need the gabble_connection_get_type() definition.

Thus my idea: split out exactly those parts required by the gabble/connection.h API from GabbleConnection to a separate object. This could be something like GabbleSidecarManager, better name suggestions welcome. Then plugins would only refer to a GabbleSidecarManager and a TpBaseConnection, and we could drop at least most of the GInterface implementation dependencies out from the plugin lib.

But is this practical? I see from connection.h that this will anyway bring in to the sidecar manager object at least the caps cache in addition to the Wocky context and local identity which are basically unavoidable. Of course, we could play some further dependency inversion tricks to cut even those out.

However, even without that this is still a big change... the plugins are quite tied to GabbleConnection details - some even connect to its GObject signals. Of course, the sidecar manager object could relay the signals, but we have to define which GabbleConnection signals are part of the plugin API and which aren't etc for this.

Of course, being plugin accessible API of GabbleConnection, all those caps cache things etc need to be in the plugin lib whether they're a part of GabbleConnection or some smaller object hanging off it.

> 
> Any comments or idea's about this approach ?  We are also wondering if there
> any other workaround which we can do without needing to change the plugin API
> again .

Anything which would enable a compatible definition of GABBLE_TYPE_CONNECTION without pulling in as dependencies all the GInterface implementations from the G_DEFINE_TYPE_WITH_CODE invocation would do.

A dirty hack idea:

* define a global GType variable in the plugin lib
* make some GabbleConnection construction function which is always called before  plugins are given the GabbleConnection set that variable to the real return value of gabble_connection_get_type()
* define GABBLE_TYPE_CONNECTION to read that variable in plugins
* ... but leave it really calling gabble_connection_get_type() for the gabble internals, so that the basic things like g_object_new work in the first place.

Ugghhh....

If we just can't work anything out, we have to pretty much just turn the former gabble-convenience library as a whole into a gabble-plugins library. But that'd be extremely shit because as a DLLs, its ordinals would change basically every second and your plugins would just fail, fail, fail to runtime link to it and would require a relink at least every gabble release.

Sorry, DLLs are utter crappy troublemakers compared to .so's. (But they're fast at runtime, essentially always prelinked, woo...)

Ideas welcome,
Olli
The GObject noob

If there really is no solution other than a very very very major gabble internals refactor, an alternative for our Ytstenut purposes could be just admitting the plugin API does not exist on Windows, and instead merging the Ytstenut service discovery and channel code directly to Gabble. How would that sound?

(In reply to comment #6)
> Thus my idea: split out exactly those parts required by the gabble/connection.h
> API from GabbleConnection to a separate object. This could be something like
> GabbleSidecarManager, better name suggestions welcome.

... or to a GInterface that GabbleConnection would implement.

In Mission Control, the plugin API is entirely in terms of GInterfaces, evenly divided between "implement this in your plugin" and "only implement this in MC"; then MC contains exactly one implementation of each "implement this in MC" interface. This might be overkill, but it keeps the plugins library really small.

> However, even without that this is still a big change... the plugins are quite
> tied to GabbleConnection details - some even connect to its GObject signals.

My opinion would tend to be "ceci n'est pas un plugin API" but you know what I'm like about stable API guarantees ;-)

(Gabble's plugin ABI can't really be considered stable anyway, until Wocky is a real shared library with a SONAME, since plugins are allowed and encouraged to use Wocky.)

(In reply to comment #7)
> an alternative for our Ytstenut purposes could be just
> admitting the plugin API does not exist on Windows, and instead merging the
> Ytstenut service discovery and channel code directly to Gabble

I'd feel happier about this if it was structured somewhat like a plugin, with only the linking/initialization being different.

(In reply to comment #6)
> A dirty hack idea:
> 
> * define a global GType variable in the plugin lib
> * make some GabbleConnection construction function which is always called
> before  plugins are given the GabbleConnection set that variable to the real
> return value of gabble_connection_get_type()
> * define GABBLE_TYPE_CONNECTION to read that variable in plugins
> * ... but leave it really calling gabble_connection_get_type() for the gabble
> internals, so that the basic things like g_object_new work in the first place.

You don't need to go that far. Just define a GABBLE_TYPE_PLUGIN_CONNECTION GInterface (whose only implementation is GabbleConnection) in the plugin-enabler library, and have the plugins use GABBLE_IS_PLUGIN_CONNECTION where they would normally use GABBLE_IS_CONNECTION, etc.

You could even have:

typedef struct _GabbleConnection GabbleConnection;

#ifndef GABBLE_IN_SRC
    /* we're compiling a plugin */
#   define GABBLE_TYPE_CONNECTION GABBLE_TYPE_PLUGIN_CONNECTION
#   define GABBLE_IS_CONNECTION(x) GABBLE_IS_PLUGIN_CONNECTION(x)
#   define GABBLE_CONNECTION(x) GABBLE_PLUGIN_CONNECTION(x)
    typedef GabbleConnection GabblePluginConnection;
    /* etc. */
#endif

in the plugin-enabler header, and -DGABBLE_IN_SRC in src/Makefile.am?

(In reply to comment #8)
> (In reply to comment #6)
> > Thus my idea: split out exactly those parts required by the gabble/connection.h
> > API from GabbleConnection to a separate object. This could be something like
> > GabbleSidecarManager, better name suggestions welcome.
> 
> ... or to a GInterface that GabbleConnection would implement.
> 
> In Mission Control, the plugin API is entirely in terms of GInterfaces, evenly
> divided between "implement this in your plugin" and "only implement this in
> MC"; then MC contains exactly one implementation of each "implement this in MC"
> interface. This might be overkill, but it keeps the plugins library really
> small.
> 

Hey. Good one that! Very good! Excellent in fact. It's dependency inversion at the level of the plugin API.

Siraj: for the moment, this would mean moving these offending GabbleConnection symbols to an explicit GabbleConnectionPluginServices GInterface, and making Gabble fill the GabbleConnectionPluginServicesInterface structure for it with function pointers to the current implementations.

The current gabble/connection.h functions would be renamed to gabble_connection_plugin_services_add_sidecar_capabilities etc, which take a GabbleConnectionPluginServices * instead of GabbleConnection * and forward the calls through the interface vtable, as usual for GInterfaces.

You also need to move the GabbleConnection signals plugins connect to to GabbleConnectionPluginServices. grep for g_signal_connect in ALL plugins to find out which these actually are.

(In reply to comment #9)
> (In reply to comment #6)
> > A dirty hack idea:
> > 
> > * define a global GType variable in the plugin lib
> > * make some GabbleConnection construction function which is always called
> > before  plugins are given the GabbleConnection set that variable to the real
> > return value of gabble_connection_get_type()
> > * define GABBLE_TYPE_CONNECTION to read that variable in plugins
> > * ... but leave it really calling gabble_connection_get_type() for the gabble
> > internals, so that the basic things like g_object_new work in the first place.
> 
> You don't need to go that far. Just define a GABBLE_TYPE_PLUGIN_CONNECTION
> GInterface (whose only implementation is GabbleConnection) in the
> plugin-enabler library, and have the plugins use GABBLE_IS_PLUGIN_CONNECTION
> where they would normally use GABBLE_IS_CONNECTION, etc.
> 
> You could even have:
> 
> typedef struct _GabbleConnection GabbleConnection;
> 
> #ifndef GABBLE_IN_SRC
>     /* we're compiling a plugin */
> #   define GABBLE_TYPE_CONNECTION GABBLE_TYPE_PLUGIN_CONNECTION
> #   define GABBLE_IS_CONNECTION(x) GABBLE_IS_PLUGIN_CONNECTION(x)
> #   define GABBLE_CONNECTION(x) GABBLE_PLUGIN_CONNECTION(x)
>     typedef GabbleConnection GabblePluginConnection;
>     /* etc. */
> #endif
> 
> in the plugin-enabler header, and -DGABBLE_IN_SRC in src/Makefile.am?

Well, this PluginConnection could be what I referred to as ConnectionPluginServices. But I like the idea of making the GabbleConnection operations used by plugins virtual functions in it in addition to just having it be a fake base GType, because then it'll be much easier to avoid pulling in the universe as dependencies to those symbols.

The patch!

This seems fair enough but there's a lot of style errors. Perhaps you might want to consult this wiki page:

http://telepathy.freedesktop.org/wiki/Style

- * connection.h - connection API available to telepathy-gabble plugins
- * Copyright © 2010 Collabora Ltd.
- * Copyright © 2010 Nokia Corporation
...
+ * plugin-connection.h — Connection API available to telepathy-gabble plugins
+ * Copyright © 2009 Collabora Ltd.
+ * Copyright © 2009 Nokia Corporation

How did this change from 2010 to 2009? Also, you should update the year for Collabora. Same thing for the source file.

+ GabblePluginConnection *plugin_connecion,

connection

-     TpBaseConnection *connection)
+    GabblePluginConnection *plugin_connection,
+    TpBaseConnection *connection)

-  tmp = gabble_plugin_loader_create_channel_managers (loader, conn);
+   tmp = gabble_plugin_loader_create_channel_managers (loader, plugin_connection,

Why the change in indentation in these?

+lib_LTLIBRARIES = libgabble-plugin.la

Can I be really annoying and ask you to change this to -plugins instead of -plugin please?

+     G_IMPLEMENT_INTERFACE (GABBLE_TYPE_PLUGIN_CONNECTION,
+       gabble_connection_plugin_service_iface_init);

Seems an odd iface_init function name choice?

+gchar *
+_gabble_plugin_connection_get_full_jid (GabblePluginConnection *plugin_conn)

Should be static, no? Same with most of the other functions that are set in GabblePluginConnectionIface the struct as they're not used anywhere else in gabble?

+ sig_id_porter_available = g_signal_new (
+ "porter-available",
+ G_OBJECT_CLASS_TYPE (iface),
+ G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED, 0, NULL, NULL,
+ g_cclosure_marshal_VOID__OBJECT, G_TYPE_NONE, 1, WOCKY_TYPE_PORTER);
+ g_once_init_leave (&once, 1);

Indentation please.

+ * SECTION: gabble-connection-plugin-service

Seems this needs to be updated.

+GType
+gabble_plugin_connection_get_type (void)

Could you use G_DEFINE_INTERFACE() here instead, to save some lines?

+gabble_plugin_connection_get_caps (
+    GabblePluginConnection *plugin_connection, TpHandle handle)

One argument per line please.

There's also some random whitespace added here and there out of context. You should review your own patch before submitting it here.

patch updated

(In reply to comment #14)
> patch updated

patch  2: http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/log/?h=windows-fixes

Windows specific build fixes, which enables us to build the plugins correctly as .dll files

note : WOCKY_LIBS was removed from few places since it results in multiple definitions of wocky symbols.

I am reviewing the diff between the two patches:

- * Copyright © 2009 Collabora Ltd.
- * Copyright © 2009 Nokia Corporation
+ * Copyright © 2012 Collabora Ltd.

- * Copyright © 2009 Collabora Ltd.
- * Copyright © 2009 Nokia Corporation
+ * Copyright © 2012 Collabora Ltd.

Are these really correct?

+typedef GabblePluginConnectionIface GabblePluginConnectionInterface;

Just rename the interface to …Interface instead of having a typedef around.

(In reply to comment #13)
> +gchar *
> +_gabble_plugin_connection_get_full_jid (GabblePluginConnection *plugin_conn)
> 
> Should be static, no? Same with most of the other functions that are set in
> GabblePluginConnectionIface the struct as they're not used anywhere else in
> gabble?

Is this not the case? You don't seem to have changed anything and you didn't reply?

> There's also some random whitespace added here and there out of context. You
> should review your own patch before submitting it here.

It would be nice if this was fixed but it's no blocker.

(In reply to comment #16)
> I am reviewing the diff between the two patches:
> 
> - * Copyright © 2009 Collabora Ltd.
> - * Copyright © 2009 Nokia Corporation
> + * Copyright © 2012 Collabora Ltd.
> 
> - * Copyright © 2009 Collabora Ltd.
> - * Copyright © 2009 Nokia Corporation
> + * Copyright © 2012 Collabora Ltd.
> 
> Are these really correct?

plugin-connection.h/c created in 2012 by collabora so int he final patch it's 2012, and
in connection.h was merged back into one .. so it's 2005-2012 .. 

> 
> +typedef GabblePluginConnectionIface GabblePluginConnectionInterface;
> 
> Just rename the interface to …Interface instead of having a typedef around.
> 
> (In reply to comment #13)
> > +gchar *
> > +_gabble_plugin_connection_get_full_jid (GabblePluginConnection *plugin_conn)
> > 
> > Should be static, no? Same with most of the other functions that are set in
> > GabblePluginConnectionIface the struct as they're not used anywhere else in
> > gabble?
> 
> Is this not the case? You don't seem to have changed anything and you didn't
> reply?
I just missed it sorry.. I don't have a reason not to make it static.

> 
> > There's also some random whitespace added here and there out of context. You
> > should review your own patch before submitting it here.
> 
> It would be nice if this was fixed but it's no blocker.

I'll fix them :)

patch now updated

and 2nd patch for windows specific stuff 

http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix&id=f2e32c07d23bce1eb6880c63769d413cd7346297

This removes plugin-util.h from the previous (2nd ) patch, and replaces it with a wocky function so that we don' t need to split uti.h/c as suggested on IRC .

(In reply to comment #18)
> patch now updated
> 
> and 2nd patch for windows specific stuff 
> 
> http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix&id=f2e32c07d23bce1eb6880c63769d413cd7346297

http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix&id=29de553a5ca01fb5497e2a4ed24a2525965f6b00

actually since previous patch results in a load error in windows.

> 
> This removes plugin-util.h from the previous (2nd ) patch, and replaces it with
> a wocky function so that we don' t need to split uti.h/c as suggested on IRC .

+Libs: -L${abs_top_srcdir}/src -lgabble-plugins

I don't think this is right for the uninstalled variant. I think it should be:

   Libs: ${abs_top_builddir}src/libgabble-plugins.la

It's fine for the installed one though.

-AM_LDFLAGS = -module -avoid-version -shared
+AM_LDFLAGS = -avoid-version -shared -no-undefined

Why the extra space?

 libgabble_plugins_la_LIBADD = \
     $(top_builddir)/lib/ext/telepathy-yell/telepathy-yell/libtelepathy-yell.la \
+        $(top_builddir)/lib/loudmouth/libloudmouth.la \
     $(ALL_LIBS)

Why different indentation?

-libgabble_plugins_la_SOURCES = $(top_srcdir)/gabble/plugin.h \
- $(top_srcdir)/gabble/caps-channel-manager.h \
- $(top_srcdir)/gabble/plugin-connection.h \
- plugin-connection.c \
- caps-channel-manager.c \
- plugin.c \
- capabilities.c \
- debug.c \
- sidecar.c
+libgabble_plugins_la_SOURCES = \
+ $(top_srcdir)/gabble/capabilities.h \
+ capabilities.c \
+ $(top_srcdir)/gabble/caps-channel-manager.h \
+ caps-channel-manager.c \
+ $(top_srcdir)/gabble/debug.h \
+ debug.c \
+ $(top_srcdir)/gabble/error.h \
+ error.c \
+ $(top_srcdir)/gabble/plugin.h \
+ plugin.c \
+ $(top_srcdir)/gabble/plugin-connection.h \
+ plugin-connection.c \
+ sidecar.c

Same thing here, why different indentation? Firefox has completely ruined the paste though... sigh.

Other than those this patch looks fine. In future do you think you could split your patches into separate commits which do one thing in one commit? It makes it much much much easier to review and that's part of the point of having a VCS which makes commits so easy (unlike Subversion).

Your first patch looks fine now. Please make sure `make check` passes before you merge it though.

http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix&id=f36bba1f367d5439f2b60c445aa216e985d69e04

patch updated .

(In reply to comment #20)
> +Libs: -L${abs_top_srcdir}/src -lgabble-plugins
> 
> I don't think this is right for the uninstalled variant. I think it should be:
> 
>    Libs: ${abs_top_builddir}src/libgabble-plugins.la
> 
> It's fine for the installed one though.
> 
> -AM_LDFLAGS = -module -avoid-version -shared
> +AM_LDFLAGS = -avoid-version -shared -no-undefined
> 
> Why the extra space?
> 
>  libgabble_plugins_la_LIBADD = \
>      $(top_builddir)/lib/ext/telepathy-yell/telepathy-yell/libtelepathy-yell.la
> \
> +        $(top_builddir)/lib/loudmouth/libloudmouth.la \
>      $(ALL_LIBS)
> 
> Why different indentation?
> 
> -libgabble_plugins_la_SOURCES = $(top_srcdir)/gabble/plugin.h \
> - $(top_srcdir)/gabble/caps-channel-manager.h \
> - $(top_srcdir)/gabble/plugin-connection.h \
> - plugin-connection.c \
> - caps-channel-manager.c \
> - plugin.c \
> - capabilities.c \
> - debug.c \
> - sidecar.c
> +libgabble_plugins_la_SOURCES = \
> + $(top_srcdir)/gabble/capabilities.h \
> + capabilities.c \
> + $(top_srcdir)/gabble/caps-channel-manager.h \
> + caps-channel-manager.c \
> + $(top_srcdir)/gabble/debug.h \
> + debug.c \
> + $(top_srcdir)/gabble/error.h \
> + error.c \
> + $(top_srcdir)/gabble/plugin.h \
> + plugin.c \
> + $(top_srcdir)/gabble/plugin-connection.h \
> + plugin-connection.c \
> + sidecar.c
> 
> Same thing here, why different indentation? Firefox has completely ruined the
> paste though... sigh.
> 
> Other than those this patch looks fine. In future do you think you could split
> your patches into separate commits which do one thing in one commit? It makes
> it much much much easier to review and that's part of the point of having a VCS
> which makes commits so easy (unlike Subversion).

(In reply to comment #22)
> http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix&id=f36bba1f367d5439f2b60c445aa216e985d69e04
> 
> patch updated .

Did you look at the patch? You didn't fix the -uninstalled.pc file, and there's still indentation problems. Look at the link you pasted and see the errors yourself.

- conn-contact-info.c \
+ c onn-contact-info.c \

I'm not sure if this was in before but with this, gabble can't possibly build...

(In reply to comment #23)
> (In reply to comment #22)
> > http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix&id=f36bba1f367d5439f2b60c445aa216e985d69e04
> > 
> > patch updated .
> 
> Did you look at the patch? You didn't fix the -uninstalled.pc file, and there's
> still indentation problems. Look at the link you pasted and see the errors
> yourself.
> 
> - conn-contact-info.c \
> + c onn-contact-info.c \
> 
> I'm not sure if this was in before but with this, gabble can't possibly
> build...

sorry about that.. just having a sloppy day http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix

(In reply to comment #24)
> sorry about that.. just having a sloppy day
> http://cgit.collabora.com/git/user/siraj/telepathy-gabble.git/commit/?h=windows-compile-fix

No worries. This looks fine now.

Although in future please split up commits like this into ones that do specific things like your other mingw32 wocky patches today.

++

Has been merged already.