Bug 27274

If we're going to do this, I think it should go further.

It should include a TpProxy subclass: TplLogger. And we should move the #defines for the well-known bus name and object path into one of these headers (currently in dbus-service.h). Also make the #defines the same format as other TP #defines. Something like TPL_LOGGER_BUS_NAME and TPL_LOGGER_OBJECT_PATH.

(In reply to comment #1)
> If we're going to do this, I think it should go further.
> 
> It should include a TpProxy subclass: TplLogger. And we should move the
> #defines for the well-known bus name and object path into one of these headers
> (currently in dbus-service.h). Also make the #defines the same format as other
> TP #defines. Something like TPL_LOGGER_BUS_NAME and TPL_LOGGER_OBJECT_PATH.
> 

I'm not sure this is a huge gain. It'd make the API a little nicer, but I don't think it's terribly compelling. Maybe we could merge what I've got and re-examine this idea after we split the various sections of the D-Bus API proper interfaces?

(In reply to comment #1)
> It should include a TpProxy subclass: TplLogger.

Yes, it should. It's more or less OK for local extensions to paste functionality onto (every) TpProxy, but libraries shouldn't do that.

Also, this would allow adding a TP_ERRORS mapping, which can't be done directly on TpProxy (an artificial restriction I put in to stop people causing collisions by defining two mappings for the same set of D-Bus errors).

How stable is the D-Bus API, compared with the real library? If it's not terrifyingly unstable, then it should just be part of the real library, in the same way that TpChannelDispatchOperation is part of telepathy-glib.

A final advantage of adding a TpProxy subclass is that you can put in the correct logic and documentation for when it should get invalidated (e.g. Channel.Closed has this effect), and whether it needs to be addressed by unique name (stateful API) or not. From context, I suspect that the logger is stateless and never invalidated, like TpConnectionManager, though?

Finally: is this actually as useful as you think? Does the library not already have GAsyncResult-based wrappers for all the D-Bus methods?

(In reply to comment #3)
> (In reply to comment #1)
> > It should include a TpProxy subclass: TplLogger.
> 
> Yes, it should. It's more or less OK for local extensions to paste
> functionality onto (every) TpProxy, but libraries shouldn't do that.

> A final advantage of adding a TpProxy subclass is that you can put in the
> correct logic and documentation for when it should get invalidated (e.g.
> Channel.Closed has this effect), and whether it needs to be addressed by unique
> name (stateful API) or not. From context, I suspect that the logger is
> stateless and never invalidated, like TpConnectionManager, though?

The API is stateless.

> Finally: is this actually as useful as you think? Does the library not already
> have GAsyncResult-based wrappers for all the D-Bus methods?

Yes, these wrappers exist and they work reasonably well (I'm using them in another project).

> > Finally: is this actually as useful as you think? Does the library not already
> > have GAsyncResult-based wrappers for all the D-Bus methods?
> 
> Yes, these wrappers exist and they work reasonably well (I'm using them in
> another project).

So the problem here is that libtelepathy-logger contains GPL code (bunch of GPL code from Empathy), so comes with all of those problems.

Also, we don't want to do read/write type things from the direct IO lib, e.g. favourites. IMO the direct IO lib is just for mass-reading log files.

(In reply to comment #5)
> > > Finally: is this actually as useful as you think? Does the library not already
> > > have GAsyncResult-based wrappers for all the D-Bus methods?
> > 
> > Yes, these wrappers exist and they work reasonably well (I'm using them in
> > another project).
> 
> So the problem here is that libtelepathy-logger contains GPL code (bunch of GPL
> code from Empathy), so comes with all of those problems.
> 
> Also, we don't want to do read/write type things from the direct IO lib, e.g.
> favourites. IMO the direct IO lib is just for mass-reading log files.
> 

Oh, sorry, I was referring to the library this branch exposes (libtpl-extensions).

(In reply to comment #5)
> So the problem here is that libtelepathy-logger contains GPL code (bunch of GPL
> code from Empathy), so comes with all of those problems.

*cries*

So we have convenience code that wraps these D-Bus calls in C API, written specifically for this project by Collabora and usable under a license of our choice, but we can't use them from certain clients of the logger, because something else in libtelepathy-logger is GPL?

The best way out of this might be to have a small libtelepathy-logger with the (LGPL && client-relevant) API, a larger libtelepathy-logger-gpl (dependent on lt-l) with the (GPL && client-relevant) API, and the daemon linking both and adding daemon-only stuff?

> Also, we don't want to do read/write type things from the direct IO lib, e.g.
> favourites.

I think you're conflating use of libtelepathy-logger with use of direct I/O into the log stores. These aren't necessarily the same thing.

I've pushed a change which splits the extensions library into an 'inner' part, which is always built and statically linked into libtelepathy-logger, and an 'outer' part (libtpl-extensions), which is only built and installed when the project is configured with --enable-public-extensions.

> So we have convenience code that wraps these D-Bus calls in C API, written
> specifically for this project by Collabora and usable under a license of our
> choice, but we can't use them from certain clients of the logger, because
> something else in libtelepathy-logger is GPL?

> The best way out of this might be to have a small libtelepathy-logger with the
> (LGPL && client-relevant) API, a larger libtelepathy-logger-gpl (dependent on
> lt-l) with the (GPL && client-relevant) API, and the daemon linking both and
> adding daemon-only stuff?

That's more or less what there is with having the GLib D-Bus API binding in one lib, and the rest in another. The GPL bits include LogManager, LogStoreEmpathy and the datetime functions. All of which are required to use the direct IO API.

The GPL code in question comes from Empathy, so maybe it would be possible to get those bits relicensed.

(In reply to comment #9)
> The GPL code in question comes from Empathy, so maybe it would be possible to
> get those bits relicensed.

Could you list thos empathy files please?

(In reply to comment #10)
> (In reply to comment #9)
> > The GPL code in question comes from Empathy, so maybe it would be possible to
> > get those bits relicensed.
> 
> Could you list thos empathy files please?

log-manager (+ log-manager-priv)
log-store
log-store-sqlite (that's a mistake, we wrote all of this and it should be LGPL)
log-store-xml (formerly log-store-empathy)
datetime

Have you tested this in both modes, and checked what gets installed, and what goes into a "make dist" tarball? I infer that you haven't, because afaics, extensions.h will get installed even if the library isn't, and some of the generated stuff incorrectly goes in the tarball.

> +libtpl_extensions_convenience_ladir = $(tplincludedir)

This is, at best, weird... the prefix for foo_HEADERS is normally something like "tplinclude", not a library name. Saying "libtpl_extensions_convenience_la_HEADERS" doesn't cause those headers to be related to libtpl-extensions-convenience.la.

> +libtpl_extensions_convenience_la_HEADERS = \

Please call this something that isn't special to Automake, like "extension_headers", and then do:

libtpl_extensions_convenience_la_SOURCES = \
       ...
       $(extension_headers) \
       $(NULL)

to get them distributed in the tarball. (You could also reference them in EXTRA_DIST, but I think treating them like sources - which are automatically distributed - is nicer.)

Inside the conditional for the public ext.library, also do:

tplinclude_HEADERS += $(extension_headers)

to get them installed (conditionally!).

> -libtpl_extensions_la_SOURCES = \
> +libtpl_extensions_convenience_la_SOURCES = \
>      extensions.c \
>      extensions-cli.c \
>      $(nodist_libtpl_extensions_internal_la_SOURCES) \
>      $(NULL)

nodist sources shouldn't be included in a normal SOURCES list; that defeats the point of having them be nodist-prefixed. Rename the variable to say "convenience" rather than "internal" and remove this reference to it, and they'll automatically be included in the link, but not distributed in the tarball, as desired.

>  21 nodist_geninclude_HEADERS = \

If these aren't needed by telepathy-logger itself (and I rather hope they're not), the variable should be called "gen_headers" or something. It should be included by reference in $(nodist_libtpl_extensions_internal_la_SOURCES) (much like extension_headers above, but as a nodist flavour).

Inside the conditional for libtpl-extensions.la, you should do something like:

geninclude_HEADERS += $(gen_headers)

(again, much like extension_headers).

(In reply to comment #12)
> Have you tested this in both modes, and checked what gets installed, and what
> goes into a "make dist" tarball? I infer that you haven't, because afaics,
> extensions.h will get installed even if the library isn't, and some of the
> generated stuff incorrectly goes in the tarball.

I was careful to check what got installed when I built with and without --enable-public-extensions, though I mainly checked whether client libaries would cleanly build against it if installed and cleanly detect it wasn't installed (eg, based on the .pc file) as necessary. I guess I missed that some headers were installed in the disabled state.

I completely missed on the "make dist" point, though. Thanks for pointing it out.


I've applied your suggestions (and rebased against head) in my (slightly misnamed) ext-library-merged branch. This seems to work well for all cases, including building from the resulting "make dist" tarball.

The only issue is that, when building from the "make dist" tarball, without external library support, ${includedir}/tpl-extensions/_gen gets installed, though without any headers in it or its parent dir. For whatever reason, this doesn't get installed if I build straight from the git tree.

Anything else that needs to happen before we can merge this? (As far as I can see, any licensing issues with libtelepathy-logger shouldn't block the specific changes in this bug, right?)

> +extensions/tpl-extensions.pc
> +extensions/_gen/
> +extensions/doc/
> +tests/test-*[^ch]
> +tests/twisted/telepathy-logger-debug
> +tests/twisted/tools/exec-with-log.sh
> +tests/twisted/tools/org.freedesktop.Telepathy.Client.Logger.service
> +tests/twisted/tools/tmp-session-bus.conf

Please anchor .gitignore paths at the repository root where possible, by prefixing "/". Please keep .gitignore sorted approximately ASCIIbetically (i.e. certainly tests/ below src/).

(/foo/bar matches only foo/bar in the root of the repository; foo/bar would also match doc/foo/bar, for example.)

> +nodist_geninclude_HEADERS = \
>      _gen/signals-marshal.h \

You shouldn't need to install this: nothing that's installed should have to include it. As such, it should move to nodist_blahblah_SOURCES.

> +    _gen/register-dbus-glib-marshallers-body.h \

The same reasoning applies to anything-body.h, which should be in SOURCES (they're essentially a generated .c, but aren't self-contained - they're designed to be #include'd into something that has first included the required headers).

(After the whole branch is taken into account, this essentially means moving some things from gen_headers into SOURCES.)

(In reply to comment #13)
> The only issue is that, when building from the "make dist" tarball, without
> external library support, ${includedir}/tpl-extensions/_gen gets installed,
> though without any headers in it or its parent dir. For whatever reason, this
> doesn't get installed if I build straight from the git tree.

If you can't avoid this, it's harmless. I'd prefer it if you could avoid it: try moving these two lines inside the conditional, perhaps?

> tplincludedir=$(includedir)/tpl-extensions
> genincludedir=$(tplincludedir)/_gen

> Anything else that needs to happen before we can merge this? (As far as I can
> see, any licensing issues with libtelepathy-logger shouldn't block the specific
> changes in this bug, right?)

No, obtaining relicensing from GPL to LGPL is not a blocker for this bug.

(In reply to comment #14)

Thanks for these pointers - I've applied them all in a few commits to my ext-library-merged branch.

Any other concerns before pushing this to mainline?

Looks good, only one remaining complaint:

> +    _gen/register-dbus-glib-marshallers-body.h \

Shouldn't be installed either. Please move that, and consider it to have been reviewed in advance :-)

> Don't create an empty /tpl-extensions/_gen when we build without tpl-extensions support.

Did this work as you'd hoped? If it did, it's worth remembering for the future.

(In reply to comment #16)
> Looks good, only one remaining complaint:
> 
> > +    _gen/register-dbus-glib-marshallers-body.h \
> 
> Shouldn't be installed either. Please move that, and consider it to have been
> reviewed in advance :-)

Whoops. Fixed and merged.

> > Don't create an empty /tpl-extensions/_gen when we build without tpl-extensions support.
> 
> Did this work as you'd hoped? If it did, it's worth remembering for the future.

Yup, it worked fine. I'll try to remember this (and all the other autofoo stuff in this bug), but as you might have guessed, my mind has a habit of blocking anything related to autotools.