(not currently active) Ted Mielczarek Reporter
	Description • 17 years ago

We should get a crash reporter client working on Linux.

	(not currently active) Ted Mielczarek Reporter
	Comment 1 • 17 years ago

fwiw, callion said that libcurl is probably installed by default on FC since OOo depends on it.

	Dave Camp (:dcamp) Assignee
	Comment 2 • 17 years ago

Here's a first stab at a linux client.  Need to figure out the libcurl stuff better before we commit it, what's in this patch is a bit of a hack.

	(not currently active) Ted Mielczarek Reporter
	Comment 3 • 17 years ago

If we just build http_upload.cc into the static lib in airbag/src/common/linux/Makefile.in, it shouldn't be a problem, should it?  The linker will throw out those functions in the main app since they're not used.  Should be easy enough to test, since the libcurl lib isn't specified for the main app.

	(not currently active) Ted Mielczarek Reporter
	Comment 4 • 17 years ago

Comment on attachment 268729 [details] [diff] [review]
WIP

>diff -r e879df4187e6 toolkit/airbag/client/Makefile.in
> LIBS += \
>-  $(DEPTH)/toolkit/airbag/airbag/src/common/linux/$(LIB_PREFIX)breakpad_linux_common_s.$(LIB_SUFFIX) \
>-  $(NULL)
>+	$(DEPTH)/toolkit/airbag/airbag/src/common/linux/$(LIB_PREFIX)breakpad_linux_common_s.$(LIB_SUFFIX) \
>+	$(NULL)

Actually I think I specifically made this indent this way, since this is what I now prefer.  If you feel like changing it, change the other LIBS sections to match the smaller indent style instead.

> include $(topsrcdir)/config/rules.mk

>+
>+ifeq ($(OS_ARCH),Linux)
>+export:: $(srcdir)/../airbag/src/common/linux/http_upload.cc
>+	$(INSTALL) $^ .
>+endif

This sucks but it's not your fault. :)

>diff -r e879df4187e6 toolkit/airbag/client/crashreporter.cpp

