Merge lp:~salgado/linaro-image-tools/port-populate_boot into lp:linaro-image-tools/11.11

[Guilherme Salgado]

This branch ports the meat of populate_boot to python.

There are a few things left to port in that function, but this branch is
already quite big.

It can be reviewed as one big diff or two incremental ones:

  r175 refactors populate_boot into smaller shell functions
  r178 ports that to python, and even makes a few improvements

[Martin Ohlsson (martin-ohlson)]

>=== modified file 'media_create/cmd_runner.py'
>--- media_create/cmd_runner.py 2010-12-01 19:08:25 +0000
>+++ media_create/cmd_runner.py 2010-12-03 19:01:21 +0000
>@@ -1,12 +1,13 @@
> import subprocess
>
>-def run(args, as_root=False):
>+def run(args, as_root=False, stdout=None):

Would it be possible to add stdin as a parameter to this function so the output from one command can be connected to the input of another? E.g. check_device() connects several commands using a pipe.

This implies also that stdout of the subprocess is returned in some way.

/Martin

[Guilherme Salgado (salgado)]

On Fri, 2010-12-03 at 19:57 +0000, Martin Ohlsson wrote:
> >=== modified file 'media_create/cmd_runner.py'
> >--- media_create/cmd_runner.py 2010-12-01 19:08:25 +0000
> >+++ media_create/cmd_runner.py 2010-12-03 19:01:21 +0000
> >@@ -1,12 +1,13 @@
> > import subprocess
> >
> >-def run(args, as_root=False):
> >+def run(args, as_root=False, stdout=None):
>
> Would it be possible to add stdin as a parameter to this function so the output from one command can be connected to the input of another? E.g. check_device() connects several commands using a pipe.

I suppose that's possible, but I wonder if there is a real advantage in
running each of the commands individually and pipe their stdin/stdout in
Python, or would this be a case we'd be better off by letting the shell
do that (i.e. run them all as a single subprocess.call(...,
shell=True))?

>
> This implies also that stdout of the subprocess is returned in some way.

I'm not sure I understand what you mean here.

[Martin Ohlsson (martin-ohlson)]

You're right, it would probably mess up the simplicity of the cmd_runner to make it handle everything. More complex cases might be better handled locally for those rare situations.

[Michael Hudson-Doyle (mwhudson)]

I haven't checked every last detail for correctness, but I approve of the overall structure. Let me know if you want me to check all the arguments to each invocation of mkImage -- it can be done, but is probably a paper and pencil job.

I wonder if it would be easier to read if populate_boot took --options rather than positional arguments. I guess it depends how you see the code developing.

Thanks for working on this, linaro-media-create just makes me want to cry and this helps :-)

review: Approve

[Guilherme Salgado (salgado)]

On Mon, 2010-12-06 at 22:21 +0000, Michael Hudson-Doyle wrote:
> Review: Approve
> I haven't checked every last detail for correctness, but I approve of
> the overall structure. Let me know if you want me to check all the
> arguments to each invocation of mkImage -- it can be done, but is
> probably a paper and pencil job.

I don't think that's necessary; I've checked it many times myself.

>
> I wonder if it would be easier to read if populate_boot took --options
> rather than positional arguments. I guess it depends how you see the
> code developing.

It'd probably be more readable, but soon enough we won't need any of
that as everything will be python, so I'd rather stick with whatever is
simpler until we get there.

>
> Thanks for working on this, linaro-media-create just makes me want to
> cry and this helps :-)

You're certainly not alone there.

Thanks for the review!

=== modified file 'linaro-media-create'
--- linaro-media-create	2010-11-30 12:35:27 +0000
+++ linaro-media-create	2010-12-03 19:01:21 +0000
@@ -535,9 +535,9 @@
   echo ""
  
   if [ "$IS_LIVE" -eq 1 ]; then
-    parts_dir=casper
+    parts_dir="${DIR}/binary/casper"
   else
-    parts_dir=boot
+    parts_dir="${DIR}/binary/boot"
   fi
  
   mkdir -p "${BOOT_DISK}"
@@ -548,92 +548,9 @@
                "${BOOT_DISK}"
   fi
 
-  case "$DEVIMAGE" in
-    beagle|igep)
-      if [ "$DEVIMAGE" = "beagle" ]; then
-        sudo cp -v binary/usr/lib/x-loader-omap/MLO "${BOOT_DISK}"
-      fi
-      sync
-      echo "done"
-
-      sudo mkimage -A arm -O linux -T kernel -C none -a 0x80008000 \
-           -e 0x80008000 -n Linux \
-           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-omap \
-           "${BOOT_DISK}/uImage"
-      sudo mkimage -A arm -O linux -T ramdisk -C none -a 0 \
-           -e 0 -n initramfs \
-           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-linaro-omap \
-           "${BOOT_DISK}/uInitrd"
-      sudo mkimage -A arm -O linux -T script -C none -a 0 \
-           -e 0 -n "boot script" -d "${TMP_DIR}/boot.cmd" \
-           "${BOOT_DISK}/boot.scr"
-      sudo cp -v "${BOOT_DISK}/boot.scr" "${BOOT_DISK}/boot.ini"
-      ;;
-
-    panda)
-      sudo cp -v binary/usr/lib/x-loader-omap4/MLO "${BOOT_DISK}"
-      sync
-      echo "done"
-  
-      sudo mkimage -A arm -O linux -T kernel -C none -a 0x80008000 \
-           -e 0x80008000 -n Linux \
-           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-omap4 \
-           "${BOOT_DISK}/uImage"
-      sudo mkimage -A arm -O linux -T ramdisk -C none -a 0 \
-           -e 0 -n initramfs \
-           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-omap4 \
-           "${BOOT_DISK}/uInitrd"
-      sudo mkimage -A arm -O linux -T script -C none -a 0 \
-           -e 0 -n "boot script" -d "${TMP_DIR}/boot.cmd" \
-           "${BOOT_DISK}/boot.scr"
-      sudo cp -v "${BOOT_DISK}/boot.scr" "${BOOT_DISK}/boot.ini"
-      ;;
-
-    ux500)
-      sudo mkimage -A arm -O linux -T kernel -C none -a 0x00008000 \
-           -e 0x00008000 -n Linux \
-           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-ux500 \
-           "${BOOT_DISK}/uImage"
-      sudo mkimage -A arm -O linux -T ramdisk -C none -a 0 \
-           -e 0 -n initramfs \
-           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-ux500 \
-           "${BOOT_DISK}/uInitrd"
-      sudo mkimage -A arm -O linux -T script -C none -a 0 \
-           -e 0 -n "boot script" -d "${TMP_DIR}/boot.cmd" \
-           "${BOOT_DISK}/flash.scr"
-      ;;
-
-    vexpress)
-      sudo mkimage -A arm -O linux -T kernel -C none -a "$KERNEL_ADDR" \
-           -e "$KERNEL_ADDR" -n Linux \
-           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-vexpress \
-           "${BOOT_DISK}/uImage"
-      sudo mkimage -A arm -O linux -T ramdisk -C none -a "$INITRD_ADDR" \
-           -e "$INITRD_ADDR" -n initramfs \
-           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-linaro-vexpress \
-           "${BOOT_DISK}/uInitrd"
-      ;;
-
-    mx51evk)
-      sudo dd if=binary/usr/lib/u-boot/mx51evk/u-boot.imx of="${DEVICE:-$IMAGE_FILE}" \
-           bs=1024 seek=1 conv=notrunc
-      sudo mkimage -A arm -O linux -T kernel -C none -a 0x90008000 \
-           -e 0x90008000 -n Linux \
-           -d "${DIR}/binary/${parts_dir}"/vmlinuz-*-linaro-mx51 \
-           "${BOOT_DISK}/uImage"
-      sudo mkimage -A arm -O linux -T ramdisk -C none -a 0 \
-           -e 0 -n initramfs \
-           -d "${DIR}/binary/${parts_dir}"/initrd.img-*-linaro-mx51 \
-           "${BOOT_DISK}/uInitrd"
-      sudo mkimage -A arm -O linux -T script -C none -a 0 \
-           -e 0 -n "boot script" -d "${TMP_DIR}/boot.cmd" \
-           "${BOOT_DISK}/boot.scr"
-      ;;
-    *)
-      echo "Internal error; missing support for $DEVIMAGE" >&2
-      exit 1
-      ;;
-  esac
+  python -m media_create.populate_boot "$DEVIMAGE" "$SUB_ARCH" "$LOAD_ADDR" \
+    "$parts_dir" "$BOOT_DISK" "$TMP_DIR" "$BOOT_SCRIPT" \
+    "${DEVICE:-$IMAGE_FILE}"
 
   sync
   sync
