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

[Milo Casagrande]

Hello,

in the proposed branch I applied kiko patch in order to be able to ask user if they want to install the required packages.

In the branch I modified a couple of things, only in the relevant functions:
- in 'install_package_providing': modified the user messages, suppressed the output from the installation process since the user was seeing apt-get messages (could be interesting to add an additional message that packages have been installed), added an addition exception class ('PackageInstallationRefused'), to handle the case where the user doesn't want to install the package (in this case it will raises an exception, and the process will terminate, making it impossible to continue using l-i-t).
- in 'tests_utils': reworked the test case where package installation was being tested: with the new functionalities in 'install_package_providing' we need the output from the process in order to parse it and check which packages should be installed, the older mock object was redirecting everything to /dev/null.
- fixed small pep8 warnings.

Thanks for the review.

[Milo Casagrande (milo)]

I was a little bit too confident, and looks like the change proposed here, at least for the test, are not working.
Even reverting back using a temp file is not working anymore now, I guess there was something else that made it work once earlier today.

Will look a little bit more into it later today.

review: Needs Fixing

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

525. By Milo Casagrande on 2012-06-07

First step into fixing the mock object.

 * Fixed the mock object in order to return back
   a predefined output string.
 * Fixed the test function accordingly.
 * Fixed PEP8 warnings.

526. By Milo Casagrande on 2012-06-07

Merged code from trunk.

527. By Milo Casagrande on 2012-06-07

Renamed test class: conflicting with another one

528. By Milo Casagrande on 2012-06-07

Fixed test regression.

 * Fixed the test regression due to the expected user input.

[Milo Casagrande (milo)]

Hi,

the latest commits on this branch now fix the problem.

For testing the output part: since with the new functionalities we
need to output of the command, in order to parse it, I needed the mock
object to return some kind of output to the callable function. I
reworked the method in the mock objects in order to take an optional
argument 'output_string' where it is defined the expected output the
function should receive in order to proceed without errors. The
default value in this case in an empty string as that was the defined
return value for the 'communicate()' function.

For the input part the solution was, obviously, dead simple: I needed
a new fixture for stdin.

I also fixed a couple of PEP8 warnings and renamed a class name since
there were two classes with the same name.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Deepti B. Kalakeri (deeptik)]

Changes looks good. But needs small fix. Please see comments in line.

On Wed, Jun 6, 2012 at 3:50 PM, Milo Casagrande
<email address hidden>wrote:

> Milo Casagrande has proposed merging lp:~milo/linaro-image-tools/bug704029
> into lp:linaro-image-tools.
>
> Requested reviews:
> Данило Шеган (danilo)
> Related bugs:
> Bug #704029 in Linaro Image Tools: "l-m-c should not install packages on
> host system without user consent"
> https://bugs.launchpad.net/linaro-image-tools/+bug/704029
>
> For more details, see:
> https://code.launchpad.net/~milo/linaro-image-tools/bug704029/+merge/108910<https://code.launchpad.net/%7Emilo/linaro-image-tools/bug704029/+merge/108910>
>
> Hello,
>
> in the proposed branch I applied kiko patch in order to be able to ask
> user if they want to install the required packages.
>
> In the branch I modified a couple of things, only in the relevant
> functions:
> - in 'install_package_providing': modified the user messages, suppressed
> the output from the installation process since the user was seeing apt-get
> messages (could be interesting to add an additional message that packages
> have been installed), added an addition exception class
> ('PackageInstallationRefused'), to handle the case where the user doesn't
> want to install the package (in this case it will raises an exception, and
> the process will terminate, making it impossible to continue using l-i-t).
> - in 'tests_utils': reworked the test case where package installation was
> being tested: with the new functionalities in 'install_package_providing'
> we need the output from the process in order to parse it and check which
> packages should be installed, the older mock object was redirecting
> everything to /dev/null.
> - fixed small pep8 warnings.
>
> Thanks for the review.
> --
> https://code.launchpad.net/~milo/linaro-image-tools/bug704029/+merge/108910<https://code.launchpad.net/%7Emilo/linaro-image-tools/bug704029/+merge/108910>
> Your team Linaro Infrastructure is subscribed to branch
> lp:linaro-image-tools.
>
> === 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 10:19:24 +0000
> @@ -24,6 +24,7 @@
> import logging
> import tempfile
> import tarfile
> +from StringIO import StringIO
>
> from linaro_image_tools import cmd_runner, utils
> from linaro_image_tools.testing import TestCaseWithFixtures
> @@ -85,7 +86,6 @@
> def wait(self):
> return self.returncode
>
> -
> class MockCmdRunnerPopen_sha1sum_fail(object):
> def __call__(self, cmd, *args, **kwargs):
> self.returncode = 0
> @@ -99,7 +99,6 @@
> def wait(self):
> return self.returncode
>
> -
> class MockCmdRunnerPopen_wait_fails(object):
> def __call__(self, cmd, *args, **kwargs):
> self.returncode = 0
> @@ -136,7 +135,7 @@
> signature_filename),
> 'sha1sum -c %s' % hash_filename],
> fixture.mock.commands_executed)
> -
> +
> def test_verify_files_returns_files(self):
> ...

