[Merge] lp:~slyon/apport/apport-snap-handling into lp:~ubuntu-core-dev/ubuntu/groovy/apport/ubuntu

[Brian Murray]

Review: Needs Information

This looks great thanks for working on it and I like the fact that a question is asked if the system has both the snap and deb installed.

I'm curious about the additions to thread_collect_info function though, this is only called for crash reports and as far as I know apport doesn't generate crash reports for snaps. Subsequently, I think the additions to thread_collect_info are unnecessary. Is there something I'm missing?



Diff comments:

> 
> === modified file 'apport/report.py'
> --- apport/report.py	2020-06-03 22:01:31 +0000
> +++ apport/report.py	2020-06-08 13:13:22 +0000
> @@ -376,6 +378,26 @@
>              self['Dependencies'] += '%s %s%s' % (
>                  dep, v, self._customized_package_suffix(dep))
>  
> +    def add_snap_info(self, snap):
> +        '''Add info about an installed Snap
> +
> +        This adds a Snap: field, containing name, version and channel.
> +        It adds a SnapSource: field, if the snap has a Launchpad contact defined.
> +        '''
> +        self['Snap'] = '%s %s (%s)' % (snap.get("name"), snap.get("version"),
> +                                        snap.get("channel", "unknown"))

Are name and version always guaranteed to be defined?

> +        # Automatically handle snaps which have a Launchpad contact defined
> +        if snap.get("contact"):
> +            # Parse Launchpad project (e.g. 'subiquity') or source package string
> +            # (e.g. 'ubuntu/+source/gnome-calculator') from snap 'contact'
> +            # Additionaly, extract any tag/tags defined in the contact URL.
> +            p = r'^https?:\/\/.*launchpad\.net\/((?:[^\/]+\/\+source\/)?[^\/]+)(?:.*field\.tags?=([^&]+))?'
> +            m = re.search(p, unquote(snap.get("contact", "")))
> +            if m and m.group(1):
> +                self['SnapSource'] = m.group(1)
> +                if m.group(2):
> +                    self['SnapTags'] = m.group(2)
> +
>      def add_os_info(self):
>          '''Add operating system information.
>  
> 
> === modified file 'apport/ui.py'
> --- apport/ui.py	2020-03-09 22:18:03 +0000
> +++ apport/ui.py	2020-06-08 13:13:22 +0000
> @@ -1153,18 +1140,52 @@
>                                                            'process this problem report:') + '\n\n' + str(e)
>                      self.report['_MarkForUpload'] = 'False'
>  
> -            snap = find_snap(self.cur_package)
> -            if snap and 'UnreportableReason' not in self.report and self.specified_a_pkg:
> -                if snap.get("contact", ""):
> -                    msg = _('You are about to report a bug against the deb package, but you also a have snap published by %s installed. You can contact them via %s for help. Do you want to continue with the bug report against the deb?') % (snap["developer"], snap["contact"])
> -                else:
> -                    msg = _('You are about to report a bug against the deb package, but you also a have snap published by %s installed. For the snap, no contact address has been provided; visit the forum at https://forum.snapcraft.io/ for help. Do you want to continue with the bug report against the deb?') % snap["developer"]
> -
> -                if not self.ui_question_yesno(msg):
> +            # ask for target, if snap and deb package are installed
> +            if 'Snap' in self.report and 'Package' in self.report:
> +                if '(not installed)' in self.report['Package']:
> +                    # choose snap automatically, if deb package is not installed
> +                    res = [0]
> +                else:
> +                    res = self.ui_question_choice(_('You have two versions of this package installed, which one do you want to report a bug against?'), [

I think this should be application instead of package.

> +                                                  _('%s Snap') % self.report['Snap'],

Please change Snap to snap as that's what is used later on.

> +                                                  _('%s deb package') % self.report['Package']
> +                                                  ], False)
> +                # bug report is about the snap, clean deb package info
> +                if res == [0]:
> +                    del self.report['Package']
> +                    if 'PackageArchitecture' in self.report:
> +                        del self.report['PackageArchitecture']
> +                    if 'SourcePackage' in self.report:
> +                        del self.report['SourcePackage']
> +                # bug report is about the deb package, clean snap info
> +                elif res == [1]:
> +                    del self.report['Snap']
> +                    if 'SnapSource' in self.report:
> +                        del self.report['SnapSource']
> +                    if 'SnapTags' in self.report:
> +                        del self.report['SnapTags']
> +                else:
>                      self.ui_stop_info_collection_progress()
>                      sys.exit(0)
>                      return
>  
> +            # append snap tags, if this report is about the snap
> +            if 'Snap' in self.report and 'SnapTags' in self.report:
> +                current_tags = self.report.get('Tags', '')
> +                if current_tags:
> +                    current_tags += ' '
> +                self.report['Tags'] = current_tags + self.report['SnapTags']
> +                del self.report['SnapTags']
> +
> +            # show a hint if we cannot auto report a snap bug via 'SnapSource'
> +            if 'Snap' in self.report and 'SnapSource' not in self.report and 'UnreportableReason' not in self.report and self.specified_a_pkg:
> +                snap = apport.fileutils.find_snap(self.cur_package)
> +                if snap.get("contact", ""):
> +                    self.report['UnreportableReason'] = _('%s is provided by a snap published by %s. Contact them via %s for help.') % (snap["name"], snap["developer"], snap["contact"])
> +                else:
> +                    self.report['UnreportableReason'] = _('%s is provided by a snap published by %s. No contact address has been provided; visit the forum at https://forum.snapcraft.io/ for help.') % (snap["name"], snap["developer"])
> +                self.report['_MarkForUpload'] = 'False'
> +
>              if 'UnreportableReason' in self.report or not self.check_report_crashdb():
>                  self.ui_stop_info_collection_progress()
>                  if on_finished:


-- 
https://code.launchpad.net/~slyon/apport/apport-snap-handling/+merge/385270
Your team Ubuntu Core Development Team is subscribed to branch lp:~ubuntu-core-dev/ubuntu/groovy/apport/ubuntu.