@@ -798,6 +715,8 @@
   ;;
 esac
 
+# This whole thing will be moved into config files that will be specific for
+# each board.
 if [ "$DEVIMAGE" ]; then
   case "$DEVIMAGE" in
     beagle|igep)
@@ -807,6 +726,9 @@
       fi
       KERNEL_ADDR="0x80000000"
       INITRD_ADDR="0x81600000"
+      LOAD_ADDR="0x80008000"
+      SUB_ARCH="linaro-omap"
+      BOOT_SCRIPT="boot.scr"
       boot_args_options="$boot_args_options earlyprintk fixrtc nocompcache vram=12M omapfb.debug=y omapfb.mode=dvi:1280x720MR-16@60"
       ;;
     panda)
@@ -816,6 +738,9 @@
       fi
       KERNEL_ADDR="0x80200000"
       INITRD_ADDR="0x81600000"
+      LOAD_ADDR="0x80008000"
+      SUB_ARCH="omap4"
+      BOOT_SCRIPT="boot.scr"
       boot_args_options="$boot_args_options earlyprintk fixrtc nocompcache vram=32M omapfb.debug=y omapfb.vram=0:8M mem=463M ip=none"
       ;;
     ux500)
@@ -825,6 +750,9 @@
       fi
       KERNEL_ADDR="0x00100000"
       INITRD_ADDR="0x08000000"
+      LOAD_ADDR="0x00008000"
+      SUB_ARCH="ux500"
+      BOOT_SCRIPT="flash.scr"
       boot_args_options="$boot_args_options earlyprintk rootdelay=1 fixrtc nocompcache"
       boot_args_options="$boot_args_options mem=96M@0 mem_modem=32M@96M mem=44M@128M pmem=22M@172M mem=30M@194M mem_mali=32M@224M pmem_hwb=54M@256M hwmem=48M@302M mem=152M@360M"
       mmc_option="1:1"
@@ -836,6 +764,9 @@
       fi
       KERNEL_ADDR="0x90000000"
       INITRD_ADDR="0x90800000"
+      LOAD_ADDR="0x90008000"
+      SUB_ARCH="linaro-mx51"
+      BOOT_SCRIPT="boot.scr"
       MMC_PART_OFFSET=1
       mmc_option="0:2"
       ;;
@@ -846,6 +777,8 @@
       fi
       KERNEL_ADDR="0x60008000"
       INITRD_ADDR="0x81000000"
+      LOAD_ADDR=$KERNEL_ADDR
+      SUB_ARCH="linaro-vexpress"
       # ARM Boot Monitor is used to load u-boot, uImage etc. into flash and
       # only allows for FAT16
       FAT_SIZE=16
 
=== modified file 'media_create/cmd_runner.py'
--- media_create/cmd_runner.py	2010-12-01 19:08:25 +0000
+++ media_create/cmd_runner.py	2010-12-03 19:01:21 +0000
@@ -1,12 +1,13 @@
 import subprocess
 
 
