Merge lp:~salgado/linaro-image-tools/fix-packaging-and-ensure-command into lp:linaro-image-tools/11.11

[Guilherme Salgado]

- change Depends: to add some dependencies that were required in
   practice
 - add a couple qemu-kvm packages to Suggests: although we should add them to
   Depends: once they build successfully on Ubuntu/armel. Is this the correct
   approach or should we add them to Depends: now and render l-i-t
   uninstallable on armel until qemu-kvm builds there?
 - change ensure_command to figure out the package which provides the given
   command so that callsites don't need to specify that.

in a subsequent branch I'll change ensure_command to ask for confirmation
before installing anything

[Loïc Minier (lool)]

Awesome, thanks

One question: why the qemu-kvm dep? I thought only qemu-kvm-extras-static was needed for qemu-arm-static?

I think you could make qemu-kvm-extras-static a Recommends (this reflects that it might not be needed in some combinations). It is a hard requirement on x86 arches right now and useless on arm right now, but it shouldn't be if linaro-image-tools was able to write any image architecture on any host. I think this is preferable to having an arch: any package just to repeat this hardcoding in the packaging.

review: Approve

[Loïc Minier (lool)]

Hmm also, filesystems should be Recommends and perhaps even Suggests in the case of btrfs.

[Guilherme Salgado (salgado)]

On Mon, 2011-01-17 at 18:13 +0000, Loïc Minier wrote:
> Review: Approve
> Awesome, thanks
>
> One question: why the qemu-kvm dep? I thought only qemu-kvm-extras-static was needed for qemu-arm-static?

qemu-kvm is needed because we use qemu-img to generate qemu images.

>
> I think you could make qemu-kvm-extras-static a Recommends (this reflects that it might not be needed in some combinations). It is a hard requirement on x86 arches right now and useless on arm right now, but it shouldn't be if linaro-image-tools was able to write any image architecture on any host. I think this is preferable to having an arch: any package just to repeat this hardcoding in the packaging.

Fair enough, I'll do that.

> Hmm also, filesystems should be Recommends and perhaps even Suggests
> in the case of btrfs.

I've added them to Depends: because at least one of them (e2fsprogs or
btrfs-tools) will be needed. Given that, does it make sense to have
them as Recommends/Suggests or should the one used by default
(e2fsprogs) be a hard dependency and btrfs-tools a suggestion?

[Loïc Minier (lool)]

In this case, you want:
Depends: e2fsprogs | btrfs-tools

[Guilherme Salgado (salgado)]

On Mon, 2011-01-17 at 19:26 +0000, Loïc Minier wrote:
> In this case, you want:
> Depends: e2fsprogs | btrfs-tools

I considered that, but it didn't seem like the right thing to do to me
given that if you happen to have btrfs-tools installed but not e2fsprogs
and try to run the tool with just the essential arguments, it will
either fail or (currently) install e2fsprogs automatically.

[Loïc Minier (lool)]

That's true, but it's not really distinguishable from the case where someone only wants to create btrfs images and doesn't want to install mkfs.extN.

I was actually wrong earlier; we shouldn't test for e2fsprogs as it's Essential: yes, which means it can be omitted from deps as it's always installed, so our whole discussion is moot :-)

Let's just list either Recommends: btrfs-tools or Suggests: btrfs-tools :-)

[Guilherme Salgado (salgado)]

I somehow forgot to bzr add the new utils.py file I created in this branch, so it didn't show up in the diff when the mp was created. Here it is, though: http://bazaar.launchpad.net/~salgado/linaro-image-tools/fix-packaging-and-ensure-command/revision/242

[Loïc Minier (lool)]

utils.py looks good; thanks!

review: Approve

=== modified file 'debian/control'
--- debian/control	2011-01-11 21:26:54 +0000
+++ debian/control	2011-01-18 11:47:15 +0000
@@ -18,8 +18,10 @@
 Package: linaro-image-tools
 Architecture: all
 Depends: ${misc:Depends},
+         util-linux,
          parted,
          uuid-runtime,
