Created attachment 115662 [details] [review]
add apport monitor

AFAIK, apport is only used by Ubuntu. As such, this funtionality doesn't really make sense upstream. I encourage you to propose this as a patch to the Ubuntu package.

Not really. Apport is also used in openSUSE and Fedora (though it's still under development) - that's already three. Apport is quiet portable, thus hopefully other distributions will start adapting it soon. What I'm suggesting with this patch is adding a cross-distribution means of checking for Apport crash reports for GNOME, is there any reason why this patch should not been included? 

Well, if it really becomes a cross-distro thing, that's a different story (although I'd still disable it by default). I'd like to know what those distros think about it in that case, though.

Well, it would certainly lower the burden for distros to have it live upstream :-) Note that we don't use apport yet in openSUSE (it's a project Nikolay is working on).

Also, feedback from Ubuntu people would be good here, I guess.

Yeah, I don't see a reason why we can't have this (disabled by default) upstream. So, Nikoilay, could you update your patch to have it disabled by default?

Created attachment 115991 [details] [review]
add apport monitor

Attached the patch with Apport plugin disabled by default.

The code has other problems that need to be fixed (the most obvious being coding style), but I'll wait with a detailed review until we've heard from Ubuntu and Fedora.

For the record, I'm the apport upstream developer and Ubuntu maintainer.

Indeed Apport has been written with portability in mind. Everything which is distro specific has an abstract API and concrete implemenations; that particularly affects package status queries (contents, checksums, package names, etc.) and bug tracker interactions (file bug, download bug, add attachment with updated stack trace, etc.).

In Ubuntu we (ab)use our package update notifier for monitoring apport, but of course we'd happily switch to this approach here.

I discussed some portability issues for Fedora with Will Woods (http://fedoraproject.org/wiki/WillWoods) a while ago (longer than a year). Back then he contributed some Fedora issues and I merged them upstream, so that the generic RPM, and particular Fedora package backend is upstream now. I haven't heard anything new from him since then. It seems they are now looking for alternatives and don't pursuit the original spec (http://fedoraproject.org/wiki/Releases/FeatureApport) any more (I'd actually prefer if they would discuss the issues they have with me, of course).

I haven't been in personal contact with anyone from OpenSUSE, but looking at the branch the development there is quite active:

  https://code.launchpad.net/~nderkach/apport/opensuse

so I guess they have some concrete plans with it.

I didn't scrutinize the patch in detail yet, but the general approach seems correct (inotifying /var/crash/, using apport-checkreports to get the status, and calling the GUI).

Thanks, Martin.

Apart from the coding style issues I mentioned:

+/* lcn */

???

+        // check for system crashes

Don't use C++ style comments. Some compilers don't like them-

+                g_warning("Can not run %s\n", CRASHREPORT_HELPER);

g_warning and friends already add a newline themselves. Applies to several places.

+void

static (applies to a number of other functions as well.)

+invoke_with_gnomesu (gchar *cmd)

const gchar *

+    	gchar *argv[3];

const?

+    	argv[0] = "/usr/bin/gnomesu";

Don't hardcode paths.

+run_apport (TrayApplet *ta)
+{
+        g_debug("fire up the crashreport tool\n");
+        if (check_system_crashes())
+        {
+                invoke_with_gnomesu(CRASHREPORT_REPORT_APP);
+                return TRUE;
+        } else
+        return g_spawn_command_line_async(CRASHREPORT_REPORT_APP, NULL);

Drop the "else".

+crashreport_check (TrayApplet *ta)
...
+        int exitcode;
...

Don't declare variables in the middle of a block. Again, not portable.

+        gboolean visible = gtk_status_icon_get_visible(ta->tray_icon);

Same here.

+        hide_crash_applet(ta);
+        
+	    return TRUE;
+}

Broken indentation.

+gboolean
+trayapplet_create (TrayApplet *ta, char *name)
+{
+    	/* setup widgets */
+    	ta->tray_icon = gtk_status_icon_new_from_icon_name (name);
+    	ta->name = name;
+    	gtk_status_icon_set_visible (ta->tray_icon, FALSE);
+    
+    	return TRUE;
+}

You either don't need ta->name (because it's always the same and a static string), or you need to make a copy. Either way, the "name" param should be const.

