Merge lp:~lool/linaro-image-tools/missing-sbin into lp:linaro-image-tools/11.11

[Loïc Minier]

Fixes runs of the testsuite (didn't check actually running linaro-media-create or other helpers) when /sbin isn't in the PATH, or when PATH is empty, or when PATH isn't set; bug #709517.

This is a very crude fix, but it seems safe; please shout if you think this is too ugly and we should go with the other approach I describe in the bug.

I tested this with:
# /sbin missing from PATH
PATH=/usr/local/bin:/usr/bin:/bin testr run
# PATH set but empty
PATH="" /usr/bin/python -m subunit.run linaro_image_tools.tests.test_suite|grep -v ^test:|grep -v ^successful:
# PATH unset
env -i testr run
env -u PATH /usr/bin/python -m subunit.run linaro_image_tools.tests.test_suite|grep -v ^test:|grep -v ^successful:

[Loïc Minier (lool)]

I've pushed a quite different approach now; the bad part is that the hwpack testsuite doesn't pass anymore when PATH is broken, this is because the hwpack code uses subprocess.Popen directly.

What I would propose is that we convert the hwpack code to use cmd_runner.

I also need to add tests for sanitize_path, but wouldn't mind some feedback on the overall approach.

[Guilherme Salgado (salgado)]

I don't quite like the fact that we have to "sanitize" both the env passed to Popen() and os.environ, but I don't see a better way to solve this problem now. Also, why do we need the sanitize() call in linaro_image_tools/media_create/utils.py?

I'd like to know what James thinks of this

[Loïc Minier (lool)]

On Thu, Mar 24, 2011, Guilherme Salgado wrote:
> Also, why do we need the sanitize() call in
> linaro_image_tools/media_create/utils.py?

 find_command was using custom code to make sure PATH was set and had
 some directories in it; it was more generic to simply make sure the
 PATH is correct (which has a global effect on os.environ the first time
 you create a Popen() anyway).

 This reminds me that I'd like to propose we move find_command to
 cmd_runner() as it relates to PATH / command handling too.

--
Loïc Minier

[James Westby (james-w)]

On Thu, 24 Mar 2011 14:44:26 -0000, Guilherme Salgado <email address hidden> wrote:
> I don't quite like the fact that we have to "sanitize" both the env
> passed to Popen() and os.environ, but I don't see a better way to
> solve this problem now. Also, why do we need the sanitize() call in
> linaro_image_tools/media_create/utils.py?

> I'd like to know what James thinks of this

I don't have a problem with it, though I share your concern.

I wonder what the hwpack failures are though.

Thanks,

James

[Loïc Minier (lool)]

On Thu, Mar 24, 2011, James Westby wrote:
> I wonder what the hwpack failures are though.

 It's when running dpkg-deb for instance; it wont be found if PATH
 doesn't contain /usr/bin.

 I find very unlikely that someone would screw up the PATH like this,
 but saw that while testing for broken PATH and since we're fixing the
 PATH for /sbin, it's just as easy to fix it for the other standard
 dirs.

--
Loïc Minier

[Guilherme Salgado (salgado)]

 review approve

On Thu, 2011-03-24 at 14:13 +0000, Loïc Minier wrote:
> === modified file 'linaro_image_tools/media_create/cmd_runner.py'
> --- linaro_image_tools/media_create/cmd_runner.py 2011-03-09 15:11:28 +0000
> +++ linaro_image_tools/media_create/cmd_runner.py 2011-03-24 14:11:34 +0000
> @@ -20,6 +20,14 @@
> import os
> import subprocess
>
> +def sanitize_path(env):
> + """Makes sure PATH is set and has important directories"""
> + default = '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
> + dirs = env.get('PATH', default).split(os.pathsep)
> + for d in default.split(os.pathsep):
> + if d not in dirs:
> + dirs.append(d)
> + env['PATH'] = os.pathsep.join(dirs)

This can be slightly simplified if you use a set():

  if 'PATH' in env:
    dirs = env.get('PATH').split(os.pathsep)
  else:
    dirs = []
  default = ['/usr/local/sbin', ...]
  env['PATH'] = os.pathsep.join(set(dirs).union(default))

It doesn't simplify much but at least allows us to get rid of the
hard-coded pathsep in the string assigned to 'default'.

review: Approve

[James Westby (james-w)]

On Thu, 24 Mar 2011 17:49:15 -0000, Guilherme Salgado <email address hidden> wrote:
> This can be slightly simplified if you use a set():
>
> if 'PATH' in env:
> dirs = env.get('PATH').split(os.pathsep)
> else:
> dirs = []
> default = ['/usr/local/sbin', ...]
> env['PATH'] = os.pathsep.join(set(dirs).union(default))
>
> It doesn't simplify much but at least allows us to get rid of the
> hard-coded pathsep in the string assigned to 'default'.

set() is no good as it will shuffle the entries in PATH won't it?
That would lead to it picking commands to run at random.

Thanks,

James

[Guilherme Salgado (salgado)]

Yeah, good catch; we definitely don't want to use set() here.

[lp:~lool/linaro-image-tools/missing-sbin]

309. By Loïc Minier on 2011-03-24

Use cmd_runner instead of subprocess, this ensures that a sane PATH is used.

[Loïc Minier (lool)]

To illustrate the hwpack failures, one can run:
PATH=/foo /usr/bin/python -m subunit.run linaro_image_tools.tests.test_suite|grep -v ^test:|grep -v ^successful:
this gave output like:
error: linaro_image_tools.hwpack.tests.test_builder.HardwarePackBuilderTests.test_builds_correct_contents [ multipart
Content-Type: text/x-traceback;charset=utf8,language=python
traceback
449
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/testtools/runtest.py", line 169, in _run_user
    return fn(*args, **kwargs)
  File "/usr/lib/python2.6/dist-packages/testtools/testcase.py", line 499, in _run_test_method
    return self._get_test_method()()
  File "linaro_image_tools/hwpack/tests/test_builder.py", line 129, in test_builds_correct_contents
    builder.build()
  File "linaro_image_tools/hwpack/builder.py", line 78, in build
    local_packages, LOCAL_ARCHIVE_LABEL))
  File "linaro_image_tools/hwpack/packages.py", line 199, in sources_entry_for_debs
    stdout=open(os.path.join(tmpdir, 'Release'), 'w'))
  File "/usr/lib/python2.6/subprocess.py", line 483, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib/python2.6/subprocess.py", line 470, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.6/subprocess.py", line 623, in __init__
    errread, errwrite)
  File "/usr/lib/python2.6/subprocess.py", line 1141, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