[Deepti B. Kalakeri (deeptik)]

review: Needs Fixing

[Milo Casagrande (milo)]

Hello Deepti,

thanks for the reivew!

On Sun, Jun 10, 2012 at 5:55 PM, Deepti B. Kalakeri
<email address hidden> wrote:
>
> The message does not indicate which service needs the package.
> We can improve the message to something like :
> "linaro-media-create needs missing package: %s to execute %s command.
> Install (Y/n)?" % (" ".join(to_install), command)

Agreed, will look into a better way to express that.

> or something better on the similar lines.
> Additionally it would good if we gave a user hint on installation
> completion through a message.

Before suppressing the command output, it showed apt-get running
output, we can re-introduce that. It is rather verbose in my opinion,
but that wouldn't add to much logic to handle percentage or completion
information here.

> Also, when I ran the l-m-c command and choose not to install we get the
> following traceback along
> with the message
>
> This does not look so clean. We should do a  graceful exit than showing all
> this trace messages.

I agree with you, I wasn't sure either to throw an exception there.
I'm not sure what is the best practice inside l-i-t, but I would
rather exit the application (via sys.exit(CODE)) then throwing the
exception that is not caught nor handled in a better way up the stack.

> I tried running the tests locally and I get the following traceback
> http://paste.ubuntu.com/1033938/.
> Though the tests changes looks good, I cannot validate if they run
> successful atleast on Natty.

I will try again and look what can be wrong: on my machine it works correctly.
Cheers.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

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

529. By Milo Casagrande on 2012-06-11

Reenabled stdout and stderr on apt-get install.

530. By Milo Casagrande on 2012-06-11

User friendly messages.

531. By Milo Casagrande on 2012-06-11

Removed thrown exception, program exits instead.

532. By Milo Casagrande on 2012-06-11

Merged from trunk.

[Milo Casagrande (milo)]

Hello Deepti,

last pushes on the repository should address all your suggestion.
Unfortunately, I'm not able to reproduce the error you have.

Cheers.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

[Deepti B. Kalakeri (deeptik)]

Overall looks good. Very minimal changes though that are needed.

202 print ("In order to use the '%s' command, the following packages have "
203 "to be installed: %s" % (command, " ".join(to_install)))
204 resp = raw_input("Install them? (Y/n) ")

Instead of displaying "command, the following packages have"
can we rewrite it as "command, the following package/s have" ?
Also, Instead of displaying
"Install them" can we rewrite it as "Install ?" this
would mean we can indicate it for one or many packages

For the message : Package installation is necessary to continue. Exiting.
Please remove the extra space between the . and Exiting.

We should add one more test to cover when the user denies installation of the
package ??

[Deepti B. Kalakeri (deeptik)]

review: Needs Fixing

[Milo Casagrande (milo)]

Hello Deepti,

thanks for the review.
All the suggested changes have been applied, and I also added a test
for the refused package installation.

Cheers.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

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

533. By Milo Casagrande on 2012-06-13

Fixed proposed review changes.

[Deepti B. Kalakeri (deeptik)]

Looks good. +1.

=== modified file 'linaro_image_tools/tests/fixtures.py'
--- linaro_image_tools/tests/fixtures.py	2012-06-07 13:22:47 +0000
+++ linaro_image_tools/tests/fixtures.py	2012-06-13 08:23:21 +0000
@@ -69,8 +69,9 @@
     # used in tests to make sure all callsites wait for their child.
     child_finished = True
 
-    def __init__(self, assert_child_finished=True):
+    def __init__(self, output_string='', assert_child_finished=True):
         self.assert_child_finished = assert_child_finished
+        self.output_string = output_string
 
     def __call__(self, cmd, *args, **kwargs):
         if self.assert_child_finished and not self.child_finished:
@@ -91,7 +92,7 @@
 
     def communicate(self, input=None):
         self.wait()
-        return '', ''
+        return self.output_string, ''
 
     def wait(self):
         self.child_finished = True