+         dosfstools,
          u-boot-tools | uboot-mkimage,
          python,
          python-hwpack (>= ${source:Version}),
@@ -28,6 +30,9 @@
          python-debian (>= 0.1.16ubuntu1),
          python-linaro-media-create (>= ${source:Version}),
          python-parted
+Recommends: qemu-kvm-extras-static,
+            qemu-kvm,
+            btrfs-tools
 Description: collection of tools to work with Linaro images
  This package offers a set of tools for use with Linaro images.
  .

=== modified file 'linaro-media-create'
--- linaro-media-create	2011-01-14 20:29:07 +0000
+++ linaro-media-create	2011-01-18 11:47:15 +0000
@@ -12,7 +12,6 @@
     )
 from linaro_media_create.check_device import (
     confirm_device_selection_and_ensure_it_is_ready)
-from linaro_media_create.ensure_command import ensure_command
 from linaro_media_create.hwpack import install_hwpacks
 from linaro_media_create.partitions import (
     Media,
@@ -21,6 +20,10 @@
 from linaro_media_create.populate_boot import populate_boot
 from linaro_media_create.rootfs import populate_rootfs
 from linaro_media_create.unpack_binary_tarball import unpack_binary_tarball
+from linaro_media_create.utils import (
+    ensure_command,
+    is_arm_host,
+    )
 from linaro_media_create import get_args_parser
 
 
@@ -53,18 +56,16 @@
 def ensure_required_commands(args):
     """Ensure we have the commands that we know are going to be used."""
     required_commands = [
-        ('sfdisk', 'util-linux'),
-        ('fdisk', 'util-linux'),
-        ('mkimage', 'uboot-mkimage'),
-        ('uuidgen', 'uuid-runtime'),
-        ('parted', 'parted'),
-        ]
+        'mkfs.vfat', 'sfdisk', 'fdisk', 'mkimage', 'uuidgen', 'parted']
+    if not is_arm_host():
+        required_commands.append('qemu-arm-static')
+        required_commands.append('qemu-img')
     if args.rootfs in ['ext2', 'ext3', 'ext4']:
-        required_commands.append(('mkfs.%s' % args.rootfs, 'e2fsprogs'))
+        required_commands.append('mkfs.%s' % args.rootfs)
     else:
-        required_commands.append(('mkfs.btrfs', 'btrfs-tools'))
-    for command, package in required_commands:
-        ensure_command(command, package)
+        required_commands.append('mkfs.btrfs')
+    for command in required_commands:
+        ensure_command(command)
 
 
 if __name__ == '__main__':
@@ -72,6 +73,8 @@
     args = parser.parse_args()
     board_config = board_configs[args.board]
 
+    ensure_required_commands(args)
+
     media = Media(args.device)
     if media.is_block_device:
         if not confirm_device_selection_and_ensure_it_is_ready(args.device):

=== removed file 'linaro_media_create/ensure_command.py'
--- linaro_media_create/ensure_command.py	2011-01-07 20:15:56 +0000
+++ linaro_media_create/ensure_command.py	1970-01-01 00:00:00 +0000
@@ -1,13 +0,0 @@
-import os
-import sys
-
-
-def apt_get_install(command, package):
-    print ("Installing required command %s from package %s"
-           % (command, package))
-    os.system('sudo apt-get install %s' % package)
-
-
-def ensure_command(command, package):
-    if os.system('which %s 2>/dev/null 1>/dev/null' % command) != 0:
-        apt_get_install(command, package)

=== modified file 'linaro_media_create/hwpack.py'
--- linaro_media_create/hwpack.py	2011-01-11 21:26:54 +0000
+++ linaro_media_create/hwpack.py	2011-01-18 11:47:15 +0000
@@ -1,8 +1,7 @@
 import os
-import platform
 
 from linaro_media_create import cmd_runner
-from linaro_media_create.ensure_command import ensure_command
+from linaro_media_create.utils import is_arm_host
 
 
 # It'd be nice if we could use atexit here, but all the things we need to undo
