← Back to team overview

apport-hackers team mailing list archive

Re: [Merge] lp:~ev/apport/grouped_reports into lp:apport

 

Hello Evan,

sorry for the late answer! Nexus sprint, FOSDEM and all that..

> +        grouped_reports = {}
> +        for f in reports:
> +            self.load_report(f)
> +            if not self.load_report(f) or not self.get_desktop_entry():

It seems we load the application reports twice here, and further down.
I guess it does it that way instead of calling run_crash(f)
immediately to avoid spreading the internal ones over several
application report popups. This should eventually be made more clever
by either saving the application reports in a separate map, or using
load() with binary=False, or just accepting that in the rare case of
multiple apps crashing at the same time the internal reports might get
spread out over two runs (which, from the POV of avoiding too many
reports in memory at the same time might not actually a bad thing?)

> +                # We may not be able to load the report, but we should still
> +                # show that it occurred and send the reason for statistical
> +                # purposes.
> +                grouped_reports[f] = self.report
> +                self.packages[f] = self.cur_package

So grouped_reports() has both the "failed to load" as well as the
"system" ones. If load_report() fails we need to be *much* more
careful, though. The file might not be readable, have disappeared from
disk, or be totally corrupted, so you cannot assume anything about
self.report or self.cur_package. self.report will be None for
many failure conditions, while self.cur_package could just be the
previous successful one.

I don't think we should do anything if the first set of checks in
load_reports() fails. For the "ensure that the crashed program is
still installed" case, if that fails we should either do nothing, or
continue to show the error message, but what's the point of telling
errors.u.c. about that?

In any case, this means that grouped_reports/self.package has some
reliable and some bogus data, and after this code we don't kno any
more which is which. Can we perhaps continue to ignore the broken ones
for now, and perhaps handle these in a separate MP?

> +                    # Make sure we don't report these again. Note that these
> +                    # grouped reports will not be marked as seen if we do not
> +                    # have a regular application error report to present.
> +                    for f in grouped_reports.keys():
> +                        apport.fileutils.mark_report_seen(f)

So we will always mark them as seen even if the user declines to send
them? I don't have a better idea about these either, but it kind of
seems to be contradictory to your usual approach of "I want to get
all reports, no matter how broken they are" :-) But this seems ok to
me.

>      def run_crash(self, report_file, confirm=True):
> [...]
> +            for path, report in self.get_reports():
> +                # check for absent CoreDumps (removed if they exceed size limit)
> +                if report.get('ProblemType') == 'Crash' and 'Signal' in report and 'CoreDump' not in report and 'Stacktrace' not in report:
> +                    reason = _('This problem report is damaged and cannot be processed.')

Given that you dropped the error message for those, do we even need
two different fields? As machine_reason is quite human (well,
developer) readable, wouldn't this suffice?

> -    def collect_info(self, symptom_script=None, ignore_uninstalled=False,
> +    def exception_wrapped_collect_info(self, report, report_file=None,
> +                                       cur_package=None, symptom_script=None,
> +                                       ignore_uninstalled=False,
> +                                       on_finished=None):

Do we actually need to split out and rename this? collect_info() could
just not throw/catch these by itself?

> +     except (IOError, zlib.error, struct.error) as e:
> + 	    # can happen with broken core dumps
> + 	    reason = _('This problem report is damaged and cannot be processed.')
> +         machine_reason = excstr(e)
> +         mark_invalid(report, report_file, reason, machine_reason)
> [...]

Same comment about reason/machine_reason as above in run_crash().

> +    def collect_all_info(self, on_finished=None):
> +        '''Collect additional information from an application report and from
> +        all grouped reports.'''
> +        if 'Dependencies' not in self.report:
> +            self.exception_wrapped_collect_info(self.report,
> +                                                self.report_file,
> +                                                self.cur_package)
> +
> +        for path in self.grouped_reports:
> +            report = self.grouped_reports[path]
> +            if 'Dependencies' not in report:
> +                self.exception_wrapped_collect_info(report, path, self.packages[path])

That could be made easier with iterating over self.get_reports()?

> +                      <object class="GtkCheckButton" id="previous_internal_errors">
> +                        <property name="label" translatable="yes">Report previous internal errors too</property>
> +                        <property name="visible">True</property>
> +                        <property name="can_focus">True</property>
> +                        <property name="receives_default">False</property>
> +                        <property name="xalign">0.0099999997764825821</property>
> +                        <property name="yalign">0.51999998092651367</property>

Wow, was that glade? 0 and 0.5 would certainly do :)

The rest looks good to me at first sight. Given that this uses some
new fancyness like itertools, does this still work with both Python 2
and 3?

Thanks again!

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

https://code.launchpad.net/~ev/apport/grouped_reports/+merge/144591
Your team Apport upstream developers is requested to review the proposed merge of lp:~ev/apport/grouped_reports into lp:apport.


References