@@ -112,9 +113,9 @@
     If no mock is given, a MockCmdRunnerPopen instance is used.
     """
 
-    def __init__(self, assert_child_finished=True):
+    def __init__(self, output_string='', assert_child_finished=True):
         super(MockCmdRunnerPopenFixture, self).__init__(
-            cmd_runner, 'Popen', MockCmdRunnerPopen(assert_child_finished))
+            cmd_runner, 'Popen', MockCmdRunnerPopen(output_string, assert_child_finished))
 
     def tearDown(self):
         super(MockCmdRunnerPopenFixture, self).tearDown()

=== modified file 'linaro_image_tools/tests/test_utils.py'
--- linaro_image_tools/tests/test_utils.py	2012-06-06 13:59:32 +0000
+++ linaro_image_tools/tests/test_utils.py	2012-06-13 08:23:21 +0000
@@ -24,6 +24,7 @@
 import logging
 import tempfile
 import tarfile
+from StringIO import StringIO
 
 from linaro_image_tools import cmd_runner, utils
 from linaro_image_tools.testing import TestCaseWithFixtures
@@ -249,19 +250,45 @@
 
 class TestInstallPackageProviding(TestCaseWithFixtures):
 
-    def test_found_package(self):
-        self.useFixture(MockSomethingFixture(
-            sys, 'stdout', open('/dev/null', 'w')))
-        fixture = self.useFixture(MockCmdRunnerPopenFixture())
+    # This is the package we need to fake the installation of, it is a
+    # slightly changed version of 'apt-get -s install' output.
+    # It is used as an argument to MockCmdRunnerPopenFixture in order to
+    # pass a string that should be expected as output from the command that
+    # is being executed.
+    output_string = 'Inst dosfstools (3.0.12-1ubuntu1 Ubuntu:12.04)'
+
+    def test_package_installation_accepted(self):
+        self.useFixture(MockSomethingFixture(sys,
+                                             'stdout',
+                                             open('/dev/null', 'w')))
+        # We need this since we are getting user input via raw_input
+        # and we need a 'Y' to proceed with the operations.
+        self.useFixture(MockSomethingFixture(sys,
+                                             'stdin',
+                                             StringIO('Y')))
+        fixture = self.useFixture(MockCmdRunnerPopenFixture(self.output_string))
         install_package_providing('mkfs.vfat')
         self.assertEqual(
-            ['%s apt-get --yes install dosfstools' % sudo_args],
-            fixture.mock.commands_executed)
+                         ['apt-get -s install dosfstools',
+                          '%s apt-get --yes install dosfstools' %
+                          sudo_args],
+                         fixture.mock.commands_executed)
+
+    def test_package_installation_refused(self):
+        self.useFixture(MockSomethingFixture(sys,
+                                             'stdout',
+                                             open('/dev/null', 'w')))
+        # We need this since we are getting user input via raw_input
+        # and we need a 'n' to mimic a refused package installation.
+        self.useFixture(MockSomethingFixture(sys,
+                                             'stdin',
+                                             StringIO('n')))
+        self.useFixture(MockCmdRunnerPopenFixture(self.output_string))
+        self.assertRaises(SystemExit, install_package_providing, 'mkfs.vfat')
 
     def test_not_found_package(self):
-        self.assertRaises(
-            UnableToFindPackageProvidingCommand,
-            install_package_providing, 'mkfs.lean')
+        self.assertRaises(UnableToFindPackageProvidingCommand,
+                          install_package_providing, 'mkfs.lean')
 
 
 class Args():
@@ -288,7 +315,7 @@
                                               board="testboard")))
 
 
-class TestPrepMediaPath(TestCaseWithFixtures):
+class TestAdditionalOptionChecks(TestCaseWithFixtures):
 
     def test_additional_option_checks(self):
         self.useFixture(MockSomethingFixture(os.path, 'abspath', lambda x: x))

=== modified file 'linaro_image_tools/utils.py'
--- linaro_image_tools/utils.py	2012-06-06 13:59:32 +0000
+++ linaro_image_tools/utils.py	2012-06-13 08:23:21 +0000
@@ -171,7 +171,10 @@
 
     If we can't find any package which provides it, raise
     UnableToFindPackageProvidingCommand.
+
+    If the user denies installing the package, the program exits.
     """
+
     if CommandNotFound is None:
         raise UnableToFindPackageProvidingCommand(
             "Cannot lookup a package which provides %s" % command)
@@ -182,12 +185,35 @@
             "Unable to find any package providing %s" % command)
 
     # TODO: Ask the user to pick a package when there's more than one that
-    # provide the given command.
+    # provides the given command.
     package, _ = packages[0]
-    print ("Installing required command %s from package %s"
-           % (command, package))
-    cmd_runner.run(
-        ['apt-get', '--yes', 'install', package], as_root=True).wait()
+    output, _ = cmd_runner.run(['apt-get', '-s', 'install', package],
+                               stdout=subprocess.PIPE).communicate()
+    to_install = []
+    for line in output.splitlines():
+        if line.startswith("Inst"):
+            to_install.append(line.split()[1])
+    if not to_install:
+        raise UnableToFindPackageProvidingCommand(
+            "Unable to find any package to be installed.")
+
+    try:
+        print ("In order to use the '%s' command, the following package/s have "
+               "to be installed: %s" % (command, " ".join(to_install)))
+        resp = raw_input("Install? (Y/n) ")
+        if resp.lower() != 'y':
+            print "Package installation is necessary to continue. Exiting."
+            sys.exit(1)
+        print ("Installing required command '%s' from package '%s'..."
+                % (command, package))
+        cmd_runner.run(['apt-get', '--yes', 'install', package],
+                        as_root=True).wait()
+    except EOFError:
+        raise PackageInstallationRefused(
+                            "Package installation interrupted: input error.")
+    except KeyboardInterrupt:
+        raise PackageInstallationRefused(
+                            "Package installation interrupted by the user.")
 
 
 def has_command(command):
@@ -272,6 +298,10 @@
     """We can't find a package which provides the given command."""
 
 
+class PackageInstallationRefused(Exception):
+    """User has chosen not to install a package."""
+
+
 class InvalidHwpackFile(Exception):
     """The hwpack parameter is not a regular file."""