Merge lp:~lool/linaro-image-tools/testsuite-when-installed into lp:linaro-image-tools/11.11

[James Westby]

[James Westby (james-w)]

I'm not sure that the

116 + if os.path.isabs(__file__):

check is really what we want.

I think that if there is a linaro-hwpack-install alongside the linaro-media-create,
then we should always use it, regardless of $PATH, or how it is being executed.

That's probably not true of all commands though.

It's probably worth looking back over the discussion around using the correct
linaro-hwpack-install from when the code to use one in the current dir was
added.

Thanks,

James

review: Needs Information

[Loïc Minier (lool)]

(Thanks for adding the dep on the sudo branch; I failed to push the sudo branch after merging it locally -- it's merged in trunk now)

The isbas(__file__) test is just a test for whether or not we're running from bzr.

Note that __file__ is the possibly relative pathname of the current module, so of hwpack.py or utils.py.

The assumptions are:
* if you're running from a checkout, __file__ is relative, you're in the top directory and commands are in the current directory
* if you're not running from a checkout, __file__ is absolute, commands are in the PATH

[Loïc Minier (lool)]

Followup IRC chat:

15:09 < james_w> lool, We have to be very careful to use a matching
                 linaro-hwpack-create for our linaro-image-tools.

15:10 < james_w> lool, so when looking for that command, I think we should
                 always take one from next to the linaro-image-tools script,
                 before searching $PATH
15:10 < james_w> I think your change will approximate that in most cases, but
                 I'm not sure if it goes far enough to be robust
15:11 < lool> james_w: So you're saying, additionally to all things I've
              listed, we should first look for linaro-hwpack-* in the directory
              where linaro-media-create is, then fallback to PATH?
15:13 < james_w> lool, that's what I think
15:13 < lool> james_w: It makes sense
15:14 < lool> james_w: albeit I can think that if you have multiple
              linaro-hwpack-*, then you probably also have multiple python
              modules, and you're asking for trouble if PYTHONPATH and PATH
              don't point at the same versions in the same order
15:14 < james_w> lool, indeed
15:15 < lool> james_w: Do you still want this covered? We could do it either
              by passing directories across calls, or by prepending PATH
              immediately
15:15 < lool> But prepending PATH might have side effects when looking for
              other commands

15:16 < james_w> lool, right, I imagine it should just be for
                 linaro-hwpack-install

[Loïc Minier (lool)]

Ok; I pushed up to r280 to implement support for preferring the dir with which linaro-media-create is run; it was a bit more intrusive than I wished for

[lp:~lool/linaro-image-tools/testsuite-when-installed]

278. By Loïc Minier on 2011-02-02

Add an optional prefer_dir arg to find_command to allow searching a specific
directory first.

279. By Loïc Minier on 2011-02-02

Pass a tools dir (preferred directory) to install_hwpacks; should be the
directory of linaro-media-create when it's run with an explicit directory, or
"." when running from a bzr checkout.

280. By Loïc Minier on 2011-02-02

More explicit handling of empty PATH components as "." in find_command.

[Loïc Minier (lool)]

I tried supporting hwpack.tests as well, and had to add r281 for this; with r281 and this diff:

=== modified file 'hwpack/tests/test_script.py'
--- hwpack/tests/test_script.py 2011-01-28 19:50:48 +0000
+++ hwpack/tests/test_script.py 2011-02-02 16:40:28 +0000
@@ -31,6 +31,7 @@
     IsHardwarePack,
     TestCaseWithFixtures,
     )
+from linaro_media_create.utils import find_command

 class ScriptTests(TestCaseWithFixtures):
@@ -38,26 +39,16 @@

     def setUp(self):
         super(ScriptTests, self).setUp()
- self.script_path = self.find_script()
+
+ prefer_dir = None
+ # running from bzr checkout?
+ if not os.path.isabs(__file__):
+ prefer_dir = os.getcwd()
+
+ self.script_path = find_command(
+ 'linaro-hwpack-create', prefer_dir=prefer_dir)
         self.useFixture(ChdirToTempdirFixture())

