|
Description
Jonny Lamb
2012-03-01 09:47:32 UTC
(Oh I just realised this patch is missing the ABI files for tp-glib and tp-glib-lowlevel. You get the idea.) (In reply to comment #0) > Simon, fancy doing this? You will probably actually get it right, after all. Sure, taking this bug. I did try already, but ended up with Bug #46523 two or three layers of yak shaving later. I think we're going to end up with something like this: - tp-glib-core: minimal TpProxy infrastructure, etc.; long-term stable (breaks every few years) - tp-glib-codegen (or -lowlevel or something): client, service codegen; breaks ABI in the development branch just after some/all 6-monthly stable branches, i.e. every 1-2 release cycles; cannot contain GTypes or other things that are flat namespaces, since those would defeat symbol versioning - tp-glib: the high-level library; as stable as -core I talked about the split briefly with andrunko the other day. Errors, enums, flags, GTypes should be in either -core or the high-level library. In tp-qt they have to go in -core because the codegen needs them; in tp-glib I think we can get away with having them be high-level-library stuff. Rationale: they're flat global namespaces within im.telepathy1, so we're not going to break them unless we move to telepathy2. (We might end up with some horribly ugly type names like Foo_Thing3_Map after a while, though.) Interface names and everything that embeds them (contact attr tokens, properties) should be in tp-glib-codegen. Rationale: they break exactly as often as the interfaces themselves. If we move from dbus-glib to GDBus before 1.0, that will break all three libraries and we'll just have to live with it, but after doing that, we might possibly be able to flatten -core into the main library, and make tp-glib-codegen only depend on GIO. Created attachment 58069 [details] [review] [master 1/2] Update GLib dependency in .pc files to catch up with configure.ac Created attachment 58070 [details] [review] [master 2/2] glib-interfaces-gen: generate self-contained header files Created attachment 58071 [details] [review] [next part 1, 1/2] Stop pretending our API/ABI matches telepathy-glib 0.x To provide parallel-installability, I've changed: - base of library name: libtelepathy-glib.so -> libtelepathy-glib-1.so - header directory: telepathy-1.0 -> telepathy-glib-1 - pkg-config package: telepathy-glib -> telepathy-glib-1 - GIR package: TelepathyGLib-0.12 -> TelepathyGLib-1 Created attachment 58072 [details] [review] [next part 1, 2/2] Remove all old ABIs --- Don't bother reviewing this one tbh. It is literally "rm telepathy-glib/versions/0*.abi". Created attachment 58073 [details] [review] [next 01/10] Split library into main, dbus and core Eventually, the dbus part will contain all the generated code, core will contain enough of TpProxy to support that, and the main part will contain all the high-level API. At the moment, circular dependencies mean that we have to keep almost everything in the main part. The core part contains the error quark for TpError and the generated enums, and the dbus part contains the quarks for interface names. Created attachment 58074 [details] [review] [next 02/10] Move TpSvc* into the dbus library This requires moving tp_dbus_g_method_return_not_implemented and tp_svc_interface_set_dbus_properties_info into the core library, so do that too. Created attachment 58075 [details] [review] [next 03/10] If generating split re-entrant functions, duplicate the collect callback This means we don't have to duplicate the entire -body.h in reentrants.c. --- This yak is freshly shaved. I was experimenting with making the contents of cli-*.h be all inline functions, for which this is probably a prerequisite, in an attempt to break the cyclic dependency between -dbus and main. Created attachment 58076 [details] [review] [next 04/10] Move TpProxy, TpDBusDaemon auto-generated code to a new header Created attachment 58077 [details] [review] [next 05/10] cli-channel: new header for TpChannel's auto-generated client code Created attachment 58078 [details] [review] [next 06/10] Split TpConnection generated API into cli-connection.h A couple of hand-written functions went with it, because they're relatively low-level too. Created attachment 58079 [details] [review] [next 07/10] Move CM, Protocol to cli-misc.h Created attachment 58080 [details] [review] [next 08/10] Move client code for TpChannelDispatcher and its children to cli-misc.h Created attachment 58081 [details] [review] [next 09/10] Move Account, AccountManager, Client generated client code to cli-misc.h Created attachment 58082 [details] [review] [next 10/10] Move Call generated code to cli-call.h --- These were the last tp_cli_* in a "main" header. Branches: fixes-from-next, next-api, split-next in my usual repository. Still to be done: obviously, get the tp-cli-*-body.h into the -dbus library. This is a real pain. Here's a fun dependency chain: - _tp_log() must be in the main lib (I don't really want it to be ABI) - generated code must be in the -dbus lib - it calls TpProxy stuff, which thus needs to be in -core - TpProxy stuff calls DEBUG() - which is _tp_log :-( I'm not very keen on duplicating the debug infrastructure into -core. Here's another: - tp_cli_protocol_do_stuff() calls TP_IS_PROTOCOL - which needs tp_protocol_get_type() - which pulls in all of TpProtocol - which is in the main library because it depends on -dbus One possible way to break this would be to make the client-side call functions all be static inline? Another would be for tp_proxy_class_init() to poke an assortment of function pointers and GTypes into -core, via a function that is an extern symbol but not visible in public headers. (In reply to comment #17) > Branches: fixes-from-next, next-api, split-next in my usual repository. All the patches above look good to me. > - _tp_log() must be in the main lib (I don't really want it to be ABI) > - generated code must be in the -dbus lib > - it calls TpProxy stuff, which thus needs to be in -core > - TpProxy stuff calls DEBUG() > - which is _tp_log :-( Ouch. > One possible way to break this would be to make the client-side call functions > all be static inline? I wouldn't be against this. > Another would be for tp_proxy_class_init() to poke an assortment of function > pointers and GTypes into -core, via a function that is an extern symbol but not > visible in public headers. This scares me... (In reply to comment #18) > (In reply to comment #17) > > Branches: fixes-from-next, next-api, split-next in my usual repository. > > All the patches above look good to me. Applied, thanks. Bug still open until I manage to wrangle tp_cli_* into the -dbus library. After a number of false starts, I have a branch that works. Created attachment 60692 [details] [review] [1/6] Move some TpProxy functionality into the -core library Most of this just calls into a vtable set up by the -main library, but it's enough for the generated code not to have an explicit (circular) dependency on the -main library. Created attachment 60693 [details] [review] [2/6] Export tp_cli_channel_add_signals etc. I don't like exporting more API than we have to, but this is a necessary evil: TpChannel, etc. initialization need to call these functions. Created attachment 60694 [details] [review] [3/6] codegen: inline type-checks into the generated header file We don't want tp_cli_channel_foo() (in the -dbus library) to depend on TP_IS_CHANNEL, which expands to something involving tp_channel_get_type() (in the -main library). So, do the type-check in an inline wrapper around the real function. As a double-check, we should make sure that the proxy is at least a proxy. Every generated function calls tp_proxy_borrow_interface_by_id() early on, so we can use that to host the type-check. Created attachment 60695 [details] [review] [4/6] Move tp_cli_* to -dbus library Created attachment 60696 [details] [review] [5/6] Move dbus-glib GType stuff to -dbus library Created attachment 60697 [details] [review] [6/6] Move Call1 Content/Stream client code to -dbus library Comment on attachment 60692 [details] [review] [1/6] Move some TpProxy functionality into the -core library Review of attachment 60692 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/core-proxy.c @@ +46,5 @@ > + * Returns: %TRUE if it is safe to call dbus_g_proxy_add_signal() > + * Since: 0.7.6 > + */ > +gboolean > +tp_proxy_dbus_g_proxy_claim_for_signal_adding (DBusGProxy *proxy) OOI, why has this been moved to core-proxy? Comment on attachment 60694 [details] [review] [3/6] codegen: inline type-checks into the generated header file Review of attachment 60694 [details] [review]: ----------------------------------------------------------------- ::: tools/glib-client-gen.py @@ +398,5 @@ > self.b('}') > self.b('') > > + # Inline the type-check into the header file, so the object code > + # doesn't depend on tp_channel_get_type() or whatever Nice one! Yeah these patches look good. It took me a while to decypher the patch to the codegen but I think it looks good. It's funny, this all looks quite simple now you've reached a solution but when I was playing with it a while ago it wasn't very clear or simple at all! (In reply to comment #27) > > +tp_proxy_dbus_g_proxy_claim_for_signal_adding (DBusGProxy *proxy) > > OOI, why has this been moved to core-proxy? Generated code calls it (tp_cli_connection_add_signals and similar), and we want that generated code to go in the -dbus library. I could do the same semi-inlining trick, but this entire function is "core-able" (it just pokes at the DBusGProxy's qdata, with no real TpProxy involvement) so there seemed no point. (In reply to comment #29) > It's funny, this all looks quite simple now you've reached a solution but when > I was playing with it a while ago it wasn't very clear or simple at all! Don't feel bad about that, it took me days/weeks of trying wrong solutions before I realised this one was viable :-) (In reply to comment #30) > (In reply to comment #27) > > > +tp_proxy_dbus_g_proxy_claim_for_signal_adding (DBusGProxy *proxy) > > > > OOI, why has this been moved to core-proxy? > > Generated code calls it (tp_cli_connection_add_signals and similar), and we > want that generated code to go in the -dbus library. Ah yes I see. Fair enough. > I could do the same semi-inlining trick, but this entire function is > "core-able" (it just pokes at the DBusGProxy's qdata, with no real TpProxy > involvement) so there seemed no point. Yeah, don't bother. > (In reply to comment #29) > > It's funny, this all looks quite simple now you've reached a solution but when > > I was playing with it a while ago it wasn't very clear or simple at all! > > Don't feel bad about that, it took me days/weeks of trying wrong solutions > before I realised this one was viable :-) Yeah, nice work! Created attachment 60801 [details] [review] Move TpDebugClient generated code to cli-misc.[ch] and don't document it --- All merged, thanks. Here's one more patch to get rid of the last bits of tp_cli from the main library - TpDebugClient was added in master since I started on this. I started to write a separate documentation bit for cli-debug-client but then I realised there was no point, since we're unlikely to add functionality to it, and if we do, it should be in the high-level API. So it's in the -dbus part of the library, but deliberately undocumented. Comment on attachment 60801 [details] [review] Move TpDebugClient generated code to cli-misc.[ch] and don't document it Review of attachment 60801 [details] [review]: ----------------------------------------------------------------- ++ Fixed in git \o/ |
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.