GNOME Bugzilla – Bug 711496
Build broken with upower 0.99
Last modified: 2013-11-14 09:22:10 UTC
[1] attempted to add support for the new version of upower, but two things went wrong: 1) AC_CHECK_FUNC was used instead of AC_CHECK_FUNCS. Only the plural version defines the HAVE_ symbol [2]. 2) up_client_get_warning_level was removed one day later by [3]. This isn't caught by gnome-continuous since it's building tracker with --disable-upower [1] https://git.gnome.org/browse/tracker/commit/?id=2f6f813b2ca28bb1a4ae937d71a2203d5c6cd3f5 [2] http://www.gnu.org/software/autoconf/manual/autoconf-2.64/html_node/Generic-Functions.html [3] http://cgit.freedesktop.org/upower/commit/?id=b2f72dd69ac683e7fa213d83b410b38fe1ab1b21
Ah. Yes, ok my fault, I guess I didn't realize offhand I wasn't even building my code since I'd been using --disable-upower =( Working on a patch.
Created attachment 259038 [details] [review] miners/fs/upower: Fix up porting to newer UPower My previous attempt was totally broken as I'd accidentally been passing --disable-upower to configure =/ This patch at least compiles with new upower; doing some testing now.
Created attachment 259054 [details] [review] miners/fs/upower: Fix up porting to newer UPower Ok, this variant builds and at least emits no warnings with the latest upower, I didn't try older upower though myself.
Comment on attachment 259054 [details] [review] miners/fs/upower: Fix up porting to newer UPower >From 56cc6294e078d01d177be5e6817ab8d40ffafe91 Mon Sep 17 00:00:00 2001 >From: Colin Walters <walters@verbum.org> >Date: Tue, 5 Nov 2013 13:53:22 -0500 >Subject: [PATCH] miners/fs/upower: Fix up porting to newer UPower > >My previous attempt was totally broken as I'd accidentally been >passing --disable-upower to configure =/ Doh :) >This patch at least compiles with new upower; doing some testing >now. Thanks for the new patch Colin! >- AC_CHECK_FUNC(up_client_get_warning_level) >+ AC_CHECK_FUNCS(up_client_get_on_low_battery) Hmm, so _get_on_low_battery() came in for 0.9.0 and we depend on that, so I wonder why we need to check for this function at all? >diff --git a/src/miners/fs/tracker-power-upower.c b/src/miners/fs/tracker-power-upower.c >index 6934fb4..677581b 100644 >--- a/src/miners/fs/tracker-power-upower.c >+++ b/src/miners/fs/tracker-power-upower.c >@@ -30,6 +30,9 @@ > > typedef struct { > UpClient *client; >+#ifndef HAVE_UP_CLIENT_GET_ON_LOW_BATTERY >+ UpDevice *composite_device; >+#endif > gboolean on_battery; > gboolean on_low_battery; > } TrackerPowerPriv; Same argument as above. We have a lot of preprocessor directives for _GET_ON_LOW_BATTERY, but AFAICS, we actually have that API in the version we depend on - so why do we need those? What I couldn't tell from looking in git and the docs, is what version the up_client_get_warning_level() was available in. I checked master and 0.9 branch for upower? I didn't have coffee yet this morning, but it seems to me easier to just remove the #ifdef/#ifndef statements, remove the _get_warning_level() call all together and make the patch much simpler :) Thoughts? :)
(In reply to comment #4) > > Hmm, so _get_on_low_battery() came in for 0.9.0 and we depend on that, so I > wonder why we need to check for this function at all? Then it was removed for 0.99.
So the situation is: up_client_get_on_low_battery was removed in upower 0.99.0. up_client_get_warning_level arrived in master a few weeks ago but was removed before 0.99.0. Colin's patch removes the use of this function.
the equivalent to the short-lived up_client_get_warning_level() is: device = up_client_get_display_device (client); g_object_get (device, "warning-level", &warning_level, NULL); if (warning_level >= UP_DEVICE_STATE_DISCHARGING) /* do low-power thing */ else /* chew CPU */
(In reply to comment #7) > the equivalent to the short-lived up_client_get_warning_level() is: > device = up_client_get_display_device (client); > g_object_get (device, "warning-level", &warning_level, NULL); > > if (warning_level >= UP_DEVICE_STATE_DISCHARGING) > /* do low-power thing */ > else > /* chew CPU */ Yeah, we're doing that in the current patch. Martyn, I believe this patch to be correct at least with UPower 0.99, though it could use testing by someone else with UPower 0.9.22.
I should have looked at the patch I guess ;)
Comment on attachment 259054 [details] [review] miners/fs/upower: Fix up porting to newer UPower The patch works fine with 0.9.23 from my testing, I think it is fine to push. At some point we should start trimming older implementations using far too old APIs :P, like HAL and whatnot
Thanks, pushed to both tracker-0.16 and master.
(In reply to comment #10) > (From update of attachment 259054 [details] [review]) > The patch works fine with 0.9.23 from my testing, I think it is fine to push. > > At some point we should start trimming older implementations using far too old > APIs :P, like HAL and whatnot Totally agree Carlos. Thanks for reviewing, much on my plate lately.