Merge lp:~malizor/update-manager/ppa-changelogs into lp:update-manager

[Nicolas Delvaux]

Retrieve Changelogs from PPA sources if python3-launchpadlib is installed.

This uses the approach suggested by Colin Watson in LP: #253119.

Retrieve Changelogs from PPA sources if python3-launchpadlib is installed.

This uses the approach suggested by Colin Watson in LP: #253119.

I don't think it would be reasonable to add python3-launchpadlib to the hard dependency list: it would pull it and its dependencies into the default install, which is an additional 3269kB on Xenial.

Therefore, I only set python3-launchpadlib as a suggest dependency and I made the whole feature optional: if launchpadlib is not installed, nothing change except for a new warning in logs to suggest to install the library.

[Brian Murray (brian-murray)]

Thanks for working on this, I appreciate it a lot. Generally, this looks good to me, but I have some comments which you'll find in line. You might also want to add a changelog entry to debian/changelog.

[Colin Watson (cjwatson)]

I would honestly just depend on python3-launchpadlib. Yes, we don't have anything in the desktop requiring it right now, but that isn't a strong reason not to add it; and without it, most people will continue not to see PPA changelogs, which seems more compelling to me.

Other than that, this seems like a good start and the use of the LP API seems mostly right, but there are a few details to fix up. Thanks!

review: Needs Fixing

[Nicolas Delvaux (malizor)]

Thanks for your comments so far.
I will try to adress them as soon as I'm back in front of a computer, which should be next week.

Le 15/06/16 02:34 Colin Watson a écrit :

Review: Needs Fixing

I would honestly just depend on python3-launchpadlib. Yes, we don't have anything in the desktop requiring it right now, but that isn't a strong reason not to add it; and without it, most people will continue not to see PPA changelogs, which seems more compelling to me.

Other than that, this seems like a good start and the use of the LP API seems mostly right, but there are a few details to fix up. Thanks!

Diff comments:

> === modified file 'UpdateManager/Core/MyCache.py'
> --- UpdateManager/Core/MyCache.py 2014-06-26 07:03:08 +0000
> +++ UpdateManager/Core/MyCache.py 2016-06-10 23:11:06 +0000
> @@ -268,6 +274,52 @@
> alllines = alllines + line
> return alllines
>
> + def _extract_ppa_changelog_uri(self, name):
> + """Return the changelog URI from the Launchpad API
> +
> + Return None in case of an error.
> + """
> + if Launchpad is None:
> + logging.warning("Please install 'python3-launchpadlib' to enable "
> + "changelog retrieval for PPAs.")
> + return
> + cdt = self[name].candidate
> + for uri in cdt.uris:
> + match = re.search('http.*/(.*)/(.*)/ubuntu/.*', uri)

This should use urllib.parse as the first step, and should only do anything if the hostname is ppa.launchpad.net.

> + if match is not None:
> + user, ppa = match.group(1), match.group(2)
> + break
> + else:
> + logging.error('Unable to extract the changelog from the PPA')
> + return
> +
> + # Login on launchpad if we are not already
> + if self.launchpad is None:
> + try:
> + self.launchpad = Launchpad.login_anonymously('update-manager',
> + 'production',
> + version='devel')
> + except:

Never use bare except. To catch everything that's reasonable to catch in most cases, use "except Exception:".

> + logging.exception("Unable to connect to Launchpad to retrieve "
> + "the changelog")
> + return
> +
> + archive = self.launchpad.archives.getByReference(
> + reference='~%s/ubuntu/%s' % (user, ppa)
> + )
> + if archive is None:
> + logging.error('Unable to extract the changelog from the PPA')
> + return
> +
> + spph = archive.getPublishedSources(source_name=cdt.source_name,
> + exact_match=True,
> + version=cdt.version)

It would be more conventional to use "spphs" given that this is plural.

> + if not spph:
> + logging.error('Unable to extract the changelog from the PPA')
> + return
> +
> + return spph[0].changelogUrl()

Why would a KeyError be relevant? Testing that the collection is non-empty should be enough to ensure that spphs[0] works, and there's no reason to suppose that .changelogUrl() will raise a KeyError. It may be worth catching HTTP exceptions (I forget the exact details), though, as temporary Launchpad outages shouldn't cause update-manager to explode.

> +
> def _guess_third_party_changelogs_uri_by_source(self, name):
> pkg = self[name]
> deb_uri = pkg.candidate.uri
> @@ -314,14 +366,21 @@
> if news:
> self.all_news[name] = news
>
> - def _fetch_changelog_for_third_party_package(self, name):
> + def _fetch_changelog_for_third_party_package(self, name, ori...

[lp:~malizor/update-manager/ppa-changelogs]

2729. By Nicolas Delvaux on 2016-06-19

Attempt to retrieve Changelogs from PPA sources (LP: #253119)

2730. By Nicolas Delvaux on 2016-06-19

Add a new entry to debian/changelog

[Nicolas Delvaux (malizor)]

I fixed what you suggested:

* Move the try/except block to the calling function, to catch all API errors
* Different messages for different errors (and end them with periods!)
* Check that candidate URI hostname matches ppa.launchpad.net
* Do not use bare excepts
* Use "spphs" instead of the singular form
* Add a missing break
* Add an entry in debian/changelog

[Brian Murray (brian-murray)]

Thanks for making those fixes. I have one more question in-line.

=== modified file 'UpdateManager/Core/MyCache.py'
--- UpdateManager/Core/MyCache.py	2014-06-26 07:03:08 +0000
+++ UpdateManager/Core/MyCache.py	2016-06-19 21:14:49 +0000
@@ -43,6 +43,7 @@
 import re
 import DistUpgrade.DistUpgradeCache
 from gettext import gettext as _
+from launchpadlib.launchpad import Launchpad
 
 SYNAPTIC_PINFILE = "/var/lib/synaptic/preferences"
 CHANGELOGS_POOL = "http://changelogs.ubuntu.com/changelogs/pool/"
@@ -79,6 +80,7 @@
             self.saveDistUpgrade()
         assert (self._depcache.broken_count == 0 and
                 self._depcache.del_count == 0)
+        self.launchpad = None
 
     def _dpkgJournalDirty(self):
         """
@@ -222,7 +224,7 @@
         # and so its possible to do a man-in-the-middle attack to steal the
         # credentials
         res = urlsplit(uri)
-        if res.scheme == "https" and res.username != "":
+        if res.scheme == "https" and res.username:
             raise HttpsChangelogsUnsupportedError(
                 "https locations with username/password are not"
                 "supported to fetch changelogs")
@@ -268,6 +270,48 @@
             alllines = alllines + line
         return alllines
 
+    def _extract_ppa_changelog_uri(self, name):
+        """Return the changelog URI from the Launchpad API
+
+        Return None in case of an error.
+        """
+        cdt = self[name].candidate
+        for uri in cdt.uris:
+            if urlsplit(uri).hostname != 'ppa.launchpad.net':
+                continue
+            match = re.search('http.*/(.*)/(.*)/ubuntu/.*', uri)
+            if match is not None:
+                user, ppa = match.group(1), match.group(2)
+                break
+        else:
+            logging.error("Unable to find a valid PPA candidate URL.")
+            return
+
+        # Login on launchpad if we are not already
+        if self.launchpad is None:
+            self.launchpad = Launchpad.login_anonymously('update-manager',
+                                                         'production',
+                                                         version='devel')
+
+
+        archive = self.launchpad.archives.getByReference(
+            reference='~%s/ubuntu/%s' % (user, ppa)
+        )
+        if archive is None:
+            logging.error("Unable to retrieve the archive from the Launchpad "
+                          "API.")
+            return
+
+        spphs = archive.getPublishedSources(source_name=cdt.source_name,
+                                            exact_match=True,
+                                            version=cdt.version)
+        if not spphs:
+            logging.error("No published sources were retrieved from the "
+                          "Launchpad API.")
+            return
+
+        return spphs[0].changelogUrl()
+
     def _guess_third_party_changelogs_uri_by_source(self, name):
         pkg = self[name]
         deb_uri = pkg.candidate.uri
@@ -314,14 +358,25 @@
         if news:
             self.all_news[name] = news
 
-    def _fetch_changelog_for_third_party_package(self, name):
+    def _fetch_changelog_for_third_party_package(self, name, origins):
+        # Special case for PPAs
+        changelogs_uri_ppa = None
+        for origin in origins:
+            if origin.origin.startswith('LP-PPA-'):
+                try:
+                    changelogs_uri_ppa = self._extract_ppa_changelog_uri(name)
+                    break
+                except Exception:
+                    logging.exception("Unable to connect to the Launchpad API.")
         # Try non official changelog location
         changelogs_uri_binary = \
             self._guess_third_party_changelogs_uri_by_binary(name)
         changelogs_uri_source = \
             self._guess_third_party_changelogs_uri_by_source(name)
         error_message = ""
-        for changelogs_uri in [changelogs_uri_binary, changelogs_uri_source]:
+        for changelogs_uri in [changelogs_uri_ppa,
+                               changelogs_uri_binary,
+                               changelogs_uri_source]:
             if changelogs_uri:
                 try:
                     changelog = self._get_changelog_or_news(
@@ -349,7 +404,7 @@
             (name, getattr(self[name].installed, "version", None),
              self[name].candidate.version)
         if self.CHANGELOG_ORIGIN not in [o.origin for o in origins]:
-            self._fetch_changelog_for_third_party_package(name)
+            self._fetch_changelog_for_third_party_package(name, origins)
             return
         # fixup epoch handling version
         srcpkg = self[name].candidate.source_name

=== modified file 'debian/changelog'
--- debian/changelog	2016-05-10 14:32:42 +0000
+++ debian/changelog	2016-06-19 21:14:49 +0000
@@ -1,3 +1,10 @@
+update-manager (1:16.10.2) UNRELEASED; urgency=medium
+
+  * Attempt to retrieve Changelogs from PPA sources (LP: #253119)
+  * Correctly detect the usage of a username in changelog URIs
+
+ -- Nicolas Delvaux <[e-mail]>  Sun, 19 Jun 2016 23:12:22 +0200
+
 update-manager (1:16.10.1) yakkety; urgency=medium
 
   * Correctly calculate the end of support, and return correctly when support

=== modified file 'debian/control'
--- debian/control	2016-04-06 22:38:57 +0000
+++ debian/control	2016-06-19 21:14:49 +0000
@@ -41,6 +41,7 @@
          ${misc:Depends},
          python3-apt (>= 0.8.5~),
          python3-distupgrade,
+         python3-launchpadlib,
          lsb-release
 Description: python 3.x module for update-manager
  Python module for update-manager (UpdateManager).