Merge lp:~salgado/linaro-image-tools/bug-710971 into lp:linaro-image-tools/11.11

[Guilherme Salgado]

Fix the bug by making sure OmapConfig.serial_tty is never used before set_appropriate_serial_tty() is called.

[James Westby (james-w)]

Looks good, thanks.

BoardConfigs as classes rather than objects is looking increasingly creaky
IMO, but it's likely to be rewritten with hwpacks v2 anyway.

Thanks,

James

review: Approve

[Guilherme Salgado (salgado)]

On Tue, 2011-02-01 at 14:33 +0000, James Westby wrote:
> Review: Approve
> Looks good, thanks.
>
> BoardConfigs as classes rather than objects is looking increasingly creaky
> IMO, but it's likely to be rewritten with hwpacks v2 anyway.

That is true, but the creaky bits are part of the interim solution for
bug 697824. As you said, though, this is all going away with hwpacks
v2.

=== modified file 'linaro_media_create/boards.py'
--- linaro_media_create/boards.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/boards.py	2011-02-01 13:26:55 +0000
@@ -146,6 +146,15 @@
     _serial_tty = None
 
     @classproperty
+    def serial_tty(cls):
+        # This is just to make sure no callsites use .serial_tty before
+        # calling set_appropriate_serial_tty(). If we had this in the first
+        # place we'd have uncovered bug 710971 before releasing.
+        raise AttributeError(
+            "You must not use this attribute before calling "
+            "set_appropriate_serial_tty")
+
+    @classproperty
     def live_serial_opts(cls):
         return cls._live_serial_opts % cls.serial_tty
 
@@ -161,18 +170,29 @@
         we use the default value (_serial_tty).
         """
         # XXX: This is also part of our temporary hack to fix bug 697824.
-        cls.serial_tty = cls._serial_tty
+        cls.serial_tty = classproperty(lambda cls: cls._serial_tty)
         vmlinuz = _get_file_matching(
             os.path.join(chroot_dir, 'boot', 'vmlinuz*'))
         basename = os.path.basename(vmlinuz)
         minor_version = re.match('.*2\.6\.([0-9]{2}).*', basename).group(1)
         if int(minor_version) < 36:
-            cls.serial_tty = 'ttyS2'
+            cls.serial_tty = classproperty(lambda cls: 'ttyS2')
+
+    @classmethod
+    def make_boot_files(cls, uboot_parts_dir, is_live, is_lowmem, consoles,
+                        root_dir, rootfs_uuid, boot_dir, boot_script,
+                        boot_device_or_file):
+        # XXX: This is also part of our temporary hack to fix bug 697824; we
+        # need to call set_appropriate_serial_tty() before doing anything that
+        # may use cls.serial_tty.
+        cls.set_appropriate_serial_tty(root_dir)
+        super(OmapConfig, cls).make_boot_files(
+            uboot_parts_dir, is_live, is_lowmem, consoles, root_dir,
+            rootfs_uuid, boot_dir, boot_script, boot_device_or_file)
 
     @classmethod
     def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
                          boot_dir, boot_script, boot_device_or_file):
-        cls.set_appropriate_serial_tty(chroot_dir)
         install_omap_boot_loader(chroot_dir, boot_dir)
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-02-01 13:26:55 +0000
@@ -3,7 +3,7 @@
 # Author: Guilherme Salgado <[e-mail]>
 #
 # This file is part of Linaro Image Tools.
-# 
+#
 # Linaro Image Tools is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation, either version 3 of the License, or
@@ -42,6 +42,7 @@
     )
 import linaro_media_create
 from linaro_media_create.boards import (
+    BoardConfig,
     board_configs,
     make_boot_script,
     make_uImage,
@@ -248,6 +249,32 @@
 
 class TestFixForBug697824(TestCaseWithFixtures):
 
+    def mock_set_appropriate_serial_tty(self, config):
+
+        def set_appropriate_serial_tty_mock(cls, chroot_dir):
+            self.set_appropriate_serial_tty_called = True
+            cls.serial_tty = cls._serial_tty
+
+        self.useFixture(MockSomethingFixture(
+            config, 'set_appropriate_serial_tty',
+            classmethod(set_appropriate_serial_tty_mock)))
+
+    def test_omap_make_boot_files(self):
+        self.set_appropriate_serial_tty_called = False
+        self.mock_set_appropriate_serial_tty(board_configs['beagle'])
+        self.useFixture(MockSomethingFixture(
+            BoardConfig, 'make_boot_files',
+            classmethod(lambda *args: None)))
+        # We don't need to worry about what's passed to make_boot_files()
+        # because we mock the method which does the real work above and here
+        # we're only interested in ensuring that OmapConfig.make_boot_files()
+        # calls set_appropriate_serial_tty().
+        board_configs['beagle'].make_boot_files(
+            None, None, None, None, None, None, None, None, None)
+        self.assertTrue(
+            self.set_appropriate_serial_tty_called,
+            "make_boot_files didn't call set_appropriate_serial_tty")
+
     def test_set_appropriate_serial_tty_old_kernel(self):
         tempdir = self.useFixture(CreateTempDirFixture()).tempdir
         boot_dir = os.path.join(tempdir, 'boot')
@@ -316,7 +343,12 @@
         self.assertEqual(expected, boot_cmd)
 
     def test_panda(self):
-        boot_cmd = board_configs['panda']._get_boot_cmd(
+        # XXX: To fix bug 697824 we have to change class attributes of our
+        # OMAP board configs, and some tests do that so to make sure they
+        # don't interfere with us we'll reset that before doing anything.
+        config = board_configs['panda']
+        config.serial_tty = config._serial_tty
+        boot_cmd = config._get_boot_cmd(
             is_live=False, is_lowmem=False, consoles=None,
             rootfs_uuid="deadbeef")
         expected = (
@@ -347,7 +379,12 @@
         self.assertEqual(expected, boot_cmd)
 
     def test_overo(self):
-        boot_cmd = board_configs['overo']._get_boot_cmd(
+        # XXX: To fix bug 697824 we have to change class attributes of our
+        # OMAP board configs, and some tests do that so to make sure they
+        # don't interfere with us we'll reset that before doing anything.
+        config = board_configs['overo']
+        config.serial_tty = config._serial_tty
+        boot_cmd = config._get_boot_cmd(
             is_live=False, is_lowmem=False, consoles=None,
             rootfs_uuid="deadbeef")
         expected = (