Software Updater

Merge lp:~rbalint/update-manager/fix-stale-pkg-refs into lp:update-manager

  1. fix-stale-pkg-refs
  2. Merge into main
Proposed by Balint Reczey
Status: Rejected
Rejected by: Balint Reczey
Proposed branch: lp:~rbalint/update-manager/fix-stale-pkg-refs
Merge into: lp:update-manager
Diff against target: 336 lines (+76/-48)
4 files modified
UpdateManager/Core/UpdateList.py (+37/-21)
UpdateManager/UpdatesAvailable.py (+17/-17)
debian/changelog (+11/-0)
tests/test_update_list.py (+11/-10)
To merge this branch: bzr merge lp:~rbalint/update-manager/fix-stale-pkg-refs
Reviewer Review Type Date Requested Status
Brian Murray Approve
Julian Andres Klode Pending
Ubuntu Core Development Team Pending
Review via email: mp+347195@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Balint Reczey (rbalint) wrote :

I'm also checking rdeps if they need update to adapt to this change.

Revision history for this message
Balint Reczey (rbalint) wrote :

I considered replacing pkg with a proxy object, but the code would have been more confusing imo.

Revision history for this message
Brian Murray (brian-murray) :
review: Approve
Revision history for this message
Julian Andres Klode (juliank) wrote :

So, does this work reliably if a package disappears between stages? I'm not sure it can even haooen.

Revision history for this message
Balint Reczey (rbalint) wrote :

It crashes with exception when a package is going away like before but this is very rare and is not a regression. We can fix that later.

Revision history for this message
Balint Reczey (rbalint) wrote :

Julian provided and alternative fix that updates referenced pkg objects upon cache.open() which is fairly fast and would fix the issue in a different way.

Unmerged revisions

2824. By Balint Reczey

Update changelog

2823. By Balint Reczey

Store package name and cache reference in UpdateItem to avoid invalid package references

