Merge lp:~barry/ubiquity/bug-959724 into lp:ubiquity

[Barry Warsaw]

Not yet ready

[Barry Warsaw (barry)]

This still needs tests and KDE support, but for now it should be useful as a proof of concept.

[Barry Warsaw (barry)]

This branch is ready to go. It has a test of the underlying (i.e. refactored) code deciding which file system to offer to install the boot loader to. I've tested the kde and gtk ui's in a live install and they seem to work. However, if you think I should add a real ui test for gtk, just mark this branch as Needs Fixing and I'll add that.

[lp:~barry/ubiquity/bug-959724]

5287. By Barry Warsaw on 2012-03-21

Remove an unneeded additional realpath() call.

[Evan (ev)]

I'm somewhat weary of this breaking subtly as grub2 changes. Could we generate the reserved sector set as part of the build, using something like:

echo 'FS_RESERVED_FIRST_SECTOR = set(['; find . -name '*.c' | xargs grep "reserved_first_sector = 1" | sed 's,.*/\(.*\).c:.*,\1,' | while read x; do echo "'$x',"; done; echo '])'

With a check that if we didn't get any input at all, break the build as the format has likely changed.

[Evan (ev)]

That said, if others think this is overkill, ignore me :).

d-i/make-keyboard-names does something similar, if you'd like a reference.

[Evan (ev)]

+1

[Evan (ev)]

review: Approve

[Colin Watson (cjwatson)]

That would require having access to the grub2 source at build time ...

This is more a property of the filesystems than of grub2, though.

[lp:~barry/ubiquity/bug-959724]

5288. By Barry Warsaw on 2012-03-21

Make sure the UI offers to install to all disk devices.

[Barry Warsaw (barry)]

On Mar 21, 2012, at 07:41 PM, Evan Dandrea wrote:

>I'm somewhat weary of this breaking subtly as grub2 changes.

That's a legitimate concern. The problem is...

>Could we generate the reserved sector set as part of the build, using
>something like:
>
>echo 'FS_RESERVED_FIRST_SECTOR = set(['; find . -name '*.c' | xargs grep "reserved_first_sector = 1" | sed 's,.*/\(.*\).c:.*,\1,' | while read x; do echo "'$x',"; done; echo '])'

You'd have to do this as part of the grub2 build and make this list available
to ubiquity somehow. See Colin's suggestion in the bug; that information
(AFAICT) is only available in the grub2 source.

Is this list expected to change often?

[Barry Warsaw (barry)]

On Mar 21, 2012, at 08:19 PM, Barry Warsaw wrote:

>On Mar 21, 2012, at 07:41 PM, Evan Dandrea wrote:
>
>>I'm somewhat weary of this breaking subtly as grub2 changes.
>
>That's a legitimate concern. The problem is...
>
>>Could we generate the reserved sector set as part of the build, using
>>something like:
>>
>>echo 'FS_RESERVED_FIRST_SECTOR = set(['; find . -name '*.c' | xargs grep "reserved_first_sector = 1" | sed 's,.*/\(.*\).c:.*,\1,' | while read x; do echo "'$x',"; done; echo '])'
>
>You'd have to do this as part of the grub2 build and make this list available
>to ubiquity somehow. See Colin's suggestion in the bug; that information
>(AFAICT) is only available in the grub2 source.
>
>Is this list expected to change often?

We discussed this out-of-band and decided it wasn't worth it.

[lp:~barry/ubiquity/bug-959724]

5289. By Barry Warsaw on 2012-03-21

Add a UI test and clean up some whitespace.

[Colin Watson (cjwatson)]

Awesome, thank you! This looks good to me now.

review: Approve

[lp:~barry/ubiquity/bug-959724]

5290. By Barry Warsaw on 2012-03-21

Remove obsolete comment

=== modified file 'debian/changelog'
--- debian/changelog	2012-03-21 20:39:21 +0000
+++ debian/changelog	2012-03-21 21:26:17 +0000
@@ -24,6 +24,7 @@
   * Refactor ubiquity.misc.grub_default to make it testable, and add tests
     for it.
 
+<<<<<<< TREE
   [ Oliver Grawert ]
   * bin/oem-config-firstboot: do not reset debconf user data in automatic mode
   * ubiquity/plugins/ubi-tasks.py: run tasksel with --new-install option in
