Merge lp:~xnox/ubiquity/fix-value-errors into lp:ubiquity

Proposed by Dimitri John Ledkov on 2012-09-11
Status: Merged
Approved by: Colin Watson on 2012-09-24
Approved revision: 5654
Merged at revision: 5681
Proposed branch: lp:~xnox/ubiquity/fix-value-errors
Merge into: lp:ubiquity
Diff against target: 41 lines (+11/-5)
2 files modified
debian/changelog (+8/-0)
ubiquity/plugins/ (+3/-5)
To merge this branch: bzr merge lp:~xnox/ubiquity/fix-value-errors
Reviewer Review Type Date Requested Status
Colin Watson 2012-09-11 Approve on 2012-09-24
Ubuntu Installer Team 2012-09-11 Pending
Review via email:

Description of the change

It appears that previous fix to the ValueError was incomplete. Specifically the cleanup method is executed on abort and results in ValueError when run in ubiquity-dm session at the stepPartAsk page / "What would you like to do?".

So it seems like communication with debconf inside cleanup is also not-safe at the "pre-install" steps. But also I don't see a reason, why this should be communicated to debconf at the cleanup. Cleanup is a bit a catch all and is called at cancel, back-out to previous plugin and abort.

I am proposing to move grub device pre-seeding into the ok_handler which is executed only when confirming the installation / moving to the next step, while we still have debconf opened.

I still need to test manual partitioning & Qt frontend. But this seems like a better solution.

Colin, where there other reasons to run grub-installer/bootdev at exit_ui_loops/cleanup?

To post a comment you must log in.
Colin Watson (cjwatson) wrote :

This was moved to exit_ui_loops (and later to cleanup) due to bug 756920. I'm OK with you moving it back to ok_handler if you can demonstrate that that bug no longer occurs; it's certainly possible that it's since been fixed some other way, but it was a sufficiently bad bug that we should test it.

review: Needs Information
lp:~xnox/ubiquity/fix-value-errors updated on 2012-09-24
5653. By Dimitri John Ledkov on 2012-09-24

catch up from trunk

Dimitri John Ledkov (xnox) wrote :

Manually testing for bug 756920 shows no regressions with proposed branch.

Moving to cleanup, doesn't look like a right solution if you want that command to run before exit_ui_loops. Because cheanup_handler performs exit_ui_loops first, and then calls cleanup.

Semantically currently this chain of calls is happening: ok_handler -> exit_ui_loops -> cleanup -> check/preseed grub device
In the proposed branch I'm moving it to become: ok_handler -> check/preseed grub device -> exit_ui_loops -> cleanup

The reason being we should not fiddle with debconf upon back/quit and setting bootdev only matters if we continue to install.
The crashes reported by users is during quitting the installer in-between transitions to/from step-partman.

lp:~xnox/ubiquity/fix-value-errors updated on 2012-09-24
5654. By Dimitri John Ledkov on 2012-09-24


Colin Watson (cjwatson) wrote :

Yes, cleanup was clearly wrong.

If bug 756920 is definitely no longer present, then go ahead and merge this (for beta-2 if possible to maximise testing).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2012-09-20 17:11:15 +0000
3+++ debian/changelog 2012-09-24 11:31:19 +0000
4@@ -1,3 +1,11 @@
5+ubiquity (2.12.3) UNRELEASED; urgency=low
7+ * Do not preseed grub-install, if we are not proceeding to install. This
8+ should fix ValueError, I/O operation on closed file (LP: #1027648)
9+ (LP: #792652)
11+ -- Dmitrijs Ledkovs <> Mon, 24 Sep 2012 11:23:57 +0100
13 ubiquity (2.12.2) quantal; urgency=low
15 * Add manual crypto partitioning (LP: #1042649)
17=== modified file 'ubiquity/plugins/'
18--- ubiquity/plugins/ 2012-09-18 15:16:46 +0000
19+++ ubiquity/plugins/ 2012-09-24 11:31:19 +0000
20@@ -3061,6 +3061,9 @@
21 return, priority, question)
23 def ok_handler(self):
24+ if self.install_bootloader and not self.is_bootdev_preseeded():
25+ self.preseed('grub-installer/bootdev', self.ui.get_grub_choice())
27 if self.current_question.endswith('automatically_partition'):
28 (autopartition_choice, self.extra_choice) = \
29 self.ui.get_autopartition_choice()
30@@ -3076,11 +3079,6 @@
31 return ('UBIQUITY_AUTOMATIC' in os.environ and
32 self.db.fget('grub-installer/bootdev', 'seen') == 'true')
34- def cleanup(self):
35- if self.install_bootloader and not self.is_bootdev_preseeded():
36- self.preseed('grub-installer/bootdev', self.ui.get_grub_choice())
37- plugin.Plugin.cleanup(self)
39 # TODO cjwatson 2006-11-01: Do we still need this?
40 def rebuild_cache(self):
41 assert self.current_question == 'partman/choose_partition'


People subscribed via source and target branches

to status/vote changes: