Merge lp:~milo/linaro-image-tools/bug727776 into lp:linaro-image-tools/11.11

[Milo Casagrande]

Hi,

as per bug 727776, I'm proposing the following branch for merge.

I added a new test case for the '--hwpack' argument, checking it against a temp directory and raising an InvalidHwpackFile exception. I added then a new check in additional_option_checkers to check that the argument passed for '--hwpack' is a regular file.

For the test case I based myself on the already defined test cases. I only had to define my Args class in order to pass the valid arguments.

I ran the tests suite and it runs without errors.
Thanks.

[Данило Шеган (danilo)]

Hi Milo,

Thanks for working on this.

У уто, 29. 05 2012. у 07:40 +0000, Milo Casagrande пише:

> as per bug 727776, I'm proposing the following branch for merge.
>
> I added a new test case for the '--hwpack' argument, checking it
> against a temp directory and raising an InvalidHwpackFile exception. I
> added then a new check in additional_option_checkers to check that the
> argument passed for '--hwpack' is a regular file.
>
> For the test case I based myself on the already defined test cases. I
> only had to define my Args class in order to pass the valid arguments.

> I ran the tests suite and it runs without errors.
> Thanks.

Cool, it's worth running pep8 on the files you've modified as well:

$ pep8 linaro_image_tools/tests/test_utils.py linaro_image_tools/utils.py
linaro_image_tools/tests/test_utils.py:90:5: E303 too many blank lines (2)
linaro_image_tools/tests/test_utils.py:140:1: W293 blank line contains whitespace
linaro_image_tools/tests/test_utils.py:198:1: E302 expected 2 blank lines, found 1
linaro_image_tools/tests/test_utils.py:321:40: W291 trailing whitespace
linaro_image_tools/utils.py:46:80: E501 line too long (81 characters)
linaro_image_tools/utils.py:107:14: E111 indentation is not a multiple of four
linaro_image_tools/utils.py:221:24: E231 missing whitespace after ','
linaro_image_tools/utils.py:256:17: E261 at least two spaces before inline comment
linaro_image_tools/utils.py:297:1: W391 blank line at end of file

(most/all are probably not your faults, but they'd be nice to clean-up
so we work towards a cleaner looking code :)

> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'linaro_image_tools/tests/test_utils.py'
> --- linaro_image_tools/tests/test_utils.py 2012-04-18 13:26:25 +0000
> +++ linaro_image_tools/tests/test_utils.py 2012-05-29 07:39:22 +0000
> @@ -44,6 +44,7 @@
> IncompatibleOptions,
> prep_media_path,
> additional_option_checks,
> + InvalidHwpackFile,
> )

It would be good to sort these alphabetically as well: that makes it
easier to see if an import is already there or not when one tries to add
something.

And if there are many problems like this (and pep8 stuff) that one
notices during development, it's usually better to leave them for after
the review (or review might be harder).

> sudo_args = " ".join(cmd_runner.SUDO_ARGS)
> @@ -286,6 +287,7 @@
> device="testdevice",
> board="testboard")))
>
> +

Thanks for fixing this one :)

> class TestPrepMediaPath(TestCaseWithFixtures):
>
> def test_additional_option_checks(self):
> @@ -303,3 +305,21 @@
> device="testdevice",
> board="testboard"))
> sys.argv.remove("--mmc")
> +
> +
> +class TestHwpackIsFile(TestCaseWithFixtures):
> +
> + """Testing '--hwpack' option only allows file."""

"only allows regular files." sounds a bit better and is more
descriptive.

Rest of the test looks good.

> === modified file 'linaro_image_tools/utils.py'
> --- linaro_image_tools/utils.py 2012-04-18 13:26:25 +0000
> +++ linaro_image_tools/utils.py ...

review: Needs Fixing

[lp:~milo/linaro-image-tools/bug727776]

526. By Milo Casagrande on 2012-05-30

Fixed typos and test description.

527. By Milo Casagrande on 2012-05-31

Fixed import sorting in alphabetical order.

528. By Milo Casagrande on 2012-05-31

Added a new test for additional hwpack checks.

 * Added new test checking multiple hwpacks, in order to prevent
   tweaking of the additional_option_checks.

[Milo Casagrande (milo)]

Hi Danilo,

I pushed some of the requested changes, following are my comments.

On Wed, May 30, 2012 at 5:32 PM, Данило Шеган <email address hidden> wrote:
>
> Cool, it's worth running pep8 on the files you've modified as well:
>
> (most/all are probably not your faults, but they'd be nice to clean-up
> so we work towards a cleaner looking code :)

I will work on this as a second step, I try to fix all the problems
introduced by my changes first, and then move to those.

The latest commit/push on my branch doesn't address these yet.

> It would be good to sort these alphabetically as well: that makes it
> easier to see if an import is already there or not when one tries to add
> something.

This should be fixed now.

> "only allows regular files." sounds a bit better and is more
> descriptive.
>
> Rest of the test looks good.
>
> Now that I am looking at this code, it'd be nice to fix the above typo:
>
>  s/incompatable/incompatible/

Typos and rewriting should be fixed now.

>> +
>> +    for hwpack in args.hwpacks:
>> +        if not os.path.isfile(hwpack):
>> +            raise InvalidHwpackFile(
>> +                "--hwpack argument (%s) is not a regular file" % hwpack)
>> +
>
> The code here actually brings up one point: it might be useful to add a
> test to ensure multiple hwpacks are handled properly as well.  I.e. we
> can have a hwpack pointing to a real file, and only the second one
> pointing to a directory or non-existing file.
>
> This would ensure that if someone touches this code and decides to just
> check for the first argument, they get told about how they've broke it
> by the test failing :)

I added a new test for this:

test_hwpacks_are_files

that tests a file and a directory, in that order, passed in a list. It
is basically the same as the test before, just with two arguments for
the hwpack option. I added it inside the same class defined for the
test before, without renaming the class (maybe would be better to
rename it to address the multiple tests it contains, maybe a general
'TestHwpack').

I will address the other changes in later commits.
Let me know.

--
Milo Casagrande

[Milo Casagrande (milo)]

The additional PEP8 fixes are in a separate branch: once the review for the new test is OK, I will merge that branch here too.

Cheers.

[Данило Шеган (danilo)]

Looks good, thanks for working on this.

  merge approve

review: Approve

[lp:~milo/linaro-image-tools/bug727776]

529. By Milo Casagrande on 2012-06-06

Applied PEP8 changes.

[Milo Casagrande (milo)]

I merged also the final bits to clean up PEP8 warnings: moslty spaces and white lines.

=== modified file 'linaro_image_tools/tests/test_utils.py'
--- linaro_image_tools/tests/test_utils.py	2012-04-18 13:26:25 +0000
+++ linaro_image_tools/tests/test_utils.py	2012-06-06 14:02:18 +0000
@@ -33,19 +33,21 @@
     MockSomethingFixture,
     )
 from linaro_image_tools.utils import (
+    IncompatibleOptions,
+    InvalidHwpackFile,
+    UnableToFindPackageProvidingCommand,
+    additional_option_checks,
+    check_file_integrity_and_log_errors,
     ensure_command,
     find_command,
     install_package_providing,
+    path_in_tarfile_exists,
     preferred_tools_dir,
-    UnableToFindPackageProvidingCommand,
+    prep_media_path,
     verify_file_integrity,
-    check_file_integrity_and_log_errors,
-    path_in_tarfile_exists,
-    IncompatibleOptions,
-    prep_media_path,
-    additional_option_checks,
     )
 
+
 sudo_args = " ".join(cmd_runner.SUDO_ARGS)
 
 
@@ -85,7 +87,6 @@
         def wait(self):
             return self.returncode
 
-
     class MockCmdRunnerPopen_sha1sum_fail(object):
         def __call__(self, cmd, *args, **kwargs):
             self.returncode = 0
@@ -99,7 +100,6 @@
         def wait(self):
             return self.returncode
 
-
     class MockCmdRunnerPopen_wait_fails(object):
         def __call__(self, cmd, *args, **kwargs):
             self.returncode = 0
@@ -136,7 +136,7 @@
                                                    signature_filename),
              'sha1sum -c %s' % hash_filename],
             fixture.mock.commands_executed)
-        
+
     def test_verify_files_returns_files(self):
         self.useFixture(MockSomethingFixture(cmd_runner, 'Popen',
                                              self.MockCmdRunnerPopen()))
@@ -194,6 +194,7 @@
         self.assertFalse(result)
         logging.getLogger().setLevel(logging.WARNING)
 
+
 class TestEnsureCommand(TestCaseWithFixtures):
 
     install_pkg_providing_called = False
@@ -286,6 +287,7 @@
                                               device="testdevice",
                                               board="testboard")))
 
+
 class TestPrepMediaPath(TestCaseWithFixtures):
 
     def test_additional_option_checks(self):
@@ -303,3 +305,44 @@
                                device="testdevice",
                                board="testboard"))
         sys.argv.remove("--mmc")
+
+
+class TestHwpackIsFile(TestCaseWithFixtures):
+
+    """Testing '--hwpack' option only allows regular files."""
+
+    def test_hwpack_is_file(self):
+        class HwPackArgs:
+            def __init__(self, hwpack):
+                self.hwpacks = [hwpack]
+                self.directory = None
+
+        try:
+            tmpdir = tempfile.mkdtemp()
+            self.assertRaises(InvalidHwpackFile, additional_option_checks,
+                              HwPackArgs(hwpack=tmpdir))
+        finally:
+            os.rmdir(tmpdir)
+
+    def test_hwpacks_are_files(self):
+
+        """
+        Tests that multiple hwpacks are regular files.
+
+        Tests against a file and a directory, to avoid circumstances in which
+        'additional_option_checks' is tweaked.
+        """
+
+        class HwPacksArgs:
+            def __init__(self, hwpacks):
+                self.hwpacks = hwpacks
+                self.directory = None
+
+        try:
+            tmpdir = tempfile.mkdtemp()
+            _, tmpfile = tempfile.mkstemp()
+            self.assertRaises(InvalidHwpackFile, additional_option_checks,
+                              HwPacksArgs([tmpfile, tmpdir]))
+        finally:
+            os.rmdir(tmpdir)
+            os.remove(tmpfile)

=== modified file 'linaro_image_tools/utils.py'
--- linaro_image_tools/utils.py	2012-04-18 13:26:25 +0000
+++ linaro_image_tools/utils.py	2012-06-06 14:02:18 +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
@@ -43,8 +43,8 @@
         ``os.path.join``.
     :param alternative: The value to return if no module can be imported.
         Defaults to None.
-    :param error_callback: If non-None, a callable that is passed the ImportError
-        when the module cannot be loaded.
+    :param error_callback: If non-None, a callable that is passed the
+        ImportError when the module cannot be loaded.
     """
     module_segments = name.split('.')
     last_error = None
@@ -84,6 +84,7 @@
         return False
     tarinfo.close()
 
+
 def verify_file_integrity(sig_file_list):
     """Verify a list of signature files.
 
@@ -104,8 +105,8 @@
         tmp = tempfile.NamedTemporaryFile()
 
         try:
-             cmd_runner.run(['gpg', '--status-file={0}'.format(tmp.name),
-                             '--verify', sig_file]).wait()
+            cmd_runner.run(['gpg', '--status-file={0}'.format(tmp.name),
+                            '--verify', sig_file]).wait()
         except cmd_runner.SubcommandNonZeroReturnValue:
             gpg_sig_ok = False
             gpg_out = gpg_out + tmp.read()
@@ -134,6 +135,7 @@
 
     return verified_files, gpg_sig_ok, gpg_out
 
+
 def check_file_integrity_and_log_errors(sig_file_list, binary, hwpacks):
     """
     Wrapper around verify_file_integrity that prints error messages to stderr
@@ -147,22 +149,23 @@
         if not gpg_sig_pass:
             logging.error("GPG signature verification failed.")
             return False, []
-    
+
         if not os.path.basename(binary) in verified_files:
             logging.error("OS Binary verification failed")
             return False, []
-        
+
         for hwpack in hwpacks:
             if not os.path.basename(hwpack) in verified_files:
                 logging.error("Hwpack {0} verification failed".format(hwpack))
                 return False, []
-    
+
         for verified_file in verified_files:
             logging.info('Hash verification of file {0} OK.'.format(
                                                                 verified_file))
-    
+
     return True, verified_files
 
+
 def install_package_providing(command):
     """Install a package which provides the given command.
 
@@ -196,6 +199,7 @@
     except cmd_runner.SubcommandNonZeroReturnValue:
         return False
 
+
 def ensure_command(command):
     """Ensure the given command is available.
 
@@ -204,6 +208,7 @@
     if not has_command(command):
         install_package_providing(command)
 
+
 def find_command(name, prefer_dir=None):
     """Finds a linaro-image-tools command.
 
@@ -218,7 +223,7 @@
 
     # default to searching in current directory when running from a bzr
     # checkout
-    dirs = [os.getcwd(),]
+    dirs = [os.getcwd(), ]
     if os.path.isabs(__file__):
         dirs = os.environ["PATH"].split(os.pathsep)
         # empty dir in PATH means current directory
@@ -253,7 +258,8 @@
         try:
             os.makedirs(loc)
         except OSError:
-            pass # Directory exists.
+            # Directory exists.
+            pass
 
         path = os.path.join(loc, args.device)
     else:
@@ -266,6 +272,10 @@
     """We can't find a package which provides the given command."""
 
 
+class InvalidHwpackFile(Exception):
+    """The hwpack parameter is not a regular file."""
+
+
 class IncompatibleOptions(Exception):
     def __init__(self, value):
         self.value = value
@@ -273,15 +283,21 @@
     def __str__(self):
         return repr(self.value)
 
+
 def additional_option_checks(args):
     if args.directory is not None:
     # If args.device is a path to a device (/dev/) then this is an error
         if "--mmc" in sys.argv:
-            raise IncompatibleOptions("--directory option incompatable with "
+            raise IncompatibleOptions("--directory option incompatible with "
                                       "option --mmc")
 
         # If directory is used as well as having a full path (rather than just
         # a file name or relative path) in args.device, this is an error.
         if re.search(r"^/", args.device):
-            raise IncompatibleOptions("--directory option incompatable with "
+            raise IncompatibleOptions("--directory option incompatible with "
                                       "a full path in --image-file")
+
+    for hwpack in args.hwpacks:
+        if not os.path.isfile(hwpack):
+            raise InvalidHwpackFile(
+                "--hwpack argument (%s) is not a regular file" % hwpack)