@@ -17,9 +16,7 @@
     temporarily_overwrite_file_on_dir('/etc/resolv.conf', chroot_etc, tmp_dir)
     temporarily_overwrite_file_on_dir('/etc/hosts', chroot_etc, tmp_dir)
 
-    if not platform.machine().startswith('arm'):
-        ensure_command('qemu-arm-static', 'qemu-arm-static')
-        ensure_command('qemu-img', 'qemu-kvm')
+    if not is_arm_host():
         copy_file('/usr/bin/qemu-arm-static',
                   os.path.join(chroot_dir, 'usr', 'bin'))
 

=== modified file 'linaro_media_create/tests/fixtures.py'
--- linaro_media_create/tests/fixtures.py	2011-01-11 21:26:54 +0000
+++ linaro_media_create/tests/fixtures.py	2011-01-18 11:47:15 +0000
@@ -85,6 +85,10 @@
     def wait(self):
         return self.returncode
 
+    @property
+    def commands_executed(self):
+        return [' '.join(args) for args in self.calls]
+
 
 class MockCmdRunnerPopenFixture(MockSomethingFixture):
     """A test fixture which mocks cmd_runner.do_run with the given mock.

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-01-18 10:16:45 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-01-18 11:47:15 +0000
@@ -16,10 +16,10 @@
 from linaro_media_create import (
     check_device,
     cmd_runner,
-    ensure_command,
     boards,
     partitions,
     rootfs,
+    utils,
     )
 from linaro_media_create.boards import (
     board_configs,
@@ -57,6 +57,11 @@
     write_data_to_protected_file,
     )
 from linaro_media_create.unpack_binary_tarball import unpack_binary_tarball
+from linaro_media_create.utils import (
+    ensure_command,
+    install_package_providing,
+    UnableToFindPackageProvidingCommand,
+    )
 
 from linaro_media_create.tests.fixtures import (
     CreateTempDirFixture,
@@ -67,28 +72,46 @@
     )
 
 
-class TestEnsureCommand(TestCase):
-
-    apt_get_called = False
+class TestEnsureCommand(TestCaseWithFixtures):
+
+    install_pkg_providing_called = False
+
+    def setUp(self):
+        super(TestEnsureCommand, self).setUp()
+        self.useFixture(MockSomethingFixture(
+            sys, 'stdout', open('/dev/null', 'w')))
 
     def test_command_already_present(self):
-        with self.mock_apt_get_install():
-            ensure_command.ensure_command('apt-get', 'apt')
-        self.assertFalse(self.apt_get_called)
+        self.mock_install_package_providing()
+        ensure_command('apt-get')
+        self.assertFalse(self.install_pkg_providing_called)
 
     def test_command_not_present(self):
-        with self.mock_apt_get_install():
-            ensure_command.ensure_command('apt-get-two-o', 'apt-2')
-        self.assertTrue(self.apt_get_called)
-
-    @contextmanager
-    def mock_apt_get_install(self):
-        def mock_apt_get_install(cmd, pkg):
-            self.apt_get_called = True
-        orig_func = ensure_command.apt_get_install
-        ensure_command.apt_get_install = mock_apt_get_install
-        yield
-        ensure_command.apt_get_install = orig_func
+        self.mock_install_package_providing()
+        ensure_command('apt-get-two-o')
+        self.assertTrue(self.install_pkg_providing_called)
+
+    def mock_install_package_providing(self):
+        def mock_func(command):
+            self.install_pkg_providing_called = True
+        self.useFixture(MockSomethingFixture(
+            utils, 'install_package_providing', mock_func))
+
+
+class TestInstallPackageProviding(TestCaseWithFixtures):
+
+    def test_found_package(self):
+        fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        install_package_providing('mkfs.vfat')
+        self.assertEqual(
+            ['sudo apt-get install dosfstools'],
+            fixture.mock.commands_executed)
+
+    def test_not_found_package(self):
+        fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        self.assertRaises(
+            UnableToFindPackageProvidingCommand,
+            install_package_providing, 'mkfs.lean')
 
 
 class TestGetBootCmd(TestCase):