@@ -35,6 +36,14 @@
     attached to the bug, thanks to colin for all the fixes and the hard work.
 
  -- Stéphane Graber <[e-mail]>  Wed, 21 Mar 2012 13:08:44 -0400
+=======
+  [ Barry Warsaw ]
+  * Do not offer to install the boot loader on device paths on which
+    incompatible file systems will be installed (e.g. XFS).
+    (LP: #959724)
+
+ -- Stéphane Graber <[e-mail]>  Wed, 21 Mar 2012 11:26:21 -0400
+>>>>>>> MERGE-SOURCE
 
 ubiquity (2.9.31) precise; urgency=low
 

=== modified file 'tests/test_ubi_partman.py'
--- tests/test_ubi_partman.py	2012-03-13 23:24:14 +0000
+++ tests/test_ubi_partman.py	2012-03-21 21:26:17 +0000
@@ -1,6 +1,7 @@
 #!/usr/bin/python
 
 
+from itertools import izip, izip_longest
 import os
 from test import test_support
 import unittest
@@ -100,12 +101,27 @@
     #            self.assertNotEqual(method,
     #                                self.page.method_description(method))
 
+# A couple of mock helpers for some of the tests below.
+def _fake_grub_options(*paths):
+    # The interface expects a sequence-of-sequences, although the method
+    # only cares about sub-sequences of length 1, where the path is
+    # element zero.
+    def grub_options():
+        return [(path,) for path in paths]
+    return grub_options
+
+def _fake_grub_default(default):
+    def grub_default():
+        return default
+    return grub_default
+
+
 @unittest.skipUnless('DEBCONF_SYSTEMRC' in os.environ, 'Need a database.')
 class TestPage(unittest.TestCase):
     def setUp(self):
         # We could mock out the db for this, but we ultimately want to make
         # sure that the debconf questions its getting exist.
-        self.page = ubi_partman.Page(None)
+        self.page = ubi_partman.Page(None, ui=mock.Mock())
         self.page.db = debconf.DebconfCommunicator('ubi-test', cloexec=True)
         self.addCleanup(self.page.db.shutdown)
 
@@ -166,6 +182,234 @@
         no_detected = self.page.extended_description(q)
         self.assertEqual(no_detected, head)
 
+    def test_maybe_update_dont_install(self):
+        self.page.install_bootloader = False
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 0)
+
+    def test_maybe_update_install(self):
+        self.page.install_bootloader = True
+        self.page.disk_cache = {}
+        self.page.partition_cache = {}
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+
+    @mock.patch('ubiquity.misc.grub_options', _fake_grub_options('/dev/vda1'))
+    @mock.patch('ubiquity.misc.grub_default', _fake_grub_default('/dev/vda'))
+    def test_install_grub_to_valid_filesystem(self):
+        # Return some fake grub options.
+        self.page.install_bootloader = True
+        self.page.disk_cache = {
+            'ignore-1': {
+                'device': '/dev/vda',
+                },
+            }
+        self.page.partition_cache = {
+            'ignore': {
+                'parted': {
+                    'path': '/dev/vda1',
+                    'fs': 'ext4',
+                    }
+                }
+            }
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+        self.page.ui.set_grub_options.assert_called_once_with('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vda1': True,
+            })
+
+    @mock.patch('ubiquity.misc.grub_options', _fake_grub_options('/dev/vda1'))
+    @mock.patch('ubiquity.misc.grub_default', _fake_grub_default('/dev/vda'))
+    def test_install_grub_to_invalid_filesystem(self):
+        # Return some fake grub options.
+        self.page.install_bootloader = True
+        self.page.disk_cache = {
+            'ignore-1': {
+                'device': '/dev/vda',
+                },
+            }
+        self.page.partition_cache = {
+            'ignore': {
+                'parted': {
+                    'path': '/dev/vda1',
+                    'fs': 'xfs',
+                    }
+                }
+            }
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+        self.page.ui.set_grub_options.assert_called_once_with('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vda1': False,
+            })
+
+    @mock.patch('ubiquity.misc.grub_options',
+                _fake_grub_options('/dev/vda1', '/dev/vda2'))
+    @mock.patch('ubiquity.misc.grub_default', _fake_grub_default('/dev/vda'))
+    def test_install_grub_to_mixed_filesystems(self):
+        # Return some fake grub options.
+        self.page.install_bootloader = True
+        self.page.disk_cache = {
+            'ignore-1': {
+                'device': '/dev/vda',
+                },
+            }
+        self.page.partition_cache = {
+            'ignore-1': {
+                'parted': {
+                    'path': '/dev/vda1',
+                    'fs': 'xfs',
+                    }
+                },
+            'ignore-2': {
+                'parted': {
+                    'path': '/dev/vda2',
+                    'fs': 'ext2',
+                    }
+                }
+            }
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+        self.page.ui.set_grub_options.assert_called_once_with('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vda1': False,
+            '/dev/vda2': True,
+            })
+
+    @mock.patch('ubiquity.misc.grub_options',
+                _fake_grub_options('/dev/vda1', '/dev/vda2', '/dev/vdb1'))
+    @mock.patch('ubiquity.misc.grub_default', _fake_grub_default('/dev/vda'))
+    def test_install_grub_offers_to_install_to_disk(self):
+        # Return some fake grub options.
+        self.page.install_bootloader = True
+        self.page.disk_cache = {
+            'ignore-1': {
+                'device': '/dev/vda',
+                },
+            'ignore-2': {
+                'device': '/dev/vdb',
+                },
+            }
+        self.page.partition_cache = {
+            'ignore-1': {
+                'parted': {
+                    'path': '/dev/vda1',
+                    'fs': 'xfs',
+                    },
+                },
+            'ignore-2': {
+                'parted': {
+                    'path': '/dev/vda2',
+                    'fs': 'ext2',
+                    },
+                },
+            'ignore-3': {
+                'parted': {
+                    'path': '/dev/vdb1',
+                    'fs': 'xfs',
+                    },
+                },
+            }
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+        self.page.ui.set_grub_options.assert_called_once_with('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vdb': True,
+            '/dev/vda1': False,
+            '/dev/vda2': True,
+            '/dev/vdb1': False,
+            })
+
+    @mock.patch('ubiquity.misc.grub_options',
+                _fake_grub_options('/dev/vda1', '/dev/vda2', '/dev/vdb1'))
+    @mock.patch('ubiquity.misc.grub_default', _fake_grub_default('/dev/vda'))
+    def test_install_grub_offers_to_install_to_all_but_jfs(self):
+        # Return some fake grub options.
+        self.page.install_bootloader = True
+        self.page.disk_cache = {
+            'ignore-1': {
+                'device': '/dev/vda',
+                },
+            'ignore-2': {
+                'device': '/dev/vdb',
+                },
+            }
+        self.page.partition_cache = {
+            'ignore-1': {
+                'parted': {
+                    'path': '/dev/vda1',
+                    'fs': 'ext4',
+                    },
+                },
+            'ignore-2': {
+                'parted': {
+                    'path': '/dev/vda2',
+                    'fs': 'ext2',
+                    },
+                },
+            'ignore-3': {
+                'parted': {
+                    'path': '/dev/vdb1',
+                    'fs': 'jfs',
+                    },
+                },
+            }
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+        self.page.ui.set_grub_options.assert_called_once_with('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vdb': True,
+            '/dev/vda1': True,
+            '/dev/vda2': True,
+            '/dev/vdb1': False,
+            })
+
+    @mock.patch('ubiquity.misc.grub_options',
+                _fake_grub_options('/dev/vda1', '/dev/vda2', '/dev/vdb1'))
+    @mock.patch('ubiquity.misc.grub_default', _fake_grub_default('/dev/vda'))
+    def test_install_grub_offers_to_install_to_all(self):
+        # Return some fake grub options.
+        self.page.install_bootloader = True
+        self.page.disk_cache = {
+            'ignore-1': {
+                'device': '/dev/vda',
+                },
+            'ignore-2': {
+                'device': '/dev/vdb',
+                },
+            }
+        self.page.partition_cache = {
+            'ignore-1': {
+                'parted': {
+                    'path': '/dev/vda1',
+                    'fs': 'ext4',
+                    },
+                },
+            'ignore-2': {
+                'parted': {
+                    'path': '/dev/vda2',
+                    'fs': 'ext2',
+                    },
+                },
+            'ignore-3': {
+                'parted': {
+                    'path': '/dev/vdb1',
+                    'fs': 'fat16',
+                    },
+                },
+            }
+        self.page.maybe_update_grub()
+        self.assertEqual(self.page.ui.set_grub_options.call_count, 1)
+        self.page.ui.set_grub_options.assert_called_once_with('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vdb': True,
+            '/dev/vda1': True,
+            '/dev/vda2': True,
+            '/dev/vdb1': True,
+            })
+
+
 @unittest.skipUnless('DEBCONF_SYSTEMRC' in os.environ, 'Need a database.')
 class TestCalculateAutopartitioningOptions(unittest.TestCase):
     '''Test that the each expected autopartitioning option exists and is
@@ -413,10 +657,19 @@
         self.assertIn('manual', options)
         self.assertItemsEqual(self.manual, options['manual'])
 
+
+def _fake_grub_options_pairs(paths, descriptions):
+    # The interface expects a sequence-of-sequences, although the method
+    # only cares about sub-sequences of length 1, where the path is
+    # element zero.
+    def grub_options():
+        return [(path, description)
+                for path, description
+                in izip_longest(paths, descriptions, fillvalue='')]
+    return grub_options
+
 class TestPageGtk(unittest.TestCase):
     def setUp(self):
-        # FIXME Not sure why this is needed.
-        from ubiquity import gtkwidgets
         controller = mock.Mock()
         self.gtk = ubi_partman.PageGtk(controller)
 
@@ -426,5 +679,33 @@
         gtkwidgets.refresh()
         self.gtk.controller.go_forward.assert_called_once_with()
 
+    @mock.patch('ubiquity.misc.grub_options',
+                _fake_grub_options_pairs(
+                    ('/dev/vda', '/dev/vdb',
+                     '/dev/vda1', '/dev/vda2', '/dev/vdb1'),
+                    ('Virtio Block Device (108 GB)',
+                     'Virtio Block Device (801 GB)')))
+    def test_boot_loader_installation_combobox(self):
+        self.gtk.set_grub_options('/dev/vda', {
+            '/dev/vda': True,
+            '/dev/vda1': True,
+            '/dev/vda2': False,
+            '/dev/vdb': True,
+            '/dev/vdb1': True,
+            })
+        # The combo box should have everything but vda2.
+        expected = [
+            '/dev/vda Virtio Block Device (108 GB)',
+            '/dev/vdb Virtio Block Device (801 GB)',
+            '/dev/vda1 ',
+            '/dev/vdb1 ',
+            ]
+        row_text = []
+        for row in self.gtk.grub_device_entry.get_model():
+            row_text.append(' '.join(row))
+        for want, got in izip(expected, row_text):
+            self.assertEqual(want, got)
+
+
 if __name__ == '__main__':
     test_support.run_unittest(TestCalculateAutopartitioningOptions, TestPage, PartmanPageDirectoryTests)

=== modified file 'ubiquity/frontend/kde_components/PartMan.py'
--- ubiquity/frontend/kde_components/PartMan.py	2012-02-04 02:53:32 +0000
+++ ubiquity/frontend/kde_components/PartMan.py	2012-03-21 21:26:17 +0000
@@ -444,11 +444,13 @@
         self.ctrlr.allow_change_step(False)
         self.ctrlr.dbfilter.undo()
 
-    def setGrubOptions(self, options, default):
+    def setGrubOptions(self, options, default, grub_installable):
         self.part_advanced_bootloader_frame.setVisible(True)
         self.grub_device_entry.clear()
         for opt in options:
-            self.grub_device_entry.addItem(opt[0]);
+            path = opt[0]
+            if grub_installable.get(path, False):
+                self.grub_device_entry.addItem(path)
 
         index = self.grub_device_entry.findText(default)
         if (index == -1):

=== modified file 'ubiquity/plugins/ubi-partman.py'
--- ubiquity/plugins/ubi-partman.py	2012-03-21 15:33:56 +0000
+++ ubiquity/plugins/ubi-partman.py	2012-03-21 21:26:17 +0000
@@ -40,6 +40,28 @@
 PartitioningOption = namedtuple('PartitioningOption', ['title', 'desc'])
 Partition = namedtuple('Partition', ['device', 'size', 'id', 'filesystem'])
 
+# List of file system types that reserve extra space for grub.  Found by
+# grepping for "reserved_first_sector = 1" in the grub2 source as per 
+# https://bugs.launchpad.net/ubuntu/+source/ubiquity/+bug/959724
+# Only those file systems that set this to 1 can have the boot loader
+# installed on them.  This is that list, with values taken from the .name
+# entry in the matching structs.
+FS_RESERVED_FIRST_SECTOR = set([
+    'btrfs',
+    'ext2',
+    'fat',
+    'hfsplus',
+    'nilfs2',
+    'ntfs',
+    # Add a few ext variants that aren't explicitly described in grub2.
+    'ext3',
+    'ext4',
+    # Add a few fat variants.
+    'fat16',
+    'fat32',
+    # Others?
+    ])
+
 
 class PageBase(plugin.PluginUI):
     def __init__(self, *args, **kwargs):
@@ -465,20 +487,20 @@
         self.controller.go_forward()
         return True
 
-    def set_grub_options(self, default):
+    def set_grub_options(self, default, grub_installable):
         from gi.repository import Gtk, GObject
         self.bootloader_vbox.show()
         options = misc.grub_options()
-        if default.startswith('/'):
-            default = os.path.realpath(default)
         l = Gtk.ListStore(GObject.TYPE_STRING, GObject.TYPE_STRING)
         self.grub_device_entry.set_model(l)
         selected = False
         for opt in options:
-            i = l.append(opt)
-            if opt[0] == default:
-                self.grub_device_entry.set_active_iter(i)
-                selected = True
+            path = opt[0]
+            if grub_installable.get(path, False):
+                i = l.append(opt)
+                if path == default:
+                    self.grub_device_entry.set_active_iter(i)
+                    selected = True
         if not selected:
             i = l.append([default, ''])
             self.grub_device_entry.set_active_iter(i)
@@ -1290,11 +1312,9 @@
         self.disk_layout = layout
         self.partAuto.setDiskLayout(layout)
 
-    def set_grub_options(self, default):
+    def set_grub_options(self, default, grub_installable):
         options = misc.grub_options()
-        if default.startswith('/'):
-            default = os.path.realpath(default)
-        self.partMan.setGrubOptions(options, default)
+        self.partMan.setGrubOptions(options, default, grub_installable)
 
     def get_grub_choice(self):
         choice = self.partMan.getGrubChoice()
@@ -2907,13 +2927,50 @@
             self.local_progress = False
 
     def maybe_update_grub(self):
-        if self.install_bootloader:
-            grub_bootdev=self.db.get("grub-installer/bootdev")
-            if grub_bootdev and grub_bootdev in (part[0] for part in misc.grub_options()):
-                default = grub_bootdev
-            else:
-                default = misc.grub_default()
-            self.ui.set_grub_options(default)
+        if not self.install_bootloader:
+            return
+        paths = [part[0] for part in misc.grub_options()]
+        # Get the default boot device.
+        grub_bootdev = self.db.get("grub-installer/bootdev")
+        if grub_bootdev and grub_bootdev in paths:
+            default = grub_bootdev
+        else:
+            default = misc.grub_default()
+        # Create a reverse mapping from grub options device path to the file
+        # system type being installed on that option.  Then later we'll make
+        # sure that grub can actually be installed on that path's file system.
+        fstype_by_path = {}
+        for key, value in self.partition_cache.items():
+            path = value.get('parted', {}).get('path')
+            fstype = value.get('parted', {}).get('fs', 'free')
+            if path is not None:
+                # Duplicate paths should not be possible, but if we find one,
+                # the first one (<wink>, i.e. random) wins.
+                if path in fstype_by_path:
+                    self.debug('already found path %s with type %s',
+                               path, fstype_by_path[path])
+                else:
+                    fstype_by_path[path] = fstype
+        if default.startswith('/'):
+            default = os.path.realpath(default)
+        # Now, create a dictionary mapping boot device path to a flag
+        # indicating whether grub can or cannot be installed on that path's
+        # file system.  Use 'free' as a default since grub can never be
+        # installed on free space.
+        grub_installable = {}
+        for path in paths:
+            fstype = fstype_by_path.get(path, 'free')
+            can_install = fstype in FS_RESERVED_FIRST_SECTOR
+            grub_installable[path] = can_install
+            self.debug('device path: %s, fstype: %s, grub installable? %s',
+                       path, fstype, 'yes' if can_install else 'no')
+        # Let grub offer to install to all the disk devices.
+        for key, value in self.disk_cache.items():
+            device = value.get('device')
+            if device is not None:
+                grub_installable[device] = True
+                self.debug('device path: %s grub installable? yes', device)
+        self.ui.set_grub_options(default, grub_installable)
 
 # Notes:
 #