- def find_script(self):
- script_name = "linaro-hwpack-create"
- this_path = os.path.abspath(__file__)
- parent_path = this_path
- for i in range(3):
- parent_path = os.path.dirname(parent_path)
- possible_paths = [
- os.path.join(parent_path, script_name),
- os.path.join("usr", "local", "bin", script_name),
- os.path.join("usr", "bin", script_name),
- ]
- for script_path in possible_paths:
- if os.path.exists(script_path):
- return script_path
- raise AssertionError(
- "Could not find linaro-hwpack-create script to test.")
-
     def run_script(self, args, expected_returncode=0):
         cmdline = [self.script_path] + args
         proc = subprocess.Popen(

all tests pass from a bzr checkout and when installed.

[lp:~lool/linaro-image-tools/testsuite-when-installed]

281. By Loïc Minier on 2011-02-02

Default to os.getcwd() instead of "." as "." breaks with some
ChdirToTempdirFixture based tests.

[James Westby (james-w)]

Looks good, aside from the merge conflicts.

Thanks,

James

review: Approve

=== modified file 'linaro-media-create'
--- linaro-media-create	2011-02-02 12:08:05 +0000
+++ linaro-media-create	2011-02-02 16:43:03 +0000
@@ -62,6 +62,7 @@
     Before doing so, make sure BOOT_DISK and ROOT_DISK are not mounted.
     """
     devnull = open('/dev/null', 'w')
+<<<<<<< TREE
     # ignore non-zero return codes
     for disk in BOOT_DISK, ROOT_DISK:
         if disk is not None:
@@ -70,10 +71,26 @@
                       stdout=devnull, stderr=devnull, as_root=True).wait()
             except cmd_runner.SubcommandNonZeroReturnValue:
                 pass
+=======
+    # ignore non-zero return codes
+    try:
+        if BOOT_DISK is not None:
+            cmd_runner.Popen(['umount', BOOT_DISK],
+                  stdout=devnull, stderr=devnull, as_root=True).wait()
+        if ROOT_DISK is not None:
+            cmd_runner.Popen(['umount', ROOT_DISK],
+                  stdout=devnull, stderr=devnull, as_root=True).wait()
+    except SubcommandNonZeroReturnValue:
+        pass
+>>>>>>> MERGE-SOURCE
     # Remove TMP_DIR as root because some files written there are
     # owned by root.
     if TMP_DIR is not None:
+<<<<<<< TREE
         cmd_runner.run(['rm', '-rf', TMP_DIR], as_root=True).wait()
+=======
+        cmd_runner.Popen(['rm', '-rf', TMP_DIR], as_root=True).wait()
+>>>>>>> MERGE-SOURCE
 
 
 def ensure_required_commands(args):
@@ -121,7 +138,11 @@
     unpack_binary_tarball(args.binary, TMP_DIR)
 
     hwpacks = args.hwpacks
-    install_hwpacks(ROOTFS_DIR, TMP_DIR, args.hwpack_force_yes, *hwpacks)
+    lmc_dir = os.path.dirname(__file__)
+    if lmc_dir == '':
+        lmc_dir = None
+    install_hwpacks(
+        ROOTFS_DIR, TMP_DIR, lmc_dir, args.hwpack_force_yes, *hwpacks)
 
     boot_partition, root_partition = setup_partitions(
         board_config, media, args.image_size, args.boot_label, args.rfs_label,
 
=== modified file 'linaro_media_create/hwpack.py'
--- linaro_media_create/hwpack.py	2011-02-01 17:07:04 +0000
+++ linaro_media_create/hwpack.py	2011-02-02 16:43:03 +0000
@@ -21,7 +21,10 @@
 import sys
 
 from linaro_media_create import cmd_runner
-from linaro_media_create.utils import is_arm_host
+from linaro_media_create.utils import (
+    is_arm_host,
+    find_command,
+    )
 
 
 # It'd be nice if we could use atexit here, but all the things we need to undo
@@ -29,7 +32,8 @@
 # functions would only be called after l-m-c.py exits.
 local_atexit = []
 
-def install_hwpacks(chroot_dir, tmp_dir, hwpack_force_yes, *hwpack_files):
+def install_hwpacks(
+    chroot_dir, tmp_dir, tools_dir, hwpack_force_yes, *hwpack_files):
     """Install the given hwpacks onto the given chroot."""
 
     chroot_etc = os.path.join(chroot_dir, 'etc')
@@ -40,13 +44,11 @@
         copy_file('/usr/bin/qemu-arm-static',
                   os.path.join(chroot_dir, 'usr', 'bin'))
 
-    # FIXME: This is an ugly hack to make sure we use the l-h-i script from
-    # the current development tree when possible.
-    here = os.path.dirname(__file__)
-    linaro_hwpack_install_path = os.path.join(
-        here, '..', 'linaro-hwpack-install')
-    if not os.path.exists(linaro_hwpack_install_path):
-        linaro_hwpack_install_path = '/usr/bin/linaro-hwpack-install'
+    linaro_hwpack_install_path = find_command(
+        'linaro-hwpack-install', prefer_dir=tools_dir)
+    # FIXME: shouldn't use chroot/usr/bin as this might conflict with installed
+    # packages; would be best to use some custom directory like
+    # chroot/linaro-image-tools/bin
     copy_file(linaro_hwpack_install_path,
               os.path.join(chroot_dir, 'usr', 'bin'))
 
 
=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-02-01 17:29:25 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-02-02 16:43:03 +0000
@@ -21,6 +21,7 @@
 import glob
 import os
 import random
+import stat
 import string
 import subprocess
 import sys
@@ -82,6 +83,7 @@
 from linaro_media_create.unpack_binary_tarball import unpack_binary_tarball
 from linaro_media_create.utils import (
     ensure_command,
+    find_command,
     install_package_providing,
     UnableToFindPackageProvidingCommand,
     )
@@ -121,6 +123,32 @@
             utils, 'install_package_providing', mock_func))
 
 
+class TestFindCommand(TestCaseWithFixtures):
+
+    def test_preferred_dir(self):
+        tempdir = self.useFixture(CreateTempDirFixture()).get_temp_dir()
+        lmc = 'linaro-media-create'
+        path = os.path.join(tempdir, lmc)
+        open(path, 'w').close()
+        os.chmod(path, stat.S_IXUSR)
+        self.assertEquals(path, find_command(lmc, tempdir))
+
+    def test_existing_command(self):
+        lmc = 'linaro-media-create'
+        # running from bzr checkout?
+        if os.path.isabs(__file__):
+            expected, _ = cmd_runner.run(
+                ['which', lmc, ],
+                stdout=subprocess.PIPE).communicate()
+            expected = expected.strip()
+        else:
+            expected = os.path.join(os.getcwd(), lmc)
+        self.assertEquals(expected, find_command(lmc))
+
+    def test_nonexisting_command(self):
+        self.assertEquals(find_command('linaro-moo'), None)
+
+
 class TestInstallPackageProviding(TestCaseWithFixtures):
 
     def test_found_package(self):
@@ -1083,8 +1111,17 @@
             sys, 'stdout', open('/dev/null', 'w')))
         fixture = self.useFixture(MockCmdRunnerPopenFixture())
         force_yes = True
+
+        prefer_dir = None
+        # running from bzr checkout?
+        if not os.path.isabs(__file__):
+            prefer_dir = os.getcwd()
+
         install_hwpacks(
-            'chroot', '/tmp/dir', force_yes, 'hwpack1.tgz', 'hwpack2.tgz')
+            'chroot', '/tmp/dir', prefer_dir, force_yes, 'hwpack1.tgz',
+            'hwpack2.tgz')
+        linaro_hwpack_install = find_command(
+            'linaro-hwpack-install', prefer_dir=prefer_dir)
         self.assertEquals(
             [['sudo', 'mv', '-f', 'chroot/etc/resolv.conf',
               '/tmp/dir/resolv.conf'],
@@ -1092,8 +1129,7 @@
              ['sudo', 'mv', '-f', 'chroot/etc/hosts', '/tmp/dir/hosts'],
              ['sudo', 'cp', '/etc/hosts', 'chroot/etc'],
              ['sudo', 'cp', '/usr/bin/qemu-arm-static', 'chroot/usr/bin'],
-             ['sudo', 'cp', 'linaro_media_create/../linaro-hwpack-install',
-              'chroot/usr/bin'],
+             ['sudo', 'cp', linaro_hwpack_install, 'chroot/usr/bin'],
              ['sudo', 'mount', 'proc', 'chroot/proc', '-t', 'proc'],
              ['sudo', 'cp', 'hwpack1.tgz', 'chroot'],
              ['sudo', 'chroot', 'chroot', 'linaro-hwpack-install',
 
=== modified file 'linaro_media_create/utils.py'
--- linaro_media_create/utils.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/utils.py	2011-02-02 16:43:03 +0000
@@ -17,6 +17,7 @@
 # You should have received a copy of the GNU General Public License
 # along with Linaro Image Tools.  If not, see <http://www.gnu.org/licenses/>.
 
+import os
 import platform
 
 try:
@@ -61,6 +62,37 @@
     except cmd_runner.SubcommandNonZeroReturnValue:
         install_package_providing(command)
 
+def find_command(name, prefer_dir=None):
+    """Finds a linaro-image-tools command.
+
+    Prefers specified directory, otherwise searches only the current directory
+    when running from a checkout, or only PATH when running from an installed
+    version.
+    """
+    assert name != ""
+    assert os.path.dirname(name) == ""
+
+    if not os.environ.has_key("PATH"):
+        os.environ["PATH"] = ":/bin:usr/bin"
+
+    # default to searching in current directory when running from a bzr
+    # checkout
+    dirs = [os.getcwd(),]
+    if os.path.isabs(__file__):
+        dirs = os.environ["PATH"].split(os.pathsep)
+        # empty dir in PATH means current directory
+        dirs = map(lambda x: x == '' and '.' or x, dirs)
+
+    if prefer_dir is not None:
+        dirs.insert(0, prefer_dir)
+
+    for dir in dirs:
+        path = os.path.join(dir, name)
+        if os.path.exists(path) and os.access(path, os.X_OK):
+            return path
+
+    return None
+
 
 def is_arm_host():
     return platform.machine().startswith('arm')