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

Proposed by Dimitri John Ledkov
Status: Merged
Approved by: Colin Watson
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/ubi-partman.py (+3/-5)
To merge this branch: bzr merge lp:~xnox/ubiquity/fix-value-errors
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Ubuntu Installer Team Pending
Review via email: mp+123727@code.launchpad.net

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.
Revision history for this message
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
5653. By Dimitri John Ledkov

catch up from trunk

Revision history for this message
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
5654. By Dimitri John Ledkov

typo

Revision history for this message
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
6+
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)
10+
11+ -- Dmitrijs Ledkovs <dmitrij.ledkov@ubuntu.com> Mon, 24 Sep 2012 11:23:57 +0100
12+
13 ubiquity (2.12.2) quantal; urgency=low
14
15 * Add manual crypto partitioning (LP: #1042649)
16
17=== modified file 'ubiquity/plugins/ubi-partman.py'
18--- ubiquity/plugins/ubi-partman.py 2012-09-18 15:16:46 +0000
19+++ ubiquity/plugins/ubi-partman.py 2012-09-24 11:31:19 +0000
20@@ -3061,6 +3061,9 @@
21 return plugin.Plugin.run(self, priority, question)
22
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())
26+
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')
33
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)
38-
39 # TODO cjwatson 2006-11-01: Do we still need this?
40 def rebuild_cache(self):
41 assert self.current_question == 'partman/choose_partition'

Subscribers

People subscribed via source and target branches

to status/vote changes: