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

[Dimitri John Ledkov]

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?

[Colin Watson (cjwatson)]

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]

5653. By Dimitri John Ledkov on 2012-09-24

catch up from trunk

[Dimitri John Ledkov (xnox)]

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]

5654. By Dimitri John Ledkov on 2012-09-24

typo

[Colin Watson (cjwatson)]

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

=== modified file 'debian/changelog'
--- debian/changelog	2012-09-20 17:11:15 +0000
+++ debian/changelog	2012-09-24 11:31:19 +0000
@@ -1,3 +1,11 @@
+ubiquity (2.12.3) UNRELEASED; urgency=low
+
+  * Do not preseed grub-install, if we are not proceeding to install. This
+    should fix ValueError, I/O operation on closed file (LP: #1027648)
+    (LP: #792652)
+
+ -- Dmitrijs Ledkovs <[e-mail]>  Mon, 24 Sep 2012 11:23:57 +0100
+
 ubiquity (2.12.2) quantal; urgency=low
 
   * Add manual crypto partitioning (LP: #1042649)
 
=== modified file 'ubiquity/plugins/ubi-partman.py'
--- ubiquity/plugins/ubi-partman.py	2012-09-18 15:16:46 +0000
+++ ubiquity/plugins/ubi-partman.py	2012-09-24 11:31:19 +0000
@@ -3061,6 +3061,9 @@
         return plugin.Plugin.run(self, priority, question)
 
     def ok_handler(self):
+        if self.install_bootloader and not self.is_bootdev_preseeded():
+            self.preseed('grub-installer/bootdev', self.ui.get_grub_choice())
+
         if self.current_question.endswith('automatically_partition'):
             (autopartition_choice, self.extra_choice) = \
                 self.ui.get_autopartition_choice()
@@ -3076,11 +3079,6 @@
         return ('UBIQUITY_AUTOMATIC' in os.environ and
                 self.db.fget('grub-installer/bootdev', 'seen') == 'true')
 
-    def cleanup(self):
-        if self.install_bootloader and not self.is_bootdev_preseeded():
-            self.preseed('grub-installer/bootdev', self.ui.get_grub_choice())
-        plugin.Plugin.cleanup(self)
-
     # TODO cjwatson 2006-11-01: Do we still need this?
     def rebuild_cache(self):
         assert self.current_question == 'partman/choose_partition'