-def run(args, as_root=False):
+def run(args, as_root=False, stdout=None):
     """Run the given command as a sub process.
 
     :param command: A list or tuple containing the command to run and the
                     arguments that should be passed to it.
     :param as_root: Should the given command be run as root (with sudo)?
+    :param stdout: Same as subprocess.Popen().
     """
     assert isinstance(args, (list, tuple)), (
         "The command to run must be a list or tuple, found: %s" % type(args))
@@ -15,15 +16,15 @@
     if as_root:
         args = args[:]
         args.insert(0, 'sudo')
-    return_value = do_run(args)
+    return_value = do_run(args, stdout=stdout)
     if return_value != 0:
         raise SubcommandNonZeroReturnValue(args, return_value)
     return return_value
 
 
-def do_run(args):
+def do_run(args, stdout=None):
     """A wrapper around subprocess.call() to make testing easier."""
-    return subprocess.call(args)
+    return subprocess.call(args, stdout=stdout)
 
 
 class SubcommandNonZeroReturnValue(Exception):
 
=== added file 'media_create/populate_boot.py'
--- media_create/populate_boot.py	1970-01-01 00:00:00 +0000
+++ media_create/populate_boot.py	2010-12-03 19:01:21 +0000
@@ -0,0 +1,128 @@
+import glob
+import sys
+
+from media_create import cmd_runner
+
+
+def _run_mkimage(img_type, load_addr, entry_point, name, img_data, img,
+                 stdout=None):
+    cmd = ['mkimage',
+           '-A', 'arm',
+           '-O', 'linux',
+           '-T', img_type,
+           '-C', 'none',
+           '-a', load_addr,
+           '-e', load_addr,
+           '-n', name,
+           '-d', img_data,
+           img]
+    return cmd_runner.run(cmd, as_root=True, stdout=stdout)
+
+
+def _get_file_matching(regex):
+    """Return a file whose path matches the given regex.
+
+    If zero or more than one files match, raise a ValueError.
+    """
+    files = glob.glob(regex)
+    if len(files) == 1:
+        return files[0]
+    elif len(files) == 0:
+        raise ValueError(
+            "No files found matching '%s'; can't continue" % regex)
+    else:
+        # TODO: Could ask the user to chosse which file to use instead of
+        # raising an exception.
+        raise ValueError("Too many files matching '%s' found." % regex)
+
+
+def make_uImage(load_addr, uboot_parts_dir, sub_arch, boot_disk):
+    img_data = _get_file_matching(
+        '%s/vmlinuz-*-%s' % (uboot_parts_dir, sub_arch))
+    img = '%s/uImage' % boot_disk
+    return _run_mkimage(
+        'kernel', load_addr, load_addr, 'Linux', img_data, img)
+
+
+def make_uInitrd(uboot_parts_dir, sub_arch, boot_disk):
+    img_data = _get_file_matching(
+        '%s/initrd.img-*-%s' % (uboot_parts_dir, sub_arch))
+    img = '%s/uInitrd' % boot_disk
+    return _run_mkimage('ramdisk', '0', '0', 'initramfs', img_data, img)
+
+
+def make_boot_script(boot_script, tmp_dir):
+    img_data = '%s/boot.cmd' % tmp_dir
+    return _run_mkimage(
+        'script', '0', '0', 'boot script', img_data, boot_script)
+
+
+def install_mx51evk_boot_loader(imx_file, boot_device_or_file):
+    cmd_runner.run([
+        "dd",
+        "if=%s" % imx_file,
+        "of=%s" % boot_device_or_file,
+        "bs=1024",
+        "seek=1",
+        "conv=notrunc"], as_root=True)
+
+
+def install_omap_boot_loader(mlo_file, boot_disk):
+    cmd_runner.run(["cp", "-v", mlo_file, boot_disk], as_root=True)
+    # XXX: Is this really needed?
+    cmd_runner.run(["sync"])
+
+
+def make_boot_ini(boot_script, boot_disk):
+    cmd_runner.run(
+        ["cp", "-v", boot_script, "%s/boot.ini" % boot_disk], as_root=True)
+
+
+def populate_boot(board, sub_arch, load_addr, uboot_parts_dir, boot_disk,
+                  tmp_dir, boot_script_name, boot_device_or_file):
+
+    boot_script = "%(boot_disk)s/%(boot_script_name)s" % (
+        dict(boot_disk=boot_disk, boot_script_name=boot_script_name))
+
+    # TODO: Once linaro-media-create is fully ported to python, we should
+    # split this into several board-specific functions that are defined
+    # somewhere else and just called here.
+    if board in ["beagle", "panda"]:
+        mlo_file = "binary/usr/lib/x-loader-omap/MLO"
+        if board == "panda":
+            mlo_file = "binary/usr/lib/x-loader-omap4/MLO"
+        install_omap_boot_loader(mlo_file, boot_disk)
+        make_uImage(load_addr, uboot_parts_dir, sub_arch, boot_disk)
+        make_uInitrd(uboot_parts_dir, sub_arch, boot_disk)
+        make_boot_script(boot_script, tmp_dir)
+        make_boot_ini(boot_script, boot_disk)
+
+    elif board == "igep":
+        make_uImage(load_addr, uboot_parts_dir, sub_arch, boot_disk)
+        make_uInitrd(uboot_parts_dir, sub_arch, boot_disk)
+        make_boot_script(boot_script, tmp_dir)
+        make_boot_ini(boot_script, boot_disk)
+
+    elif board == "ux500":
+        make_uImage(load_addr, uboot_parts_dir, sub_arch, boot_disk)
+        make_uInitrd(uboot_parts_dir, sub_arch, boot_disk)
+        make_boot_script(boot_script, tmp_dir)
+
+    elif board == "vexpress":
+        make_uImage(load_addr, uboot_parts_dir, sub_arch, boot_disk)
+        make_uInitrd(uboot_parts_dir, sub_arch, boot_disk)
+
+    elif board == "mx51evk":
+        install_mx51evk_boot_loader(
+            "binary/usr/lib/u-boot/mx51evk/u-boot.imx", boot_device_or_file)
+        make_uImage(load_addr, uboot_parts_dir, sub_arch, boot_disk)
+        make_uInitrd(uboot_parts_dir, sub_arch, boot_disk)
+        make_boot_script(boot_script, tmp_dir)
+
+    else:
+        raise AssertionError(
+            "Internal error; missing support for board: %s" % board)
+
+
+if __name__ == "__main__":
+    sys.exit(populate_boot(*sys.argv[1:]))
 