>+bool CrashReporterReadStrings(istream& in, StringTable& strings, bool unescape)
>+bool CrashReporterReadStringsFromFile(const string& path,
>+bool CrashReporterWriteStrings(ostream& out,
>+bool CrashReporterWriteStringsToFile(const string& path,

I'd prefer namespace CrashReporter {} around this instead.  It's only 2 more characters in the function name then, and you can even do using namespace CrashReporter.
 
>diff -r e879df4187e6 toolkit/airbag/client/crashreporter_linux.cpp
>+  if (CrashReporterReadStringsFromFile(gSettingsPath + "/" + kIniFile,
>+                                       settings, true)) {
>+    gtk_entry_set_text(GTK_ENTRY(gEmailEntry), settings["Email"].c_str());

Any reason you don't do the same test here you do below it?  (if settings.find(foo) != settings.end())

>+static gboolean SendReportIdle(gpointer userData)
>+{
>+  string response;
>+  bool success = google_breakpad::HTTPUpload::SendRequest
>+    (gSendURL,
>+     gQueryParameters,
>+     gDumpFile,
>+     "upload_file_minidump",
>+     "", "",
>+     &response);

We should file a followup on passing a proxy through from the app if one is set.

> void UIShowCrashUI(const string& dumpfile,

I'm just going to pretend I didn't read this function, because it totally sucks to have to do all of that.  But again, not your fault.

Looks good aside from those few little things!  r=me

	Dave Camp (:dcamp) Assignee
	Comment 6 • 17 years ago

(In reply to comment #4)
> >diff -r e879df4187e6 toolkit/airbag/client/crashreporter.cpp
> 
> >+bool CrashReporterReadStrings(istream& in, StringTable& strings, bool unescape)
> >+bool CrashReporterReadStringsFromFile(const string& path,
> >+bool CrashReporterWriteStrings(ostream& out,
> >+bool CrashReporterWriteStringsToFile(const string& path,
> 
> I'd prefer namespace CrashReporter {} around this instead.  It's only 2 more
> characters in the function name then, and you can even do using namespace
> CrashReporter.

I moved most of crashreporter.cpp into that namespace (everything but main()).

> >diff -r e879df4187e6 toolkit/airbag/client/crashreporter_linux.cpp
> >+  if (CrashReporterReadStringsFromFile(gSettingsPath + "/" + kIniFile,
> >+                                       settings, true)) {
> >+    gtk_entry_set_text(GTK_ENTRY(gEmailEntry), settings["Email"].c_str());
> 
> Any reason you don't do the same test here you do below it?  (if
> settings.find(foo) != settings.end())

The default string is empty, so setting it has no effect.  But I put in a check there for consistency. 
 
> >+static gboolean SendReportIdle(gpointer userData)
> >+{
> >+  string response;
> >+  bool success = google_breakpad::HTTPUpload::SendRequest
> >+    (gSendURL,
> >+     gQueryParameters,
> >+     gDumpFile,
> >+     "upload_file_minidump",
> >+     "", "",
> >+     &response);
> 
> We should file a followup on passing a proxy through from the app if one is
> set.

filed bug 385280

	(not currently active) Ted Mielczarek Reporter
	Comment 7 • 17 years ago

Looks good.  The current tinderbox might not have libcurl (I'm not sure), but the new refplatform definitely does, so as long as we're not building this by default, we're ok right now.  I'll check to see if the current tinderbox has libcurl, and if so, maybe we can enable sooner.

	Benjamin Smedberg
	Comment 8 • 17 years ago

What headers/libs should we be looking for?

	(not currently active) Ted Mielczarek Reporter
	Comment 9 • 17 years ago

pkg-config --modversion libcurl

Should be sufficient to verify that it will work.

	Dave Camp (:dcamp) Assignee
	Comment 10 • 17 years ago

committed the updated patch, here's one to enable by default

	(not currently active) Ted Mielczarek Reporter
	Comment 11 • 17 years ago

Comment on attachment 269250 [details] [diff] [review]
enable breakpad by default on linux

We may want to disable building this on 64-bit linux, but I don't actually know how to test for that in configure.  I'm sure someone will complain about it if it breaks.

	Dave Camp (:dcamp) Assignee
	Comment 12 • 17 years ago

This version falls back to curl-config if libcurl.pc isn't found.

	(not currently active) Ted Mielczarek Reporter
	Comment 13 • 17 years ago

Comment on attachment 269277 [details] [diff] [review]
fall back to curl-config

Awesome, let's try this again.

	Dave Camp (:dcamp) Assignee
	Comment 14 • 17 years ago

adds some #includes on linux and mac

	Dave Camp (:dcamp) Assignee
	Comment 15 • 17 years ago

This adds the crash reporter to linux's manifest file and updates the comment on windows

	Dave Camp (:dcamp) Assignee
	Comment 17 • 17 years ago

(attached the wrong file)

	Bill Gianopoulos [:WG9s]
	Comment 18 • 17 years ago

(In reply to comment #11)
> (From update of attachment 269250 [details] [diff] [review])
> We may want to disable building this on 64-bit linux, but I don't actually know
> how to test for that in configure.  I'm sure someone will complain about it if
> it breaks.
> 

The correct way to test in configure would be to key off of HAVE_64BIT_OS.  This way if you are doing a build on a 64-bit OS, but have set CFLAGS=-m32 in order to do a 32-bit build it will figure this out and not disable airbag.

	(not currently active) Ted Mielczarek Reporter
	Comment 19 • 17 years ago

Comment on attachment 269456 [details] [diff] [review]
update mail manifest too

God I hate how this works.  :-(

	Axel Hecht [:Pike]
	Comment 20 • 17 years ago

Attachment 269250 [details] [diff] broke --disable-compile-environment again, sadly. I guess the extra error message should be if test_libs or whatever that is called. Bug 383463 shows how I fixed this just recently.

This turned the trunk builds on http://l10n.mozilla.org/buildbot/ red.

	(not currently active) Ted Mielczarek Reporter
	Comment 21 • 17 years ago

(In reply to comment #20)
> Attachment 269250 [details] [diff] broke --disable-compile-environment again, sadly. I guess the
> extra error message should be if test_libs or whatever that is called. Bug
> 383463 shows how I fixed this just recently.
> 
> This turned the trunk builds on http://l10n.mozilla.org/buildbot/ red.

Axel, can you file a new bug on this?  This bug is getting a bit overused.  Let's track any remaining issues in dependencies.

	Axel Hecht [:Pike]
	Comment 22 • 17 years ago

Filed bug 385745 on --disable-compile-environment.