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

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.

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!

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...

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

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