0
]

I've just changed one file to use cmd_runner, and kept the testing code using subprocess.

[lp:~lool/linaro-image-tools/missing-sbin]

310. By Loïc Minier on 2011-03-24

Merge lp:linaro-image-tools; update cmd_runner import path.

311. By Loïc Minier on 2011-03-24

Expose default path of sanitize_path() as to help testing and add tests for
sanitize_path().

=== modified file 'linaro_image_tools/cmd_runner.py'
--- linaro_image_tools/cmd_runner.py	2011-03-24 10:48:18 +0000
+++ linaro_image_tools/cmd_runner.py	2011-03-24 22:15:56 +0000
@@ -21,9 +21,19 @@
 import subprocess
 
 
+DEFAULT_PATH = '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
 SUDO_ARGS = ['sudo', '-E']
 
 
+def sanitize_path(env):
+    """Makes sure PATH is set and has important directories"""
+    dirs = env.get('PATH', DEFAULT_PATH).split(os.pathsep)
+    for d in DEFAULT_PATH.split(os.pathsep):
+        if d not in dirs:
+            dirs.append(d)
+    env['PATH'] = os.pathsep.join(dirs)
+
+
 def run(args, as_root=False, stdin=None, stdout=None, stderr=None):
     """Run the given command as a sub process.
 
@@ -59,6 +69,10 @@
         if env is None:
             env = os.environ.copy()
         env['LC_ALL'] = 'C'
+        # ensure a proper PATH before calling Popen
+        sanitize_path(os.environ)
+        # and for subcommands
+        sanitize_path(env)
         super(Popen, self).__init__(args, env=env, **kwargs)
 
     def wait(self):

=== modified file 'linaro_image_tools/hwpack/packages.py'
--- linaro_image_tools/hwpack/packages.py	2011-01-28 19:50:48 +0000
+++ linaro_image_tools/hwpack/packages.py	2011-03-24 22:15:56 +0000
@@ -34,6 +34,8 @@
 
 from debian.debfile import DebFile
 
+from linaro_image_tools import cmd_runner
+
 
 logger = logging.getLogger(__name__)
 
@@ -191,12 +193,12 @@
         with open(os.path.join(tmpdir, 'Packages'), 'w') as packages_file:
             packages_file.write(get_packages_file(local_debs, rel_to=tmpdir))
         if label:
-            subprocess.check_call(
+            proc = cmd_runner.run(
                 ['apt-ftparchive',
                  '-oAPT::FTPArchive::Release::Label=%s' % label,
                  'release',
                  tmpdir],
-                stdout=open(os.path.join(tmpdir, 'Release'), 'w'))
+                stdout=open(os.path.join(tmpdir, 'Release'), 'w')).wait()
         return 'file://%s ./' % (tmpdir, )
 
 
@@ -245,7 +247,7 @@
         env = os.environ
         env['LC_ALL'] = 'C'
         env['NO_PKG_MANGLE'] = '1'
-        proc = subprocess.Popen(
+        proc = cmd_runner.Popen(
             ['dpkg-deb', '-b', packaging_dir],
             env=env,
             stdout=subprocess.PIPE, stderr=subprocess.PIPE)

=== modified file 'linaro_image_tools/tests/test_cmd_runner.py'
--- linaro_image_tools/tests/test_cmd_runner.py	2011-03-24 17:58:26 +0000
+++ linaro_image_tools/tests/test_cmd_runner.py	2011-03-24 22:15:56 +0000
@@ -30,6 +30,27 @@
 sudo_args = " ".join(cmd_runner.SUDO_ARGS)
 
 
+class TestSanitizePath(TestCaseWithFixtures):
+    def setUp(self):
+        super(TestSanitizePath, self).setUp()
+        self.env = {}
+
+    def test_path_unset(self):
+        cmd_runner.sanitize_path(self.env)
+        self.assertEqual(cmd_runner.DEFAULT_PATH, self.env['PATH'])
+
+    def test_path_missing_dirs(self):
+        path = '/bin:/sbin:/foo:/usr/local/sbin'
+        self.env['PATH'] = path
+        cmd_runner.sanitize_path(self.env)
+        expected = '%s:/usr/local/bin:/usr/sbin:/usr/bin' % path
+        self.assertEqual(expected, self.env['PATH'])
+
+    def test_idempotent(self):
+        self.env['PATH'] = cmd_runner.DEFAULT_PATH
+        cmd_runner.sanitize_path(self.env)
+        self.assertEqual(cmd_runner.DEFAULT_PATH, self.env['PATH'])
+
 class TestCmdRunner(TestCaseWithFixtures):
 
     def test_run(self):

=== modified file 'linaro_image_tools/utils.py'
--- linaro_image_tools/utils.py	2011-03-24 18:41:59 +0000
+++ linaro_image_tools/utils.py	2011-03-24 22:15:56 +0000
@@ -73,8 +73,7 @@
     assert name != ""
     assert os.path.dirname(name) == ""
 
-    if not os.environ.has_key("PATH"):
-        os.environ["PATH"] = ":/bin:usr/bin"
+    cmd_runner.sanitize_path(os.environ)
 
     # default to searching in current directory when running from a bzr
     # checkout