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

Proposed by Nicolas Delvaux on 2016-06-10
Status: Merged
Merged at revision: 2730
Proposed branch: lp:~malizor/update-manager/ppa-changelogs
Merge into: lp:update-manager
Diff against target: 141 lines (+67/-4)
3 files modified
UpdateManager/Core/MyCache.py (+59/-4)
debian/changelog (+7/-0)
debian/control (+1/-0)
To merge this branch: bzr merge lp:~malizor/update-manager/ppa-changelogs
Reviewer Review Type Date Requested Status
Colin Watson 2016-06-10 Needs Fixing on 2016-06-15
Review via email: mp+297119@code.launchpad.net

Commit message

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

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

Description of the change

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.

To post a comment you must log in.
Brian Murray (brian-murray) wrote :

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) wrote :

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) wrote :
Download full text (4.0 KiB)

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

Read more...

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) wrote :

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) wrote :

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

Colin Watson (cjwatson) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'UpdateManager/Core/MyCache.py'
2--- UpdateManager/Core/MyCache.py 2014-06-26 07:03:08 +0000
3+++ UpdateManager/Core/MyCache.py 2016-06-19 21:14:49 +0000
4@@ -43,6 +43,7 @@
5 import re
6 import DistUpgrade.DistUpgradeCache
7 from gettext import gettext as _
8+from launchpadlib.launchpad import Launchpad
9
10 SYNAPTIC_PINFILE = "/var/lib/synaptic/preferences"
11 CHANGELOGS_POOL = "http://changelogs.ubuntu.com/changelogs/pool/"
12@@ -79,6 +80,7 @@
13 self.saveDistUpgrade()
14 assert (self._depcache.broken_count == 0 and
15 self._depcache.del_count == 0)
16+ self.launchpad = None
17
18 def _dpkgJournalDirty(self):
19 """
20@@ -222,7 +224,7 @@
21 # and so its possible to do a man-in-the-middle attack to steal the
22 # credentials
23 res = urlsplit(uri)
24- if res.scheme == "https" and res.username != "":
25+ if res.scheme == "https" and res.username:
26 raise HttpsChangelogsUnsupportedError(
27 "https locations with username/password are not"
28 "supported to fetch changelogs")
29@@ -268,6 +270,48 @@
30 alllines = alllines + line
31 return alllines
32
33+ def _extract_ppa_changelog_uri(self, name):
34+ """Return the changelog URI from the Launchpad API
35+
36+ Return None in case of an error.
37+ """
38+ cdt = self[name].candidate
39+ for uri in cdt.uris:
40+ if urlsplit(uri).hostname != 'ppa.launchpad.net':
41+ continue
42+ match = re.search('http.*/(.*)/(.*)/ubuntu/.*', uri)
43+ if match is not None:
44+ user, ppa = match.group(1), match.group(2)
45+ break
46+ else:
47+ logging.error("Unable to find a valid PPA candidate URL.")
48+ return
49+
50+ # Login on launchpad if we are not already
51+ if self.launchpad is None:
52+ self.launchpad = Launchpad.login_anonymously('update-manager',
53+ 'production',
54+ version='devel')
55+
56+
57+ archive = self.launchpad.archives.getByReference(
58+ reference='~%s/ubuntu/%s' % (user, ppa)
59+ )
60+ if archive is None:
61+ logging.error("Unable to retrieve the archive from the Launchpad "
62+ "API.")
63+ return
64+
65+ spphs = archive.getPublishedSources(source_name=cdt.source_name,
66+ exact_match=True,
67+ version=cdt.version)
68+ if not spphs:
69+ logging.error("No published sources were retrieved from the "
70+ "Launchpad API.")
71+ return
72+
73+ return spphs[0].changelogUrl()
74+
75 def _guess_third_party_changelogs_uri_by_source(self, name):
76 pkg = self[name]
77 deb_uri = pkg.candidate.uri
78@@ -314,14 +358,25 @@
79 if news:
80 self.all_news[name] = news
81
82- def _fetch_changelog_for_third_party_package(self, name):
83+ def _fetch_changelog_for_third_party_package(self, name, origins):
84+ # Special case for PPAs
85+ changelogs_uri_ppa = None
86+ for origin in origins:
87+ if origin.origin.startswith('LP-PPA-'):
88+ try:
89+ changelogs_uri_ppa = self._extract_ppa_changelog_uri(name)
90+ break
91+ except Exception:
92+ logging.exception("Unable to connect to the Launchpad API.")
93 # Try non official changelog location
94 changelogs_uri_binary = \
95 self._guess_third_party_changelogs_uri_by_binary(name)
96 changelogs_uri_source = \
97 self._guess_third_party_changelogs_uri_by_source(name)
98 error_message = ""
99- for changelogs_uri in [changelogs_uri_binary, changelogs_uri_source]:
100+ for changelogs_uri in [changelogs_uri_ppa,
101+ changelogs_uri_binary,
102+ changelogs_uri_source]:
103 if changelogs_uri:
104 try:
105 changelog = self._get_changelog_or_news(
106@@ -349,7 +404,7 @@
107 (name, getattr(self[name].installed, "version", None),
108 self[name].candidate.version)
109 if self.CHANGELOG_ORIGIN not in [o.origin for o in origins]:
110- self._fetch_changelog_for_third_party_package(name)
111+ self._fetch_changelog_for_third_party_package(name, origins)
112 return
113 # fixup epoch handling version
114 srcpkg = self[name].candidate.source_name
115
116=== modified file 'debian/changelog'
117--- debian/changelog 2016-05-10 14:32:42 +0000
118+++ debian/changelog 2016-06-19 21:14:49 +0000
119@@ -1,3 +1,10 @@
120+update-manager (1:16.10.2) UNRELEASED; urgency=medium
121+
122+ * Attempt to retrieve Changelogs from PPA sources (LP: #253119)
123+ * Correctly detect the usage of a username in changelog URIs
124+
125+ -- Nicolas Delvaux <contact@nicolas-delvaux.org> Sun, 19 Jun 2016 23:12:22 +0200
126+
127 update-manager (1:16.10.1) yakkety; urgency=medium
128
129 * Correctly calculate the end of support, and return correctly when support
130
131=== modified file 'debian/control'
132--- debian/control 2016-04-06 22:38:57 +0000
133+++ debian/control 2016-06-19 21:14:49 +0000
134@@ -41,6 +41,7 @@
135 ${misc:Depends},
136 python3-apt (>= 0.8.5~),
137 python3-distupgrade,
138+ python3-launchpadlib,
139 lsb-release
140 Description: python 3.x module for update-manager
141 Python module for update-manager (UpdateManager).

Subscribers

People subscribed via source and target branches

to status/vote changes: