Bug 28782

telepathy-glib uses dbus-glib extensively, and has it as part of its API, so this is not a short-term goal; telepathy-glib will need porting first, which will break compatibility.

This also blocks bgo#630755

Long-term, yes we need to migrate to GDBus. At this stage I'm not sure whether this will happen in 0.13.x, or whether to do one more stable branch with dbus-glib then switch to GDBus immediately after.

I'll clone bugs for the migration path.

(In reply to comment #3)
> Long-term, yes we need to migrate to GDBus. At this stage I'm not sure whether
> this will happen in 0.13.x, or whether to do one more stable branch with
> dbus-glib then switch to GDBus immediately after.
> 
> I'll clone bugs for the migration path.

0.13.x has come and gone - any plans to do this in 0.15.x?

Minor Folks bugs bgo#630755 and bgo#653198 depend on this bug.

I've been playing a bit with gdbus-codegenn reported some issues:

https://bugzilla.gnome.org/show_bug.cgi?id=659646 - generated code does not build on our strict gcc flags. Already made a patch.

https://bugzilla.gnome.org/show_bug.cgi?id=659682 - namespace of generate C code does not work well with our interface naming.

https://bugzilla.gnome.org/show_bug.cgi?id=659690 - another build warning in generated code

https://bugzilla.gnome.org/show_bug.cgi?id=659699 - more serious property name collision for our interface "Connection".

After all those bugs have been fixed, here is a tp-glib branch generating GDBus code for the whole spec, and use it in TpConnection to get the self handle, etc. That's just a demo to see if the codegen works with our spec.

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=gdbus

Hello, Any plans to do this in 0.17.x ?

(In reply to comment #8)
> Hello, Any plans to do this in 0.17.x ?

No, 0.17.x remains compatible with earlier branches.

We're breaking ABI for Telepathy 1.0 (the 'next' branch in git); the plan is for the corresponding telepathy-glib version to also be 1.0. Depending how long it takes, we'll either move to GDBus in that branch, or carry on using dbus-glib and break ABI again later to get rid of it.

Our use of dbus-glib is very pervasive, so this isn't like porting (say) GConf to GDBus.

I have a "branch" (one giant commit, which I'll break up later) that completely moves telepathy-glib to GDBus.

It doesn't *work* yet - it compiles, but more than half the tests fail. It's getting there, though.

Here is some work-in-progress, which depends on Bug #76120. The telepathy-glib tests now pass, but several telepathy-logger tests fail (I haven't debugged those yet).

Next steps: make the logger tests pass; try porting a connection manager, probably Gabble.

Mission Control is (as always) going to be the most difficult bit.

http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=wip-gdbus3

Happily, this is already a win in terms of amount of code, even though we're still kludging everything via TpSvcSomething/tp_cli_something and aren't really using GDBus "properly" yet:

 144 files changed, 2400 insertions(+), 3918 deletions(-)

(not counting gdbus-prep3 which adds 141 LoC more than it deletes)

I've read the general idea and I have to say, it's brilliant ! I will try to find time to do an in-deph review asap.

I just didn't understand how property getter/setter works service side. Generated code set NULL for get/set_property in GDBusInterfaceVTable and method_call does handle Get/GetAll/Set methods. I'm missing something.

All those GVariant conversions are sad, but that will be the 2nd step.

(In reply to comment #13)
> I just didn't understand how property getter/setter works service side.

I still implement each method of o.fd.DBus.Properties "the hard way" with TpDBusPropertiesMixin, just like we did for dbus-glib.

For dbus-glib, we did that instead of using dbus-glib's own facilities because dbus-glib incorrectly conflated GObject-level "access control" with D-Bus-level "access control", which would have led to absurdities like being able to change a channel's TargetHandle from D-Bus just because that GObject property was marked READWRITE (and CONSTRUCT_ONLY, but dbus-glib didn't respect that).

I didn't realize when I worked around it, but that was actually an exploitable security bug in certain system services, CVE-2010-1172.

For GDBus, it would be nice to be able to switch away from implementing o.fd.DBus.Properties ourselves, but I haven't implemented it yet. I'd like to make the Logger tests work, and port at least one connection manager, before thinking too hard about what the "better than TpSvc" API should look like.

(In reply to comment #14)
> I still implement each method of o.fd.DBus.Properties "the hard way" with
> TpDBusPropertiesMixin, just like we did for dbus-glib.

Expanding on that a little: this takes precedence over GDBus' own implementation, just like it took precedence over dbus-glib's own implementation.

The pseudocode is now something like this:

* incoming method call: o.fd.DBus.Properties.$method($iface, ...)
  where $method in { Get, Set, GetAll }
* does its object path have a GDBusInterfaceVTable for o.fd.DBus.Properties?
  - yes: call $method using that vtable; stop [*]
  - no: continue
* does it have a GDBusInterfaceVTable for $iface?
  - yes: continue
  - no: raise an error; stop
* does it have a property getter or setter in the GDBusInterfaceVTable
  for $iface?
  - yes: call it; stop
  - no: continue
* try calling the method_call function in the GDBusInterfaceVTable for $iface,
  with interface_name="org.freedesktop.DBus.Properties" (*not* $iface)
  and method_name=$method

We currently do the line marked [*].

ok thanks, didn't know it worked like that. Good enough for now, I agree.

http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=wip-gdbus4 is a simple rebase onto a revised Bug #76120, plus an extra #include <dbus/dbus-glib.h> in the last commit (I hadn't committed it when I stopped on Friday).

The contacts test very occasionally fails with:

ERROR:/home/smcv/src/fdo/tpglib-next/tests/dbus/contacts.c:1566:assert_subscription_states: assertion failed (tp_contact_get_subscribe_state (contact) == states->subscribe): (4 == 3)

I don't know why, and I can't reproduce it reliably. I don't *think* I've seen it do that under dbus-glib.

(In reply to comment #11)
> Next steps: make the logger tests pass

5/7 pass immediately when I wrap them in tp_tests_run_with_bus().

The remaining 2/7 (log-store-xml and log-store-pidgin) both appear to have the same bug: they carry over some objects between test cases, resulting in the same sort of failure modes as account-request on Bug #76120. I suspect refleaks, but so far, I haven't found them.

Salut tests pass!

<http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=next-misc>
<http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus>
<http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=wip-gdbus>

(In reply to comment #19)
> Next steps: make the logger tests pass

They do now; the next-misc branch "fixes" them by being careful how we use GTestDBus.

Eventually, it would be nice to use one GTestDBus per setup()/teardown() pair, but that'll need some careful changes to the various singleton objects, and I think that can wait.

> <http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=next-misc>

All merged.

> <http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus>

Rebased onto the next-api branch from Bug #76462, which is itself based on the next-tap branch from Bug #74626.

<http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus2>

A couple of extra commits on top of that, to fix the MC tests.

<http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=next-gdbus>

Ported MC to GDBus. Its tests pass now.

Next things: making the Gabble tests pass; checking that the Salut tests still pass; converting Haze, Idle and Rakia; converting Folks (if necessary); converting Empathy.

 - You can remove tp_proxy_get_interface_by_id() completely, it's not used any more. It creates the proxy with sync call so we shouldn't use it in the future neither but rather use gdbus-codegen to create GDBusProxy subclasses.

 - tp_svc_interface_set_dbus_interface_info(): doc says there is a vtable arg, but there isn't.

 - "tp_dbus_daemon_register_object: rewrite in terms of TpSvcInterfaceSkeleton":
   1) I think GSList's only purpose is making your app crash if you use g_list_free on it. I see no other reason anyone will ever use it. We are not going to export millions of objects, so a GList is perfectly good.
   2) I would add a comment in tp_dbus_daemon_unregister_object() telling that setting the qdata to NULL will call tp_dbus_daemon_registration_free() that unexport stuff.
   3) g_object_replace_qdata() seems complex, had to read that if condition 3 times. I would just have classic r=g_object_get_qdata(); if(r!=NULL){...; return;} g_object_set_qdata(); We are not threadsafe so atomic operation isn't needed and I don't think the perf gain it worth it.

 - "tp_dbus_daemon_dup: rewrite in terms of GDBus, and use session bus": dbus_g_bus_get() was doing blocking operation as well? Probably not a big deal, but I'm wondering what would be our API if we want to be fully async.

 - "tp_dbus_daemon_watch_name_owner: rewrite in terms of g_bus_watch_name_on_conn...": I think it does have a behaviour difference: g_dbus_watch_name() will call vanished or appeared for the initial status, not only when it changes later. Also you add in the doc "New code should use g_bus_watch_name() or similar instead." that sounds like deprecated function, do you consider 1.0-blocker to actually remove that function?

 - "tp_dbus_daemon_request_name: rewrite using GDBus": Hm, it still uses dbus-glib's enum for the reply. Maybe we want to write our own if GDBus does not have helper for that. Maybe open a glib bug to add the helper?

 - "tp_dbus_daemon_release_name: rewrite using GDBus": same.

 - "tp_run_connection_manager: use GDBus to watch for disconnection": commit msg has a FIXME. You didn't add the g_dbus_connection_set_exit_on_close() and I think it's fine for the CM-side.

 - "TpProxy: rewrite async and reentrant calls": We still have reentrant calls in 1.0? I would consider blocker to remove them, or hand-write the few that we really want and drop the codegen for them, using g_dbus_connection_call_sync() probably?


I think that's all. I reviewed more the generated code than the generator tbh. The branch is huge and complex, so I probably overlooked a few stuff, but we can fix stuff after the merge anyway :)

So I know: what's the last commit you reviewed? Did you get as far as 03a5f5d4 "TpProxy: strip detailed error name..." on the gdbus2 branch, which is my latest?

(In reply to comment #23)
>  - You can remove tp_proxy_get_interface_by_id() completely, it's not used
> any more.

Yeah, I did think about that, but it isn't critical-path. I want to get the whole stack ported first.

[ACTION smcv] Do this later

>  - tp_svc_interface_set_dbus_interface_info(): doc says there is a vtable
> arg, but there isn't.

Fixed in af9d506f, I think.

[ACTION xclaesse] Please confirm

>  - "tp_dbus_daemon_register_object: rewrite in terms of
> TpSvcInterfaceSkeleton":
>    1) I think GSList's only purpose is making your app crash if you use
> g_list_free on it. I see no other reason anyone will ever use it. We are not
> going to export millions of objects, so a GList is perfectly good.

Fair enough, we can use a GSList (but whole-stack porting > this).

[ACTION smcv] Do this later

>    2) I would add a comment in tp_dbus_daemon_unregister_object() telling
> that setting the qdata to NULL will call tp_dbus_daemon_registration_free()
> that unexport stuff.

Reasonable.

[ACTION smcv] Do this later

>    3) g_object_replace_qdata() seems complex, had to read that if condition
> 3 times. I would just have classic r=g_object_get_qdata(); if(r!=NULL){...;
> return;} g_object_set_qdata(); We are not threadsafe so atomic operation
> isn't needed and I don't think the perf gain it worth it.

Now that we're setting fire to dbus-glib, there is at least the possibility that we can become thread-safe in future.

I understand your clarity concerns, though. I could go either way.

Would a better comment help?

[ACTION smcv] Write a better comment, or split it, later

>  - "tp_dbus_daemon_dup: rewrite in terms of GDBus, and use session bus":
> dbus_g_bus_get() was doing blocking operation as well? Probably not a big
> deal, but I'm wondering what would be our API if we want to be fully async.

Yes, the underlying dbus_connection_open() blocks, because writing async API in libdbus (without even GMainContext, let alone something like GAsyncResult) is pretty horrible.

Note that g_bus_get_sync() returns immediately if you already have the relevant singleton, so one easy API for fully-async is:

    g_bus_get (...);

    ...later, in the callback...

    g_bus_get_finish (...);
    if (error)
      {
        ... handle it ...;
        return;
      }
    my_dbus_daemon = tp_dbus_daemon_dup (NULL); /* can't fail now */

When we've swapped the hierarchy around so that TpClientFactory is at the top, we could add a tp_client_factory_get_async(), if you really want to.

[ACTION xclaesse] Will this do for now?

>  - "tp_dbus_daemon_watch_name_owner: rewrite in terms of
> g_bus_watch_name_on_conn...": I think it does have a behaviour difference:
> g_dbus_watch_name() will call vanished or appeared for the initial status,
> not only when it changes later.

So does the old tp_dbus_daemon_watch_name_owner() - they are both implemented as a combination of GetNameOwner() and NameOwnerChanged. I removed the explicit call to GetNameOwner(), relying on the implicit call done by g_dbus_watch_name().

[ACTION xclaesse] Please re-read and confirm that it's OK

> Also you add in the doc "New code should use
> g_bus_watch_name() or similar instead." that sounds like deprecated
> function, do you consider 1.0-blocker to actually remove that function?

No, I don't think I want to put "remove all uses of this function" on the to-do list (and that's why I didn't formally deprecate it). We can have a closer look at how much it's called after I get all the CMs, MC, Empathy and Folks ported, but for now, my plan is "deprecate in 1.2, don't remove in the forseeable future".

One possibility would be to internalize it, if MC turns out to be the only caller.

[ACTION xclaesse] Is my plan OK?

[ACTION smcv] Later, do a git grep for it to assess feasibility of making it private

>  - "tp_dbus_daemon_request_name: rewrite using GDBus": Hm, it still uses
> dbus-glib's enum for the reply.

Actually libdbus' enum. I don't consider this to be a big problem: the numbers are fixed by the D-Bus Specification, and if/when we finally get rid of libdbus, we can just have our own #define.

[ACTION xclaesse] is that OK?

> Maybe we want to write our own if GDBus does
> not have helper for that. Maybe open a glib bug to add the helper?

It blocks. I don't think GDBus wants that. It's also really quite simple.

I don't want to put removal of this function on the critical path :-)

[ACTION xclaesse] is that OK?

>  - "tp_run_connection_manager: use GDBus to watch for disconnection": commit
> msg has a FIXME. You didn't add the g_dbus_connection_set_exit_on_close()
> and I think it's fine for the CM-side.

Thanks for the reminder. I'm vaguely inclined to reinstate turning off exit-on-close to preserve existing behaviour, and so CMs can be valgrinded (valground?).

On one hand I want to get a minimum viable port of the whole stack working first; on the other hand, getting closer to how it used to work is likely to make that go quicker.

[ACTION smcv] reinstate existing behaviour

>  - "TpProxy: rewrite async and reentrant calls": We still have reentrant
> calls in 1.0?

We have them in regression tests. They're private.

>  I would consider blocker to remove them, or hand-write the few
> that we really want and drop the codegen for them, using
> g_dbus_connection_call_sync() probably?

They're only in regression tests, where there are are a lot of them. I don't want things that don't affect our API to be on the critical path.

[ACTION xclaesse] Is that OK?

(In reply to comment #24)
> So I know: what's the last commit you reviewed? Did you get as far as
> 03a5f5d4 "TpProxy: strip detailed error name..." on the gdbus2 branch, which
> is my latest?

I reviewed smcv/gdbus at commit 902e566ba9f566c8015d6d9aebc54cee2dceaf6b

> (In reply to comment #23)
> >  - You can remove tp_proxy_get_interface_by_id() completely, it's not used
> > any more.
> 
> Yeah, I did think about that, but it isn't critical-path. I want to get the
> whole stack ported first.
> 
> [ACTION smcv] Do this later

Ok.

> >  - tp_svc_interface_set_dbus_interface_info(): doc says there is a vtable
> > arg, but there isn't.
> 
> Fixed in af9d506f, I think.
> 
> [ACTION xclaesse] Please confirm

Right.

> >  - "tp_dbus_daemon_register_object: rewrite in terms of
> > TpSvcInterfaceSkeleton":
> >    1) I think GSList's only purpose is making your app crash if you use
> > g_list_free on it. I see no other reason anyone will ever use it. We are not
> > going to export millions of objects, so a GList is perfectly good.
> 
> Fair enough, we can use a GSList (but whole-stack porting > this).
> 
> [ACTION smcv] Do this later

I didn't know we were using GSList in other places. Opening a bug is good enough for now.

> >    2) I would add a comment in tp_dbus_daemon_unregister_object() telling
> > that setting the qdata to NULL will call tp_dbus_daemon_registration_free()
> > that unexport stuff.
> 
> Reasonable.
> 
> [ACTION smcv] Do this later

Ok

> >    3) g_object_replace_qdata() seems complex, had to read that if condition
> > 3 times. I would just have classic r=g_object_get_qdata(); if(r!=NULL){...;
> > return;} g_object_set_qdata(); We are not threadsafe so atomic operation
> > isn't needed and I don't think the perf gain it worth it.
> 
> Now that we're setting fire to dbus-glib, there is at least the possibility
> that we can become thread-safe in future.
> 
> I understand your clarity concerns, though. I could go either way.
> 
> Would a better comment help?
> 
> [ACTION smcv] Write a better comment, or split it, later

Actually you commented it well already, maybe I'm just not familiar enough with compare-and-exchange syntax because I don't do threading a lot. It just felt uselesly complex to me because we are miles away at being thread safe anyway. But I'm ok to keep it like that if you feel it's valuable :)

> >  - "tp_dbus_daemon_dup: rewrite in terms of GDBus, and use session bus":
> > dbus_g_bus_get() was doing blocking operation as well? Probably not a big
> > deal, but I'm wondering what would be our API if we want to be fully async.
> 
> Yes, the underlying dbus_connection_open() blocks, because writing async API
> in libdbus (without even GMainContext, let alone something like
> GAsyncResult) is pretty horrible.
> 
> Note that g_bus_get_sync() returns immediately if you already have the
> relevant singleton, so one easy API for fully-async is:
> 
>     g_bus_get (...);
> 
>     ...later, in the callback...
> 
>     g_bus_get_finish (...);
>     if (error)
>       {
>         ... handle it ...;
>         return;
>       }
>     my_dbus_daemon = tp_dbus_daemon_dup (NULL); /* can't fail now */
> 
> When we've swapped the hierarchy around so that TpClientFactory is at the
> top, we could add a tp_client_factory_get_async(), if you really want to.
> 
> [ACTION xclaesse] Will this do for now?

Good enough. In GNOME world you would use GApplication and get the GDBusConnection from there.

> >  - "tp_dbus_daemon_watch_name_owner: rewrite in terms of
> > g_bus_watch_name_on_conn...": I think it does have a behaviour difference:
> > g_dbus_watch_name() will call vanished or appeared for the initial status,
> > not only when it changes later.
> 
> So does the old tp_dbus_daemon_watch_name_owner() - they are both
> implemented as a combination of GetNameOwner() and NameOwnerChanged. I
> removed the explicit call to GetNameOwner(), relying on the implicit call
> done by g_dbus_watch_name().
> 
> [ACTION xclaesse] Please re-read and confirm that it's OK

Oh right, previous code was notifying about initial state as well. I'm fine then.

> > Also you add in the doc "New code should use
> > g_bus_watch_name() or similar instead." that sounds like deprecated
> > function, do you consider 1.0-blocker to actually remove that function?
> 
> No, I don't think I want to put "remove all uses of this function" on the
> to-do list (and that's why I didn't formally deprecate it). We can have a
> closer look at how much it's called after I get all the CMs, MC, Empathy and
> Folks ported, but for now, my plan is "deprecate in 1.2, don't remove in the
> forseeable future".
> 
> One possibility would be to internalize it, if MC turns out to be the only
> caller.
> 
> [ACTION xclaesse] Is my plan OK?

> [ACTION smcv] Later, do a git grep for it to assess feasibility of making it
> private

Ok, please internalize if possible, otherwise we'll deprecated it post-1.0.

> >  - "tp_dbus_daemon_request_name: rewrite using GDBus": Hm, it still uses
> > dbus-glib's enum for the reply.
> 
> Actually libdbus' enum. I don't consider this to be a big problem: the
> numbers are fixed by the D-Bus Specification, and if/when we finally get rid
> of libdbus, we can just have our own #define.
> 
> [ACTION xclaesse] is that OK?

Ok, fine while we are still depending on libdbus anyway.

> > Maybe we want to write our own if GDBus does
> > not have helper for that. Maybe open a glib bug to add the helper?
> 
> It blocks. I don't think GDBus wants that. It's also really quite simple.

fair enough.

> I don't want to put removal of this function on the critical path :-)
> [ACTION xclaesse] is that OK?

sure.

> >  - "tp_run_connection_manager: use GDBus to watch for disconnection": commit
> > msg has a FIXME. You didn't add the g_dbus_connection_set_exit_on_close()
> > and I think it's fine for the CM-side.
> 
> Thanks for the reminder. I'm vaguely inclined to reinstate turning off
> exit-on-close to preserve existing behaviour, and so CMs can be valgrinded
> (valground?).

Why would valgrind close the connection? I don't understand why it would affect it. Note that g_test_dbus_down() will disable exit-on-close before stopping the test dbus-daemon.

> On one hand I want to get a minimum viable port of the whole stack working
> first; on the other hand, getting closer to how it used to work is likely to
> make that go quicker.
> [ACTION smcv] reinstate existing behaviour

Thinking about it, if dbus-daemon crash the CM probably want to first close the server connection before leaving. I don't know if our CMs does that, but could be safer to reinstate that, yes.

> >  - "TpProxy: rewrite async and reentrant calls": We still have reentrant
> > calls in 1.0?
> 
> We have them in regression tests. They're private.
> 
> >  I would consider blocker to remove them, or hand-write the few
> > that we really want and drop the codegen for them, using
> > g_dbus_connection_call_sync() probably?
> 
> They're only in regression tests, where there are are a lot of them. I don't
> want things that don't affect our API to be on the critical path.
> 
> [ACTION xclaesse] Is that OK?

Ok if it's only private usage. Would be nice to have gio-like _sync() methods instead of spinning our mainloop, but it doesn't matter for our unit tests I guess.


Please open bugs for all the "later" points, or keep this bug open to handle them, but I'm fine with merging that branch as-is already. Good job!

Reviewing the extra 2 commits in gdbus2 branch:

 -"name owner watching: more debug, and use a free-function to free watches": I don't understand how that can fix a bug. For me the only explanation would be that MC calls that from different threads. Or am I missing something?

But I like it anyway. You can just simplify dispose a lot, it's just a g_clear_pointer(&self->priv->name_owner_watches, g_hash_table_unref); now.

(In reply to comment #25)
> > Thanks for the reminder. I'm vaguely inclined to reinstate turning off
> > exit-on-close to preserve existing behaviour, and so CMs can be valgrinded
> > (valground?).
> 
> Why would valgrind close the connection? I don't understand why it would
> affect it. Note that g_test_dbus_down() will disable exit-on-close before
> stopping the test dbus-daemon.

It's not that; the issue is that if you're using valgrind to spot memory leaks, it's better to terminate the GMainLoop, unref everything and exit gracefully, rather than committing suicide with _exit(0) (libdbus) or raise(SIGTERM) (GDBus) which leaves the last known state of lots of objects as "reachable".

> > On one hand I want to get a minimum viable port of the whole stack working
> > first; on the other hand, getting closer to how it used to work is likely to
> > make that go quicker.
> > [ACTION smcv] reinstate existing behaviour
> 
> Thinking about it, if dbus-daemon crash the CM probably want to first close
> the server connection before leaving. I don't know if our CMs does that, but
> could be safer to reinstate that, yes.

I don't think they do, but the kernel will close the TCP sockets for them, which should be enough in most protocols.

> Please open bugs for all the "later" points, or keep this bug open to handle
> them, but I'm fine with merging that branch as-is already. Good job!

I'm going to hold off on this until I have at least the CMs, and preferably Folks, passing tests. My gdbus3 branch has a rebase onto next-tap and next-abi, plus one more commit, which turned out to be necessary for Rakia.

(In reply to comment #26)
>  -"name owner watching: more debug, and use a free-function to free
> watches": I don't understand how that can fix a bug. For me the only
> explanation would be that MC calls that from different threads. Or am I
> missing something?

I'm not sure why this was wrong myself, which worries me a bit; but the new version is definitely nicer.

I might come back to this, and have another go at working out how it can go wrong.

> But I like it anyway. You can just simplify dispose a lot, it's just a
> g_clear_pointer(&self->priv->name_owner_watches, g_hash_table_unref); now.

[ACTION smcv]: do this later

It looks as though tp_dbus_daemon_watch_name_owner() is called by telepathy-glib (4x, + its own regression test, + 1x in an unrelated regression test) and MC (3x), so porting to g_bus_watch_name_on_connection looks entirely feasible.

(No other search hits in Empathy, Folks, the core CMs, or codesearch.debian.net.)

(In reply to comment #27)
> I'm going to hold off on this until I have at least the CMs, and preferably
> Folks, passing tests. My gdbus3 branch has a rebase onto next-tap and
> next-abi, plus one more commit, which turned out to be necessary for Rakia.

The daemons pass tests! (Except that Gabble's client-types.py sometimes fails when run as part of the large "make check", but passes when run individually; possibly fixed by my 4-month-old patch on Bug #37381, I haven't tried.)

http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus3
http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=next-gdbus
http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=next-gdbus
http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=next-gdbus
http://cgit.freedesktop.org/~smcv/telepathy-haze/log/?h=next-gdbus
http://cgit.freedesktop.org/~smcv/telepathy-idle/log/?h=next-gdbus
http://cgit.freedesktop.org/~smcv/telepathy-rakia/log/?h=next-gdbus

Next: Folks.

(In reply to comment #28)
> I'm not sure why this was wrong myself, which worries me a bit

I think I'll remove this logic altogether, which neatly solves the problem :-)

(In reply to comment #31)
> (In reply to comment #28)
> > I'm not sure why this was wrong myself, which worries me a bit
> 
> I think I'll remove this logic altogether, which neatly solves the problem
> :-)

I already have a branch that does it: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factory

You can ignore the big TMP commit, but the rest can be reviewed already.

(In reply to comment #32)
> I already have a branch that does it:
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factory

That overlaps with stuff I've done... I'll have a look, and take your implementations unless mine look significantly better (or simpler).

I've been concentrating on trying to get a whole pre-GDBus stack (tp-glib,  Empathy) into a testable state, so that we know whether "it stops working with GDBus" is a regression. Unfortunately, it seems that nobody has tested this stuff as a whole stack, because I'm still finding bugs like "the Mission Control .pc files are wrong".

If nobody is testing Empathy with Telepathy 1.0 anyway, then I suppose I might as well land GDBus...

(In reply to comment #27)
> My gdbus3 branch has a rebase onto next-tap and
> next-abi, plus one more commit, which turned out to be necessary for Rakia.

Are you OK with merging "Debug sender: avoid using codegen for signal emission" too?

Yes, please merge your branch. We are missing full-stack testing, and that's why we should release 1.0 early in the GNOME cycle. With your DD hat on, it would be nice if you can make debian packages, I could push them into an ubuntu PPA. That's the best way to get testers.

I'm also wondering if we should merge into master after branching a last stable branch everywhere. We already stopped backporting the stuff we do in next to master...

== tp-glib ==

(In reply to comment #33)
> That overlaps with stuff I've done... I'll have a look, and take your
> implementations unless mine look significantly better (or simpler).

Your implementations were nicer. I merged them, then pushed more changes which do what you asked me to do:

http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=next-after-gdbus

== MC ==

MC branch updated to cope with the removal of o.fd.DBus convenience APIs:

http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=next-gdbus

== CMs ==

> http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=next-gdbus
> http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=next-gdbus
> http://cgit.freedesktop.org/~smcv/telepathy-haze/log/?h=next-gdbus
> http://cgit.freedesktop.org/~smcv/telepathy-idle/log/?h=next-gdbus
> http://cgit.freedesktop.org/~smcv/telepathy-rakia/log/?h=next-gdbus

Those still pass tests. I rebased the Gabble branch onto Bug #76690 since that's what I've been using for smoke-testing.

== Clients ==

Folks and Empathy pass tests with their respective 'next-gdbus' branches, but I still need to smoke-test them, and the 'next' branches are also unreviewed:

http://cgit.freedesktop.org/~smcv/folks/log/?h=next-gdbus
http://cgit.freedesktop.org/~smcv/empathy/log/?h=next-gdbus

(In reply to comment #35)
> With your DD hat on, it
> would be nice if you can make debian packages, I could push them into an
> ubuntu PPA. That's the best way to get testers.

People who aren't active Telepathy hackers probably don't want these yet, but:

http://lists.freedesktop.org/archives/telepathy/2014-March/006596.html

> I'm also wondering if we should merge into master after branching a last
> stable branch everywhere.

First we need to get this stuff merged, and ask the maintainers of jhbuild and GNOME-Continuous to switch to stable-branches so it won't all stop working when we merge next into master.

(In reply to comment #36)
> Folks and Empathy pass tests with their respective 'next-gdbus' branches,
> but I still need to smoke-test them, and the 'next' branches are also
> unreviewed

Smoke-testing was successful; reviews still desired.

Fixed in git for 0.99.10

(In reply to Simon McVittie from comment #39)
> Fixed in git for 0.99.10

The last stable release was 0.24.1. When can we expect a stable release from 0.99.x branch?
Thank you.