=== modified file 'media_create/tests/test_media_create.py'
--- media_create/tests/test_media_create.py	2010-12-03 18:48:55 +0000
+++ media_create/tests/test_media_create.py	2010-12-03 19:01:21 +0000
@@ -1,7 +1,10 @@
 from contextlib import contextmanager
 import os
+import random
+import string
 import subprocess
 import sys
+import tempfile
 
 from testtools import TestCase
 
@@ -11,6 +14,14 @@
 from media_create import cmd_runner
 from media_create import ensure_command
 
+from media_create import populate_boot
+from media_create.populate_boot import (
+    make_boot_script,
+    make_uImage,
+    make_uInitrd,
+    _get_file_matching,
+    _run_mkimage,
+    )
 from media_create.remove_binary_dir import remove_binary_dir
 from media_create.unpack_binary_tarball import unpack_binary_tarball
 
@@ -18,6 +29,7 @@
     CreateTempDirFixture,
     CreateTarballFixture,
     MockDoRunFixture,
+    MockSomethingFixture,
     )
 
 
@@ -135,3 +147,89 @@
     def test_do_run(self):
         return_code = cmd_runner.do_run('true')
         self.assertEqual(0, return_code)
+
+
+class TestPopulateBoot(TestCaseWithFixtures):
+
+    def _mock_get_file_matching(self):
+        self.useFixture(MockSomethingFixture(
+            populate_boot, '_get_file_matching',
+            lambda regex: regex))
+
+    def _mock_do_run(self):
+        fixture = MockDoRunFixture()
+        self.useFixture(fixture)
+        return fixture
+
+    def test_make_uImage(self):
+        self._mock_get_file_matching()
+        fixture = self._mock_do_run()
+        make_uImage('load_addr', 'parts_dir', 'sub_arch', 'boot_disk')
+        expected = [
+            'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'kernel',
+            '-C', 'none', '-a', 'load_addr', '-e', 'load_addr', '-n', 'Linux',
+            '-d', 'parts_dir/vmlinuz-*-sub_arch', 'boot_disk/uImage']
+        self.assertEqual(expected, fixture.mock.args)
+
+    def test_make_uInitrd(self):
+        self._mock_get_file_matching()
+        fixture = self._mock_do_run()
+        make_uInitrd('parts_dir', 'sub_arch', 'boot_disk')
+        expected = [
+            'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'ramdisk',
+            '-C', 'none', '-a', '0', '-e', '0', '-n', 'initramfs',
+            '-d', 'parts_dir/initrd.img-*-sub_arch', 'boot_disk/uInitrd']
+        self.assertEqual(expected, fixture.mock.args)
+
+    def test_make_boot_script(self):
+        self._mock_get_file_matching()
+        fixture = self._mock_do_run()
+        make_boot_script('boot_script', 'tmp_dir')
+        expected = [
+            'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'script',
+            '-C', 'none', '-a', '0', '-e', '0', '-n', 'boot script',
+            '-d', 'tmp_dir/boot.cmd', 'boot_script']
+        self.assertEqual(expected, fixture.mock.args)
+
+    def test_get_file_matching(self):
+        prefix = ''.join(
+            random.choice(string.ascii_lowercase) for x in range(5))
+        file1 = self._create_temp_file_as_fixture(prefix)
+        directory = os.path.dirname(file1)
+        self.assertEqual(
+            file1, _get_file_matching('%s/%s*' % (directory, prefix)))
+
+    def test_get_file_matching_too_many_files_found(self):
+        prefix = ''.join(
+            random.choice(string.ascii_lowercase) for x in range(5))
+        file1 = self._create_temp_file_as_fixture(prefix)
+        file2 = self._create_temp_file_as_fixture(prefix)
+        directory = os.path.dirname(file1)
+        self.assertRaises(
+            ValueError, _get_file_matching, '%s/%s*' % (directory, prefix))
+
+    def test_get_file_matching_no_files_found(self):
+        self.assertRaises(
+            ValueError, _get_file_matching, '/foo/bar/baz/*non-existent')
+
+    def test_run_mkimage(self):
+        # Create a fake boot script.
+        filename = self._create_temp_file_as_fixture()
+        f = open(filename, 'w')
+        f.write("setenv bootcmd 'fatload mmc 0:1 0x80000000 uImage;\nboot")
+        f.close()
+
+        img = self._create_temp_file_as_fixture()
+        # Use that fake boot script to create a boot loader using mkimage.
+        # Send stdout to file as mkimage will print to stdout and we don't
+        # want that.
+        retval = _run_mkimage(
+            'script', '0', '0', 'boot script', filename, img,
+            stdout=open(self._create_temp_file_as_fixture(), 'w'))
+
+        self.assertEqual(0, retval)
+
+    def _create_temp_file_as_fixture(self, prefix='tmp'):
+        _, filename = tempfile.mkstemp(prefix=prefix)
+        self.addCleanup(os.unlink, filename)
+        return filename