+        g_signal_connect(G_OBJECT(ta->tray_icon),

Cast is unnecessary.

+    	/* Check for crashes for the first time */
+	    crashreport_check (ta);

Broken identation.

+        GFile *file = g_file_new_for_path(CRASHREPORT_DIR);

Never freed.

+        GFileMonitor *m = g_file_monitor_directory(file, 0, NULL, NULL);
+        g_signal_connect(m, "changed", G_CALLBACK(apport_watchdir_changed), ta); 

"m" may be NULL.

+void
+gsd_apport_manager_stop (GsdApportManager *manager)
+{
+        g_debug ("Stopping apport manager");
+}

You need to disconnect from the signal and free the directory monitor here.

+#define CRASHREPORT_HELPER "/usr/share/apport/apport-checkreports"
+#define CRASHREPORT_REPORT_APP "/usr/share/apport/apport-gtk"
+#define CRASHREPORT_DIR "/var/crash/"

Don't hardcode paths. (The last one might be ok, the others certainly aren't.)

+TrayApplet *ta;

Use a private struct instead of a global object. Needs to be freed on stop () as well.

+        manager = gsd_apport_manager_new ();
+        g_idle_add ((GSourceFunc)idle, manager);
+
+        gtk_main ();
+
+        return 0;

Release the manager.

I guess you should also provide a way to cleanly stop the test program.

Created attachment 116238 [details] [review]
add apport monitor

Fixed accordingly.

It looks to me like you skipped a few of my comments. Please go over the list again and make sure you don't miss any.

In addition that:

+        g_warning("Can not run %s", CRASHREPORT_HELPER);

Please always use "function (...)" consistently instead of  "function(...)".

+    /* exitcode == 0: repots found, else no reports */

Typo.

