Merge lp:~marcustomlinson/update-manager/update-manager into lp:update-manager

Proposed by Marcus Tomlinson on 2020-03-01
Status: Merged
Approved by: Iain Lane on 2020-04-06
Approved revision: 2865
Merged at revision: 2865
Proposed branch: lp:~marcustomlinson/update-manager/update-manager
Merge into: lp:update-manager
Diff against target: 357 lines (+212/-8)
5 files modified
UpdateManager/Core/UpdateList.py (+11/-2)
UpdateManager/UpdateManager.py (+5/-2)
UpdateManager/UpdatesAvailable.py (+9/-2)
UpdateManager/backend/__init__.py (+180/-2)
debian/changelog (+7/-0)
To merge this branch: bzr merge lp:~marcustomlinson/update-manager/update-manager
Reviewer Review Type Date Requested Status
Iain Lane 2020-03-02 Approve on 2020-04-06
Ubuntu Core Development Team 2020-03-01 Pending
Review via email: mp+380060@code.launchpad.net

Commit message

UpdateManager/backend/__init__.py: once apt upgrade completes, use the deb2snap.json file from ubuntu-release-upgrader to perform snap updates.

To post a comment you must log in.
Iain Lane (laney) wrote :

Free-form thoughts

  - This causes polkit prompts from snapd, is that acceptable?
  - I'm not sure that putting it after apt updating and before apt installing is right - what if we:
    - update ubuntu-release-upgrader-core (new transitions to know about)
    - Change the seeds (things get marked as removable that weren't before)
    ?
  - Why is there a spawned thread and how is it supposed to be waited on? (Could be fine, I just can't tell)
  - Would probably be better to use snapd-glib

I'll probably follow up with more later!

Iain Lane (laney) :
review: Needs Fixing
Iain Lane (laney) wrote :

I also do not see where update-manager performs autoremoval; does that actually exist? If not, we would need to do the apt removal here too (we already identified the package and that it's autoremovable, so should be possible).

Marcus Tomlinson (marcustomlinson) wrote :

Hey Iain, feeling a bit ill so forgive me if I ramble or make no sense anywhere. I'll try explain myself as best as I can.

The reason I'm doing the transitions immediately after update is to ensure everyone gets them, even apt users. I, for example, will usually just close update-manager whenever it prompts and perform my updates via terminal when I feel like it. Transitioning before the prompt is - granted - a bit of a hack to force everyone to get them (I don't know who wouldn't want them really).

I don't actually see the autoremoval as part of this logic. Once we update the seeds on desktop-minimal that propagates to the deb being marked for removal and thus update-manager and apt do their thing as usual. This is why I can do the cache[deb].marked_delete check the way I am, as that is my indication that the deb has been unseeded, is currently installed, and should be removed.

Iain Lane (laney) wrote :

"update-manager [...] do their thing as usual"

Where is that in update-manager though? It could happen, but I cannot find the code for it - except for kernel autoremovals.

Not commenting on the rest for now.

Sebastien Bacher (seb128) wrote :

> I feel like it. Transitioning before the prompt is -
> granted - a bit of a hack to force everyone to get them
> (I don't know who wouldn't want them really).

Commenting just on that but I think that's a weird logic/user experience, we shouldn't get out of our way doing weird UI just to force technical users to be force migrated. Those who close update-manager and go to the command line should be able to transition between snap&deb by themselve

Ken VanDine (ken-vandine) wrote :

>
> The reason I'm doing the transitions immediately after update is to ensure
> everyone gets them, even apt users. I, for example, will usually just close
> update-manager whenever it prompts and perform my updates via terminal when I
> feel like it. Transitioning before the prompt is - granted - a bit of a hack
> to force everyone to get them (I don't know who wouldn't want them really).

I don't think we should actually do the transition before the user actually clicks on the button in the UI. Doing it without the user interaction there feels wrong to me.

Marcus Tomlinson (marcustomlinson) wrote :

Ok I misunderstood what we were aiming for here. Apologies.

Steve Langasek (vorlon) wrote :

> I also do not see where update-manager performs autoremoval;
> does that actually exist? If not, we would need to do the apt
> removal here too (we already identified the package and that
> it's autoremovable, so should be possible).

We should not, as a policy, do autoremovals of other debs as part of an ordinary update-manager run because we don't know why the package has become autoremovable and this can significantly impact the usability of the user's system.

We do autoremovals only on upgrade between releases, because that's a significant event where the possibility of some fallout is to be expected.

Iain Lane (laney) wrote :

On Wed, Mar 18, 2020 at 03:08:13PM -0000, Steve Langasek wrote:
> > I also do not see where update-manager performs autoremoval;
> > does that actually exist? If not, we would need to do the apt
> > removal here too (we already identified the package and that
> > it's autoremovable, so should be possible).
>
> We should not, as a policy, do autoremovals of other debs as part of an ordinary update-manager run because we don't know why the package has become autoremovable and this can significantly impact the usability of the user's system.

This is specifically limited to cases where we know a deb has been
replaced by a snap.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Iain Lane (laney) wrote :

I'll look into the requested changes.

Iain Lane (laney) wrote :

> I'll look into the requested changes.

Struggling to find time for this :(

Marcus Tomlinson (marcustomlinson) wrote :

Alright, changes made:

- I've split the large update_snaps() function into smaller parts.
- Snap updates now occur _after_ the user has opted in to the updates.
- snapd-glib for (un)installs means just one polkit prompt now for the all snap updates.
- debs with a snap replacement that were not installed manually get marked for deletion.
- The thread is still there, sorry, though I'm not sure why it's a bad idea really.

Marcus Tomlinson (marcustomlinson) wrote :

Sorry, just pushed another update to fix the pycodestyle test.

Iain Lane (laney) wrote :

> - The thread is still there, sorry, though I'm not sure why it's a bad idea really.

Usual GLib style is to use async functions and make use of the main loop, then you can avoid caring about issues with thread safety. In this case that was more of a style thing / observation / comment, not a blocker for me though, so reviewing now. Thanks for the updates!

Marcus Tomlinson (marcustomlinson) wrote :

Bug updated to FFe and testing instructions added.

2865. By Marcus Tomlinson on 2020-04-06

UpdateManager/backend/__init__.py: once apt upgrade completes, use the deb2snap.json file from ubuntu-release-upgrader to perform snap updates.

Iain Lane (laney) wrote :

Thanks. I think this looks good to me now. I'm going to give it some more testing and will upload.

There are some review comments inline, nothing blocking.

One bit of feedback: I just got 'snap-store' installed thanks to this change. It took a couple of minutes to download though, and I was a bit worried that the process had stalled or broken somehow. I think it'd be reassuring if you could somehow output progress information, e.g. the amount that's been downloaded. I think this must be possible with snapd-glib, since gnome-software can do it.

review: Approve

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 2020-01-31 21:26:39 +0000
3+++ UpdateManager/Core/UpdateList.py 2020-04-06 11:32:18 +0000
4@@ -212,6 +212,7 @@
5 self.update_groups = []
6 self.security_groups = []
7 self.kernel_autoremove_groups = []
8+ self.duplicate_groups = []
9 self.num_updates = 0
10 self.random = random.Random()
11 self.ignored_phased_updates = []
12@@ -471,7 +472,7 @@
13
14 return app_groups + pkg_groups
15
16- def update(self, cache, eventloop_callback=None):
17+ def update(self, cache, eventloop_callback=None, duplicate_packages=[]):
18 self.held_back = []
19
20 # do the upgrade
21@@ -480,6 +481,7 @@
22 security_pkgs = []
23 upgrade_pkgs = []
24 kernel_autoremove_pkgs = []
25+ duplicate_pkgs = []
26
27 # Find all upgradable packages
28 for pkg in cache:
29@@ -516,18 +518,23 @@
30 and not cache.running_kernel_pkgs_regexp.match(
31 pkg.name))):
32 kernel_autoremove_pkgs.append(pkg)
33+ if (pkg.name in duplicate_packages):
34+ duplicate_pkgs.append(pkg)
35
36 # perform operations after the loop to not skip packages which
37 # changed state due to the resolver
38 for pkg in kernel_autoremove_pkgs:
39 pkg.mark_delete()
40+ for pkg in duplicate_pkgs:
41+ pkg.mark_delete()
42 for pkg in self.ignored_phased_updates:
43 pkg.mark_keep()
44
45 if security_pkgs or upgrade_pkgs:
46 # There's updates available. Initiate the desktop file cache.
47 pkg_names = [p.name for p in
48- security_pkgs + upgrade_pkgs + kernel_autoremove_pkgs]
49+ security_pkgs + upgrade_pkgs
50+ + kernel_autoremove_pkgs + duplicate_pkgs]
51 self._populate_desktop_cache(pkg_names)
52 self.update_groups = self._make_groups(cache, upgrade_pkgs,
53 eventloop_callback)
54@@ -535,3 +542,5 @@
55 eventloop_callback)
56 self.kernel_autoremove_groups = self._make_groups(
57 cache, kernel_autoremove_pkgs, eventloop_callback, True)
58+ self.duplicate_groups = self._make_groups(
59+ cache, duplicate_pkgs, eventloop_callback, True)
60
61=== modified file 'UpdateManager/UpdateManager.py'
62--- UpdateManager/UpdateManager.py 2019-04-08 17:00:30 +0000
63+++ UpdateManager/UpdateManager.py 2020-04-06 11:32:18 +0000
64@@ -86,6 +86,7 @@
65 self.update_list = None
66 self.meta_release = None
67 self.hwe_replacement_packages = None
68+ self.duplicate_packages = None
69
70 # Basic GTK+ parameters
71 self.set_title(_("Software Updater"))
72@@ -232,7 +233,8 @@
73 self._look_busy()
74 self.refresh_cache()
75
76- pane = self._make_available_pane(self.cache.install_count,
77+ pane = self._make_available_pane(self.cache.install_count
78+ + self.cache.del_count,
79 os.path.exists(REBOOT_REQUIRED_FILE),
80 cancelled_update, error_occurred)
81 self._start_pane(pane)
82@@ -387,7 +389,8 @@
83
84 self.update_list = UpdateList(self)
85 try:
86- self.update_list.update(self.cache, eventloop_callback=iterate)
87+ self.update_list.update(self.cache, eventloop_callback=iterate,
88+ duplicate_packages=self.duplicate_packages)
89 except SystemError as e:
90 header = _("Could not calculate the upgrade")
91 desc = _("An unresolvable problem occurred while "
92
93=== modified file 'UpdateManager/UpdatesAvailable.py'
94--- UpdateManager/UpdatesAvailable.py 2019-04-08 17:00:30 +0000
95+++ UpdateManager/UpdatesAvailable.py 2020-04-06 11:32:18 +0000
96@@ -1055,7 +1055,8 @@
97 self._add_groups(self.list.security_groups)
98 if self.list.security_groups and self.list.update_groups:
99 self._add_header(_("Other updates"), self.list.update_groups)
100- elif self.list.update_groups and self.list.kernel_autoremove_groups:
101+ elif self.list.update_groups and (self.list.kernel_autoremove_groups
102+ or self.list.duplicate_groups):
103 self._add_header(_("Updates"), self.list.update_groups)
104 if self.list.update_groups:
105 self._add_groups(self.list.update_groups)
106@@ -1064,11 +1065,17 @@
107 _("Unused kernel updates to be removed"),
108 self.list.kernel_autoremove_groups)
109 self._add_groups(self.list.kernel_autoremove_groups)
110+ if self.list.duplicate_groups:
111+ self._add_header(
112+ _("Duplicate packages to be removed"),
113+ self.list.duplicate_groups)
114+ self._add_groups(self.list.duplicate_groups)
115
116 self.treeview_update.set_model(self.store)
117 self.pkg_cell_area.indent_toplevel = (
118 bool(self.list.security_groups)
119- or bool(self.list.kernel_autoremove_groups))
120+ or bool(self.list.kernel_autoremove_groups)
121+ or bool(self.list.duplicate_groups))
122 self.update_close_button()
123 self.update_count()
124 self.setBusy(False)
125
126=== modified file 'UpdateManager/backend/__init__.py'
127--- UpdateManager/backend/__init__.py 2019-04-08 17:00:30 +0000
128+++ UpdateManager/backend/__init__.py 2020-04-06 11:32:18 +0000
129@@ -6,11 +6,20 @@
130
131 from __future__ import absolute_import
132
133-from gi.repository import GLib
134+import gi
135+gi.require_version('Snapd', '1')
136+from gi.repository import GLib, Gtk, Snapd
137
138 from apt import Cache
139+import json
140+import logging
141 import os
142+import re
143+import subprocess
144+from subprocess import PIPE, Popen
145+from threading import Thread
146
147+from UpdateManager.Core.MyCache import MyCache
148 from UpdateManager.Core.utils import inhibit_sleep
149 from UpdateManager.Dialogs import Dialog
150
151@@ -67,6 +76,170 @@
152 """Commit the cache changes """
153 raise NotImplementedError
154
155+ def get_snap_seeds(self):
156+ seeded_snaps = {}
157+ unseeded_snaps = {}
158+
159+ release = subprocess.Popen(["lsb_release", "-r", "-s"],
160+ universal_newlines=True,
161+ stdout=subprocess.PIPE).communicate()
162+ release = release[0].split()[0]
163+ curr_channel = "stable/ubuntu-" + release
164+
165+ try:
166+ d2s_file = open(
167+ '/usr/share/ubuntu-release-upgrader/deb2snap.json', 'r')
168+ d2s = json.load(d2s_file)
169+ d2s_file.close()
170+
171+ for snap in d2s["seeded"]:
172+ seed = d2s["seeded"][snap]
173+ deb = seed.get("deb", None)
174+ to_channel = seed.get("to_channel", curr_channel)
175+ seeded_snaps[snap] = (deb, to_channel)
176+
177+ for snap in d2s["unseeded"]:
178+ unseed = d2s["unseeded"][snap]
179+ from_channel = unseed.get("from_channel", curr_channel)
180+ unseeded_snaps[snap] = (from_channel)
181+ except Exception as e:
182+ logging.debug("error reading deb2snap.json file (%s)" % e)
183+
184+ return seeded_snaps, unseeded_snaps
185+
186+ def get_deb2snap_dups(self):
187+ # update and grab the latest cache
188+ try:
189+ if self.window_main.cache is None:
190+ self.window_main.cache = MyCache(None)
191+ else:
192+ self.window_main.cache.open(None)
193+ self.window_main.cache._initDepCache()
194+ cache = self.window_main.cache
195+ except Exception as e:
196+ logging.debug("error reading cache (%s)" % e)
197+ cache = None
198+
199+ duplicates = []
200+ seeded_snaps, _ = self.get_snap_seeds()
201+
202+ for snap, (deb, _) in seeded_snaps.items():
203+ # if the deb is installed and was not manually installed,
204+ # replace it
205+ if (deb in cache and cache[deb].is_auto_installed):
206+ duplicates.append(deb)
207+
208+ return duplicates
209+
210+ def get_snap_transitions(self):
211+ # populate snap_list with deb2snap transitions
212+ snap_list = {}
213+ seeded_snaps, unseeded_snaps = self.get_snap_seeds()
214+
215+ for snap, (deb, to_channel) in seeded_snaps.items():
216+ snap_object = {}
217+ # check if the snap is already installed
218+ snap_info = subprocess.Popen(["snap", "info", snap],
219+ universal_newlines=True,
220+ stdout=subprocess.PIPE).communicate()
221+ if re.search("^installed: ", snap_info[0], re.MULTILINE):
222+ logging.debug("Snap %s is installed" % snap)
223+ continue
224+ elif (deb in self.window_main.duplicate_packages):
225+ # install the snap if the deb was just marked delete
226+ snap_object['command'] = 'install'
227+ snap_object['channel'] = to_channel
228+ snap_list[snap] = snap_object
229+
230+ for snap, (_) in unseeded_snaps.items():
231+ snap_object = {}
232+ # check if the snap is already installed
233+ snap_info = subprocess.Popen(["snap", "info", snap],
234+ universal_newlines=True,
235+ stdout=subprocess.PIPE).communicate()
236+ if re.search("^installed: ", snap_info[0], re.MULTILINE):
237+ logging.debug("Snap %s is installed" % snap)
238+ # its not tracking the release channel so don't remove
239+ from_channel = "stable/ubuntu-[0-9][0-9].[0-9][0-9]"
240+ if not re.search(r"^tracking:.*%s" % from_channel,
241+ snap_info[0], re.MULTILINE):
242+ logging.debug("Snap %s is not tracking the release channel"
243+ % snap)
244+ continue
245+
246+ snap_object['command'] = 'remove'
247+
248+ # check if this snap is being used by any other snaps
249+ conns = subprocess.Popen(["snap", "connections", snap],
250+ universal_newlines=True,
251+ stdout=subprocess.PIPE).communicate()
252+
253+ for conn in conns[0].split('\n'):
254+ conn_cols = conn.split()
255+ if len(conn_cols) != 4:
256+ continue
257+ plug = conn_cols[1]
258+ slot = conn_cols[2]
259+
260+ if slot.startswith(snap + ':'):
261+ plug_snap = plug.split(':')[0]
262+ if plug_snap != '-' and \
263+ plug_snap not in unseeded_snaps:
264+ logging.debug("Snap %s is being used by %s. "
265+ "Switching it to stable track"
266+ % (snap, plug_snap))
267+ snap_object['command'] = 'refresh'
268+ snap_object['channel'] = 'stable'
269+ break
270+
271+ snap_list[snap] = snap_object
272+
273+ return snap_list
274+
275+ def update_snaps(self):
276+ # update status and progress bar
277+ def update_status(status):
278+ GLib.idle_add(self.label_details.set_label, status)
279+
280+ def update_progress(progress_bar):
281+ progress_bar.pulse()
282+ return True
283+
284+ update_status(_("Updating snaps"))
285+
286+ progress_timer = None
287+ progress_bars = self.progressbar_slot.get_children()
288+ if progress_bars and isinstance(progress_bars[0], Gtk.ProgressBar):
289+ progress_timer = GLib.timeout_add(100, update_progress,
290+ progress_bars[0])
291+
292+ # populate snap_list with deb2snap transitions
293+ snap_list = self.get_snap_transitions()
294+
295+ # (un)install (un)seeded snap(s)
296+ try:
297+ client = Snapd.Client()
298+ client.connect_sync()
299+ for snap, snap_object in snap_list.items():
300+ command = snap_object['command']
301+ if command == 'refresh':
302+ update_status(_("Refreshing %s snap" % snap))
303+ client.refresh_sync(snap, snap_object['channel'])
304+ elif command == 'remove':
305+ update_status(_("Removing %s snap" % snap))
306+ client.remove_sync(snap)
307+ else:
308+ update_status(_("Installing %s snap" % snap))
309+ client.install_sync(snap, snap_object['channel'])
310+ except GLib.Error as e:
311+ logging.debug("error updating snaps (%s)" % e)
312+
313+ # continue with the rest of the updates
314+ if progress_timer:
315+ GLib.source_remove(progress_timer)
316+
317+ GLib.idle_add(self.window_main.start_available)
318+
319 def _action_done(self, action, authorized, success, error_string,
320 error_desc, trans_failed=False):
321
322@@ -78,7 +251,9 @@
323 #close_after_install = False
324
325 if action == self.ACTION_INSTALL:
326- if success:
327+ if success and os.path.exists("/usr/bin/snap"):
328+ Thread(target=self.update_snaps).start()
329+ elif success:
330 self.window_main.start_available()
331 elif error_string:
332 self.window_main.start_error(trans_failed, error_string,
333@@ -90,6 +265,9 @@
334 else:
335 if error_string:
336 self.window_main.start_error(True, error_string, error_desc)
337+ elif success and os.path.exists("/usr/bin/snap"):
338+ self.window_main.duplicate_packages = self.get_deb2snap_dups()
339+ self.window_main.start_available()
340 else:
341 is_cancelled_update = not success
342 self.window_main.start_available(is_cancelled_update)
343
344=== modified file 'debian/changelog'
345--- debian/changelog 2020-01-31 21:44:36 +0000
346+++ debian/changelog 2020-04-06 11:32:18 +0000
347@@ -1,3 +1,10 @@
348+update-manager (1:20.04.2) UNRELEASED; urgency=medium
349+
350+ * UpdateManager/backend/__init__.py: once apt upgrade completes, use the
351+ deb2snap.json file from ubuntu-release-upgrader to perform snap updates.
352+
353+ -- Marcus Tomlinson <marcus.tomlinson@canonical.com> Mon, 02 Mar 2020 08:52:13 +0000
354+
355 update-manager (1:20.04.1) focal; urgency=medium
356
357 * UpdateManager/Core/UpdateList.py: remove unused import of subprocess.

Subscribers

People subscribed via source and target branches

to status/vote changes: