|
Description
Simon McVittie
2013-10-29 11:13:48 UTC
(Note that "telepathy-logger 1" is now part of the telepathy-glib repository.) Created attachment 91976 [details] [review] log-store-xml: stop using TPL_TEST_LOG_DIR Created attachment 91977 [details] [review] log-store-pidin: stop relying on TPL_TEST_LOG_DIR for tests Created attachment 91978 [details] [review] log stores: add constructor helpers Created attachment 91979 [details] [review] log-manager: use store constructor helpers Created attachment 91980 [details] [review] log-store-{pidgin,xml}: remove 'basedir' property Created attachment 91981 [details] [review] remove 'testmode' on log stores Created attachment 91982 [details] [review] logger tests: use store constructor helpers Created attachment 91983 [details] [review] log-store: add 'name' property Created attachment 91984 [details] [review] log-store: add 'writable' property Created attachment 91985 [details] [review] implement the empathy store using the XML store directly Created attachment 91986 [details] [review] Store XML logs to telepathy-1.0/ Created attachment 91987 [details] [review] add XML legacy log store I think that's the last thing needed to make the logger parallel installable, isn'it? Comment on attachment 91976 [details] [review] log-store-xml: stop using TPL_TEST_LOG_DIR Review of attachment 91976 [details] [review]: ----------------------------------------------------------------- tests/logger/Makefile.am doesn't adjust XDG_DATA_HOME. Should it? It would be great if the Logger's XDG_DATA_HOME could move into the tests themselves so they can safely be run without the wrapper (as a bonus, they could use separate log directories and become parallel-safe) but that's a job for the future. Comment on attachment 91977 [details] [review] log-store-pidin: stop relying on TPL_TEST_LOG_DIR for tests Review of attachment 91977 [details] [review]: ----------------------------------------------------------------- ::: telepathy-logger/log-store-pidgin.c @@ +207,4 @@ > { > gchar *dir; > > + dir = g_build_path (G_DIR_SEPARATOR_S, g_get_home_dir (), ".purple", This is only OK for the tests because we have a hard dependency on GLib 2.36, in which g_get_home_dir() respects $HOME. I think this deserves a comment? Comment on attachment 91978 [details] [review] log stores: add constructor helpers Review of attachment 91978 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 91979 [details] [review] log-manager: use store constructor helpers Review of attachment 91979 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 91980 [details] [review] log-store-{pidgin,xml}: remove 'basedir' property Review of attachment 91980 [details] [review]: ----------------------------------------------------------------- OK, if unused Comment on attachment 91981 [details] [review] remove 'testmode' on log stores Review of attachment 91981 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 91982 [details] [review] logger tests: use store constructor helpers Review of attachment 91982 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 91983 [details] [review] log-store: add 'name' property Review of attachment 91983 [details] [review]: ----------------------------------------------------------------- ::: telepathy-logger/log-store-pidgin.c @@ +134,5 @@ > + > + g_free (priv->basedir); > + g_free (priv->name); > + > + G_OBJECT_CLASS (tpl_log_store_pidgin_parent_class)->dispose (self); should be ->finalize Comment on attachment 91985 [details] [review] implement the empathy store using the XML store directly Review of attachment 91985 [details] [review]: ----------------------------------------------------------------- ::: telepathy-logger/log-store-pidgin.c @@ +93,4 @@ > g_value_set_boolean (value, TRUE); > break; > case PROP_WRITABLE: > + g_value_set_boolean (value, self->priv->writable); Why does this patch make log-store-pidgin potentially writable? ::: telepathy-logger/log-store-sqlite.c @@ +119,4 @@ > break; > > case PROP_WRITABLE: > + g_value_set_boolean (value, priv->writable); I can see that there's value in being able to make a sqlite store read-only, but it should be mentioned in the commit message, which currently implies that only the XML store got this treatment. Comment on attachment 91987 [details] [review] add XML legacy log store Review of attachment 91987 [details] [review]: ----------------------------------------------------------------- ::: tests/logger/dbus/test-log-manager.c @@ +325,1 @@ > /* it includes 1 date from libpurple logs, 5 from TpLogger. Empathy ... 5 from telepathy-1.0 and 1 from TpLogger I'd suggest using clever numbering to make it obvious which one(s) are being used, something like this: * 1 item in libpurple * 2 items in Empathy which are a subset of those in telepathy-1.0 * 4 items in TpLogger * 8 in telepathy-1.0 Then you'd expect to see 13 items, and if you see anything different, you can work out what has gone wrong using binary. Comment on attachment 91986 [details] [review] Store XML logs to telepathy-1.0/ Review of attachment 91986 [details] [review]: ----------------------------------------------------------------- telepathy-1, perhaps? We've been reasonably consistent about that naming elsewhere. Looks good except where noted. (In reply to comment #14) > I think that's the last thing needed to make the logger parallel > installable, isn'it? Seems likely. (In reply to comment #15) > Comment on attachment 91976 [details] [review] [review] > log-store-xml: stop using TPL_TEST_LOG_DIR > > Review of attachment 91976 [details] [review] [review]: > ----------------------------------------------------------------- > > tests/logger/Makefile.am doesn't adjust XDG_DATA_HOME. Should it? It was already set. > It would be great if the Logger's XDG_DATA_HOME could move into the tests > themselves so they can safely be run without the wrapper (as a bonus, they > could use separate log directories and become parallel-safe) but that's a > job for the future. I tried that actually: relying on the C tests to set all the magic env variable itself. My goal was to run each test in its own /tmp/logger-tests-log-XXX/ directory and so be more self contained. But that didn't work because GLib internally cache the value of the variable, so it can't be changed during the lifetime of the process: https://git.gnome.org/browse/glib/tree/glib/gutils.c#n1156 (In reply to comment #16) > Comment on attachment 91977 [details] [review] [review] > log-store-pidin: stop relying on TPL_TEST_LOG_DIR for tests > > Review of attachment 91977 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-logger/log-store-pidgin.c > @@ +207,4 @@ > > { > > gchar *dir; > > > > + dir = g_build_path (G_DIR_SEPARATOR_S, g_get_home_dir (), ".purple", > > This is only OK for the tests because we have a hard dependency on GLib > 2.36, in which g_get_home_dir() respects $HOME. I think this deserves a > comment? What do you mean? Purple logs are always stored in ~/.purple anyway. (In reply to comment #22) > Comment on attachment 91983 [details] [review] [review] > log-store: add 'name' property > > Review of attachment 91983 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-logger/log-store-pidgin.c > @@ +134,5 @@ > > + > > + g_free (priv->basedir); > > + g_free (priv->name); > > + > > + G_OBJECT_CLASS (tpl_log_store_pidgin_parent_class)->dispose (self); > > should be ->finalize Good catch; fixed. (In reply to comment #23) > Comment on attachment 91985 [details] [review] [review] > implement the empathy store using the XML store directly > > Review of attachment 91985 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-logger/log-store-pidgin.c > @@ +93,4 @@ > > g_value_set_boolean (value, TRUE); > > break; > > case PROP_WRITABLE: > > + g_value_set_boolean (value, self->priv->writable); > > Why does this patch make log-store-pidgin potentially writable? > > ::: telepathy-logger/log-store-sqlite.c > @@ +119,4 @@ > > break; > > > > case PROP_WRITABLE: > > + g_value_set_boolean (value, priv->writable); > > I can see that there's value in being able to make a sqlite store read-only, > but it should be mentioned in the commit message, which currently implies > that only the XML store got this treatment. I just naively implemented the property in each implementation of the log store interface, wich requires a setting function as the property is write only. Actually I'm wondering if it wouldn't be easier/cleaner to turn the store iface to a base class so it could implement the property itself, saving us to do it in each store. What do you think? Created attachment 92031 [details] [review] fixed Created attachment 92032 [details] [review] Store XML logs to telepathy-1/ changed. (In reply to comment #27) > > tests/logger/Makefile.am doesn't adjust XDG_DATA_HOME. Should it? > > It was already set. tests/logger/dbus/Makefile.am does set it, but tests/logger/Makefile.am doesn't? (Perhaps that doesn't matter, though.) (In reply to comment #28) > > This is only OK for the tests because we have a hard dependency on GLib > > 2.36, in which g_get_home_dir() respects $HOME. I think this deserves a > > comment? > > What do you mean? Purple logs are always stored in ~/.purple anyway. Before GLib 2.36, g_get_home_dir() used the equivalent of `getent passwd` to find the home directory, and ignored $HOME. (There was a Debian-specific patch to use $G_HOME.) (In reply to comment #30) > > I can see that there's value in being able to make a sqlite store read-only, > > but it should be mentioned in the commit message, which currently implies > > that only the XML store got this treatment. > > I just naively implemented the property in each implementation of the log > store interface, wich requires a setting function as the property is write > only. Ah, I understand now. I think the Pidgin log store should g_warn_if_fail (writable == FALSE) or something, and be read-only anyway. > Actually I'm wondering if it wouldn't be easier/cleaner to turn the store > iface to a base class so it could implement the property itself, saving us > to do it in each store. What do you think? Neutral. If you want to do this I won't stop you, but I don't fully understand how much of this stuff is exposed to API users, and interface > class in terms of minimizing API guarantees. Comment on attachment 92031 [details] [review] fixed Review of attachment 92031 [details] [review]: ----------------------------------------------------------------- Looks OK Comment on attachment 92032 [details] [review] Store XML logs to telepathy-1/ Review of attachment 92032 [details] [review]: ----------------------------------------------------------------- Sure Created attachment 92037 [details] [review] add XML legacy log store Fixed the comment. I tried adding more logs as you suggested but it broke at lot of others tests which are very strict expectations on the number and order logs stored and I didn't bother fixing all of those. (In reply to comment #33) > (In reply to comment #27) > > > tests/logger/Makefile.am doesn't adjust XDG_DATA_HOME. Should it? > > > > It was already set. > > tests/logger/dbus/Makefile.am does set it, but tests/logger/Makefile.am > doesn't? (Perhaps that doesn't matter, though.) Yeah all the actual tests are in tests/logger/dbus. > (In reply to comment #28) > > > This is only OK for the tests because we have a hard dependency on GLib > > > 2.36, in which g_get_home_dir() respects $HOME. I think this deserves a > > > comment? > > > > What do you mean? Purple logs are always stored in ~/.purple anyway. > > Before GLib 2.36, g_get_home_dir() used the equivalent of `getent passwd` to > find the home directory, and ignored $HOME. (There was a Debian-specific > patch to use $G_HOME.) Ah I see. Adding a comment. > (In reply to comment #30) > > > I can see that there's value in being able to make a sqlite store read-only, > > > but it should be mentioned in the commit message, which currently implies > > > that only the XML store got this treatment. > > > > I just naively implemented the property in each implementation of the log > > store interface, wich requires a setting function as the property is write > > only. > > Ah, I understand now. > > I think the Pidgin log store should g_warn_if_fail (writable == FALSE) or > something, and be read-only anyway. Good point; adding that. > > Actually I'm wondering if it wouldn't be easier/cleaner to turn the store > > iface to a base class so it could implement the property itself, saving us > > to do it in each store. What do you think? > > Neutral. If you want to do this I won't stop you, but I don't fully > understand how much of this stuff is exposed to API users, and interface > > class in terms of minimizing API guarantees. Fair enough; let's keep it as it for now. Created attachment 92038 [details] [review] log-store-pidin: stop relying on TPL_TEST_LOG_DIR for tests Created attachment 92040 [details] [review] log-store: add 'writable' property Review ping? That's one of our latest 1.0 blocker. Reviewing this, sorry for the delay. (In reply to comment #27) > > tests/logger/Makefile.am doesn't adjust XDG_DATA_HOME. Should it? > > It was already set. Not in tests/logger/Makefile.am, only in tests/logger/dbus/Makefile.am. Also, there's a difference: tests/logger/dbus/Makefile.am: TPL_TEST_LOG_DIR=@abs_top_srcdir@/tests/logger/logs \ tests/logger/dbus/Makefile.am: XDG_DATA_HOME=@abs_top_builddir@/tests/logger/logs \ Which should it be? I wonder whether we should in fact be creating a temporary directory in the builddir, and copying the logs in from the srcdir before each test? (In reply to comment #42) > Not in tests/logger/Makefile.am, only in tests/logger/dbus/Makefile.am. I fixed that. > tests/logger/dbus/Makefile.am: > TPL_TEST_LOG_DIR=@abs_top_srcdir@/tests/logger/logs \ > tests/logger/dbus/Makefile.am: > XDG_DATA_HOME=@abs_top_builddir@/tests/logger/logs \ I fixed that; it should have been consistently the srcdir (tests failed without this change, and pass with it). > I wonder whether we should in fact be creating a temporary directory in the > builddir, and copying the logs in from the srcdir before each test? The tests that need write access already do that. Pushed with those changes. |
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.