+    int crashreports_found = 0;
...
+    crashreports_found = !exitcode || system_crashes;
...
+    if ((crashreports_found > 0) && (system_crashes || first_run)) {

Is crashreports_found an int or a boolean? Also, the initialization seems unnecessary.

+    GFileMonitor *m = g_file_monitor_directory (file, 0, NULL, NULL);
+    g_object_unref (file);
+    if (m) {
+        manager->priv->monitor_id = g_signal_connect (m, "changed", G_CALLBACK (apport_watchdir_changed), manager->priv->ta);
+        manager->priv->monitor = m;
+    }
+
+    gnome_settings_profile_end (NULL);
+
+    return TRUE;

If setting up the monitor fails, the plugin doesn't do anything at all. In that case you should simply fail the initialization and return an error.

+    manager->priv->ta = g_new0 (TrayApplet, 1);
...
+    g_object_unref (manager->priv->ta);

That's not going to work...

+libapport_la_CPPFLAGS = 					\
+	-I$(top_srcdir)/gnome-settings-daemon			\
+	-DGNOME_SETTINGS_LOCALEDIR=\""$(datadir)/locale"\"	\
+	-DCRASHREPORT_HELPER=\""/usr/share/apport/apport-checkreports"\"  \
+	-DCRASHREPORT_REPORT_APP=\""/usr/share/apport/apport-gtk"\"  \
+	-DGNOMESU=\""/usr/bin/gnomesu"\"  \
+	$(AM_CPPFLAGS)

Er, how is this not hardcoding things? You should either provide options via configure to set them (the apport prefix, probably), or detect them at runtime from the path (gnomesu).

Some more comments:

instead of g_debug, use gnome_settings_profile functions.

+    if ((crashreports_found > 0) && (system_crashes || first_run)) {
+        gtk_status_icon_set_tooltip(ta->tray_icon,
+                                    _("Crash report detected"));
+        gtk_status_icon_set_blinking(ta->tray_icon, TRUE);
+        gtk_status_icon_set_visible(ta->tray_icon, TRUE);
+    }
+    /* crashreport found and already visible */
+    else if

I think we use in most of the code the:

if (...) {
} else {
}

format.

Also, there are lots of places with "function(...)", so please add a space between the function name and the (

+struct _TrayApplet 
+{
+   GtkStatusIcon *tray_icon;
+   GtkWidget *menu;
+};

do you really need to have this struct? Seems you don't use the menu field anywhere. Also, you g_object_unref the priv->ta thing after having allocated it with g_new0. You need on to replace that g_object_unref with a gtk_widget_destroy on the tray icon

Also, why do you need both trayapplet_create and tray_icon_init functions? Just having one, or putting all that code into gsd_apport_manager_start would be enough

+    GFile *file = g_file_new_for_path (CRASHREPORT_DIR);
+    GFileMonitor *m = g_file_monitor_directory (file, 0, NULL, NULL);

please don't use C99, that is, declare all variables at the beginning of each code block. This is to make sure the code compiles on all supported environments (solaris, for instance)

To clarify, Apport is not used in Fedora.  It may be available as an extra package, but that's it.  The reason?  What I've heard is because the server is not Free Software.  If that's the case, I think it's not suitable for upstream inclusion.

It kinda doesn't feel right to do this in g-s-d, but it's sure better than adding a new process to the session just for this.  I may in fact port the selinux-troubleshooter to g-s-d too.

That's the second place in two days where I read that apport's backend is not free software. Where does that come from? There are really two parts:

- A place to store the crash reports, which is just a bug tracker; apport has an abstract class CrashDB for this, with a Launchpad implementation, and the beginnings of a Bugzilla one. So yes, the storage of those bugs/crash reports for Ubuntu actually uses Launchpad, but they are not more or less free than any other bug report.

- A bot which grabs newly reported bugs, and recombines the core dumps and debug symbol packages to symbolic stack traces, checks for duplicates, and updates the bug report. The code is entirely free and contained in the public bzr repo. It has nothing to do with Launchpad, it's just running in the Canonical data center for Ubuntu because that has good bandwidth and well administered servers. That part is highly distro specific by nature, of course, since it needs to create chroots, install packages, etc.

(In reply to comment #15)
> That's the second place in two days where I read that apport's backend is not
> free software. Where does that come from? There are really two parts:
> 
> - A place to store the crash reports, which is just a bug tracker; apport has
> an abstract class CrashDB for this, with a Launchpad implementation, and the
> beginnings of a Bugzilla one. So yes, the storage of those bugs/crash reports
> for Ubuntu actually uses Launchpad, but they are not more or less free than any
> other bug report.
> 
> - A bot which grabs newly reported bugs, and recombines the core dumps and
> debug symbol packages to symbolic stack traces, checks for duplicates, and
> updates the bug report. The code is entirely free and contained in the public
> bzr repo. It has nothing to do with Launchpad, it's just running in the
> Canonical data center for Ubuntu because that has good bandwidth and well
> administered servers. That part is highly distro specific by nature, of course,
> since it needs to create chroots, install packages, etc.

Umm, I think I may be basing my comment on the same place that you read also.  So you can discount this as the second time.  I should have done more research.  With a bugzilla backend under development, I don't see any non-freeness reasons.

Sorry about the noise.

FWIW, Fedora uses ABRT (for better or for worst).

Given that Martin doesn't seem opposed to adding a gnome-settings-daemon plugin, and is an Apport upstream maintainer, I'm sure he'd appreciate a patch to get this integrated in apport itself.

gnome-settings-daemon has a plugin API which can be used to create and ship such plugins externally. For my part, I'd probably rather see this in a separate, small, command-line application that started with the session.