LP: #1773316

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'UpdateManager/Core/UpdateList.py'
2--- UpdateManager/Core/UpdateList.py 2018-03-21 17:41:10 +0000
3+++ UpdateManager/Core/UpdateList.py 2018-05-31 12:03:14 +0000
4@@ -43,29 +43,42 @@
5
6
7 class UpdateItem():
8- def __init__(self, pkg, name, icon, to_remove):
9+ def __init__(self, pkg, cache, name, icon, to_remove):
10 self.icon = icon
11 self.name = name
12- self.pkg = pkg
13+ self.cache = cache
14+ if pkg is not None:
15+ self.pkg_name = pkg.name
16+ else:
17+ self.pkg_name = None
18 self.to_remove = to_remove
19
20 def is_selected(self):
21 if not self.to_remove:
22- return self.pkg.marked_install or self.pkg.marked_upgrade
23+ return (self.get_pkg().marked_install or
24+ self.get_pkg().marked_upgrade)
25 else:
26- return self.pkg.marked_delete
27+ return self.get_pkg().marked_delete
28+
29+ def get_pkg(self):
30+ if self.cache is None:
31+ return None
32+ try:
33+ return self.cache[self.pkg_name]
34+ except KeyError:
35+ return None
36
37
38 class UpdateGroup(UpdateItem):
39 _depcache = {}
40
41- def __init__(self, pkg, name, icon, to_remove):
42- UpdateItem.__init__(self, pkg, name, icon, to_remove)
43+ def __init__(self, pkg, cache, name, icon, to_remove):
44+ UpdateItem.__init__(self, pkg, cache, name, icon, to_remove)
45 self._items = set()
46 self._deps = set()
47 self.core_item = None
48 if pkg is not None:
49- self.core_item = UpdateItem(pkg, name, icon, to_remove)
50+ self.core_item = UpdateItem(pkg, cache, name, icon, to_remove)
51 self._items.add(self.core_item)
52
53 @property
54@@ -77,7 +90,7 @@
55 def add(self, pkg, cache=None, eventloop_callback=None, to_remove=False):
56 name = utils.get_package_label(pkg)
57 icon = Gio.ThemedIcon.new("package")
58- self._items.add(UpdateItem(pkg, name, icon, to_remove))
59+ self._items.add(UpdateItem(pkg, cache, name, icon, to_remove))
60 # If the pkg is in self._deps, stop here. We have already calculated
61 # the recursive dependencies for this package, no need to do it again.
62 if cache and pkg.name in cache and pkg.name not in self._deps:
63@@ -93,8 +106,8 @@
64
65 def _init_deps(self, cache, eventloop_callback):
66 for item in self._items:
67- if item.pkg and item.pkg.name not in self._deps:
68- self._add_deps(item.pkg, cache, eventloop_callback)
69+ if item.pkg_name is not None and item.pkg_name not in self._deps:
70+ self._add_deps(item.get_pkg(), cache, eventloop_callback)
71
72 def _add_deps(self, pkg, cache, eventloop_callback):
73 """Adds pkg and dependencies of pkg to the dependency list."""
74@@ -148,23 +161,24 @@
75 return 0
76 size = 0
77 for item in self.items:
78- size += getattr(item.pkg.candidate, "size", 0)
79+ size += getattr(item.get_pkg().candidate, "size", 0)
80 return size
81
82
83 class UpdateApplicationGroup(UpdateGroup):
84- def __init__(self, pkg, application, to_remove):
85+ def __init__(self, pkg, cache, application, to_remove):
86 name = application.get_display_name()
87 icon = application.get_icon()
88- super(UpdateApplicationGroup, self).__init__(pkg, name, icon,
89+ super(UpdateApplicationGroup, self).__init__(pkg, cache, name, icon,
90 to_remove)
91
92
93 class UpdatePackageGroup(UpdateGroup):
94- def __init__(self, pkg, to_remove):
95+ def __init__(self, pkg, cache, to_remove):
96 name = utils.get_package_label(pkg)
97 icon = Gio.ThemedIcon.new("package")
98- super(UpdatePackageGroup, self).__init__(pkg, name, icon, to_remove)
99+ super(UpdatePackageGroup, self).__init__(pkg, cache, name, icon,
100+ to_remove)
101
102
103 class UpdateSystemGroup(UpdateGroup):
104@@ -173,7 +187,8 @@
105 # the core components and packages.
106 name = _("%s base") % utils.get_ubuntu_flavor_name(cache=cache)
107 icon = Gio.ThemedIcon.new("distributor-logo")
108- super(UpdateSystemGroup, self).__init__(None, name, icon, to_remove)
109+ super(UpdateSystemGroup, self).__init__(None, None, name, icon,
110+ to_remove)
111
112
113 class UpdateOrigin():
114@@ -429,7 +444,7 @@
115 for pkg in pkgs:
116 app = self._get_application_for_package(pkg)
117 if app is not None:
118- app_group = UpdateApplicationGroup(pkg, app, to_remove)
119+ app_group = UpdateApplicationGroup(pkg, cache, app, to_remove)
120 app_groups.append(app_group)
121 else:
122 ungrouped_pkgs.append(pkg)
123@@ -450,20 +465,21 @@
124 if ungrouped_pkgs:
125 # Separate out system base packages. If we have already found an
126 # application for all updates, don't bother.
127- meta_group = UpdateGroup(None, None, None, to_remove)
128+ meta_group = UpdateGroup(None, None, None, None, to_remove)
129 flavor_package = utils.get_ubuntu_flavor_package(cache=cache)
130 meta_pkgs = [flavor_package, "ubuntu-standard", "ubuntu-minimal"]
131 meta_pkgs.extend(self._get_linux_packages())
132 for pkg in meta_pkgs:
133 if pkg in cache:
134- meta_group.add(cache[pkg])
135+ meta_group.add(cache[pkg], cache)
136 for pkg in ungrouped_pkgs:
137 if meta_group.is_dependency(pkg, cache, eventloop_callback):
138 if system_group is None:
139 system_group = UpdateSystemGroup(cache, to_remove)
140- system_group.add(pkg)
141+ system_group.add(pkg, cache)
142 else:
143- pkg_groups.append(UpdatePackageGroup(pkg, to_remove))
144+ pkg_groups.append(UpdatePackageGroup(pkg, cache,
145+ to_remove))
146
147 app_groups.sort(key=lambda a: a.name.lower())
148 pkg_groups.sort(key=lambda a: a.name.lower())
149
150=== modified file 'UpdateManager/UpdatesAvailable.py'
151--- UpdateManager/UpdatesAvailable.py 2018-03-18 21:43:58 +0000
152+++ UpdateManager/UpdatesAvailable.py 2018-05-31 12:03:14 +0000
153@@ -407,14 +407,13 @@
154 path = model.get_path(iter)
155
156 requires_restart = False
157- if data.item and data.item.pkg:
158- requires_restart = self.pkg_requires_restart(data.item.pkg)
159+ if data.item:
160+ requires_restart = self.pkg_requires_restart(data.item.get_pkg())
161 elif data.group:
162 if not self.treeview_update.row_expanded(path):
163 # A package in the group requires restart
164 for group_item in data.group.items:
165- if group_item.pkg and self.pkg_requires_restart(
166- group_item.pkg):
167+ if self.pkg_requires_restart(group_item.get_pkg()):
168 requires_restart = True
169 break
170
171@@ -431,7 +430,7 @@
172 activatable = False
173 inconsistent = False
174 if data.item:
175- activatable = data.item.pkg.name not in self.list.held_back
176+ activatable = data.item.pkg_name not in self.list.held_back
177 inconsistent = False
178 elif data.group:
179 activatable = True
180@@ -534,16 +533,16 @@
181 if (item is None and data.group is not None and
182 data.group.core_item is not None):
183 item = data.group.core_item
184- if (item is None or item.pkg is None or
185- item.pkg.candidate is None or
186- item.pkg.candidate.description is None):
187+ pkg = None if item is None else item.get_pkg()
188+ if pkg is None or pkg.candidate is None or \
189+ pkg.candidate.description is None:
190 changes_buffer = self.textview_changes.get_buffer()
191 changes_buffer.set_text("")
192 desc_buffer = self.textview_descr.get_buffer()
193 desc_buffer.set_text("")
194 self.notebook_details.set_sensitive(False)
195 return
196- long_desc = item.pkg.candidate.description
197+ long_desc = pkg.candidate.description
198 self.notebook_details.set_sensitive(True)
199 # do some regular expression magic on the description
200 # Add a newline before each bullet
201@@ -560,7 +559,7 @@
202 desc_buffer.set_text(long_desc)
203
204 # now do the changelog
205- name = item.pkg.name
206+ name = item.pkg_name
207 if name is None:
208 return
209
210@@ -713,9 +712,9 @@
211 return
212 pkg = None
213 if data.item:
214- pkg = data.item.pkg
215+ pkg = data.item.get_pkg()
216 elif data.group and data.group.core_item:
217- pkg = data.group.core_item.pkg
218+ pkg = data.group.core_item.get_pkg()
219 if pkg and self.pkg_requires_restart(pkg):
220 nonlocal requires_restart
221 requires_restart = True
222@@ -953,12 +952,12 @@
223 for item in items:
224 try:
225 if keep_packages:
226- item.pkg.mark_keep()
227- elif item.pkg.name not in self.list.held_back:
228+ item.get_pkg().mark_keep()
229+ elif item.pkg_name not in self.list.held_back:
230 if not item.to_remove:
231- item.pkg.mark_install()
232+ item.get_pkg().mark_install()
233 else:
234- item.pkg.mark_delete()
235+ item.get_pkg().mark_delete()
236 except SystemError:
237 pass
238
239@@ -1030,7 +1029,8 @@
240 item_row = [
241 item.name,
242 UpdateData(None, None, item),
243- humanize_size(getattr(item.pkg.candidate, "size", 0)),
244+ humanize_size(getattr(item.get_pkg().candidate,
245+ "size", 0)),
246 True
247 ]
248 self.store.append(group_iter, item_row)
249
250=== modified file 'debian/changelog'
251--- debian/changelog 2018-05-28 14:07:41 +0000
252+++ debian/changelog 2018-05-31 12:03:14 +0000
253@@ -1,3 +1,14 @@
254+update-manager (1:18.10.2) UNRELEASED; urgency=medium
255+
256+ [ Steve Langasek ]
257+ * Fix code comment to document the actual proxy behavior, which is correct.
258+
259+ [ Balint Reczey ]
260+ * Store package name and cache reference in UpdateItem to avoid stale
261+ package references (LP: #1773316)
262+
263+ -- Balint Reczey <rbalint@ubuntu.com> Thu, 31 May 2018 13:59:25 +0200
264+
265 update-manager (1:18.10.1) cosmic; urgency=medium
266
267 * Block style context changed signal while enforcing the main window's
268
269=== modified file 'tests/test_update_list.py'
270--- tests/test_update_list.py 2017-08-07 19:42:06 +0000
271+++ tests/test_update_list.py 2018-05-31 12:03:14 +0000
272@@ -59,7 +59,8 @@
273 ignored_srcs = set([pkg.candidate.source_name for pkg in
274 self.updates_list.ignored_phased_updates])
275 group = self.updates_list.update_groups[0]
276- install_srcs = set([x.pkg.candidate.source_name for x in group.items])
277+ install_srcs = set(
278+ [x.get_pkg().candidate.source_name for x in group.items])
279 self.assertEqual(ignored_srcs, set({'zsh'}))
280 self.assertEqual(install_srcs, set({'apt'}))
281 self.assertTrue(len(ignored_srcs & install_srcs) == 0)
282@@ -142,8 +143,8 @@
283 group = self.updates_list.update_groups[0]
284 self.assertIsInstance(group, UpdateList.UpdateApplicationGroup)
285 self.assertIsNotNone(group.core_item)
286- self.assertEqual(group.core_item.pkg.name, 'installed-app')
287- self.assertListEqual([x.pkg.name for x in group.items],
288+ self.assertEqual(group.core_item.pkg_name, 'installed-app')
289+ self.assertListEqual([x.pkg_name for x in group.items],
290 ['installed-app'])
291
292 def test_app_with_subitems(self):
293@@ -151,9 +152,9 @@
294 group = self.updates_list.update_groups[1]
295 self.assertIsInstance(group, UpdateList.UpdateApplicationGroup)
296 self.assertIsNotNone(group.core_item)
297- self.assertEqual(group.core_item.pkg.name,
298+ self.assertEqual(group.core_item.pkg_name,
299 'installed-app-with-subitems')
300- self.assertListEqual([x.pkg.name for x in group.items],
301+ self.assertListEqual([x.pkg_name for x in group.items],
302 ['installed-app-with-subitems',
303 'installed-pkg-single-dep'])
304
305@@ -162,8 +163,8 @@
306 group = self.updates_list.update_groups[2]
307 self.assertIsInstance(group, UpdateList.UpdatePackageGroup)
308 self.assertIsNotNone(group.core_item)
309- self.assertEqual(group.core_item.pkg.name, 'installed-pkg')
310- self.assertListEqual([x.pkg.name for x in group.items],
311+ self.assertEqual(group.core_item.pkg_name, 'installed-pkg')
312+ self.assertListEqual([x.pkg_name for x in group.items],
313 ['installed-pkg'])
314
315 def test_pkg_multiple_deps(self):
316@@ -171,9 +172,9 @@
317 group = self.updates_list.update_groups[3]
318 self.assertIsInstance(group, UpdateList.UpdatePackageGroup)
319 self.assertIsNotNone(group.core_item)
320- self.assertEqual(group.core_item.pkg.name,
321+ self.assertEqual(group.core_item.pkg_name,
322 'installed-pkg-multiple-deps')
323- self.assertListEqual([x.pkg.name for x in group.items],
324+ self.assertListEqual([x.pkg_name for x in group.items],
325 ['installed-pkg-multiple-deps'])
326
327 def test_security(self):
328@@ -181,7 +182,7 @@
329 group = self.updates_list.security_groups[0]
330 self.assertIsInstance(group, UpdateList.UpdateSystemGroup)
331 self.assertIsNone(group.core_item)
332- self.assertListEqual([x.pkg.name for x in group.items], ['base-pkg'])
333+ self.assertListEqual([x.pkg_name for x in group.items], ['base-pkg'])
334
335
336 if __name__ == "__main__":

Subscribers

People subscribed via source and target branches

to status/vote changes: