Fixes BSP download flow over to errors.

Instead of the older flow using True/False.
Uses ToolWrapper for the tools.
Updates ToolWrapper to use Popen instead of
subprocess, so that stdout/stderr can be read if
desired.

BUG: https://b/27930418, https://b/27930909
TEST: unit tests pass

Change-Id: Ida705491c6fbd18f987e5281bb278e0b1edef9ca

12 files changed, 247 insertions, 135 deletions

diff --git a/cli/lib/bdk.py b/cli/lib/bdk.py
index 3596159..747fa10 100644
--- a/cli/lib/bdk.py
+++ b/cli/lib/bdk.py
@@ -23,6 +23,7 @@ import sys
 
 # pylint: disable=relative-import
 from cli import climanager
+import error
 from metrics import metrics_util
 from metrics.data_types import exception
 
@@ -70,6 +71,8 @@ def main():
       raise
     else:
       print(e, file=sys.stderr)
+      if isinstance(e, error.Error):
+        return e.errno
       return 1
 
 
diff --git a/cli/lib/bsp/device.py b/cli/lib/bsp/device.py
index b2ec839..19c5732 100644
--- a/cli/lib/bsp/device.py
+++ b/cli/lib/bsp/device.py
@@ -260,9 +260,13 @@ class Device(object):
 
     Args:
       tarball: a path to a tarball file.
+
     Returns:
       The name of the matching package, if any.
       None otherwise.
+
+    Raises:
+      package.VersionError, if there is a problem reading the version.
     """
     tar_hash = package.TarPackage.get_tarball_version(tarball)
     for pkg in self._package_map:
diff --git a/cli/lib/bsp/package.py b/cli/lib/bsp/package.py
index 6f25bc4..427675e 100644
--- a/cli/lib/bsp/package.py
+++ b/cli/lib/bsp/package.py
@@ -24,7 +24,7 @@ import shutil
 
 from bsp import status
 from bsp import subpackage
-from core import popen
+from core import tool
 from core import user_config
 from core import util
 import error
@@ -52,6 +52,21 @@ class DownloadError(Error):
   description = 'Failed to download package'
 
 
+class RemoteError(DownloadError):
+  """Raised when there is a problem checking the remote version of a Package."""
+  description = 'Trouble getting remote version'
+
+
+class VersionError(DownloadError):
+  """Raised when the package source is not/doesn't have the correct version."""
+  description = 'Failed to get correct version'
+
+
+class UnzipError(DownloadError):
+  """Raised when the downloaded tarball fails to unzip correctly."""
+  description = 'Failed to unzip tarball'
+
+
 class MissingDirectoryError(Error):
   """Raised when an expected directory is missing."""
   description = 'Directory is missing'
@@ -176,11 +191,12 @@ class Package(object):
         download and skips straight to unzipping.
 
     Raises:
-      DownloadError: if the package fails to download.
+      DownloadError: (or a child thereof) if the package fails to complete
+        all steps of the download process (downloading, verifying version,
+        etc.).
       PathBlockedError: if there is an unknown file/dir at the
         target download location.
-      Probably some others (ValueError, OSError, etc.)
-      TODO(b/27930909): Clean up this flow; actually identify possible errors.
+      ValueError: if a tarball is passed to a git package.
     """
     if self.is_downloaded():
       return
@@ -192,11 +208,7 @@ class Package(object):
     success = False
     try:
       os.makedirs(self.directory)
-      result = self._type_specific_download(tarball)
-      if not result:
-        # TODO(b/27930909): Clean up _type_specific_download implementations
-        #   to throw more specific errors.
-        raise DownloadError()
+      self._type_specific_download(tarball)
       success = True
     finally:
       if not success:
@@ -219,11 +231,12 @@ class Package(object):
     Args:
       tarball: (optional) if provided, replaces the download part of tar
         download and skips straight to unzipping.
+
+    Raises:
+      ValueError: If a tarball is passed to a git package.
+      DownloadError: (or a subclass thereof) if there is some complication
+        downloading.
     """
-    # TODO(b/27930909): catch popen OSErrors, etc.
-    #   Ideally this function would throw something like:
-    #   * ValidationError if version is wrong
-    #   * SubprocessError if a subprocess goes wrong
     raise NotImplementedError(
         'Download function should be overridden in child class.')
 
@@ -358,16 +371,14 @@ class GitPackage(Package):
     # Setup. Break down version into branch + hash
     # Use rsplit for the off chance ':' is part of the branch name.
     (branch, branch_hash) = self.version.rsplit(':', 1)
+    git = tool.PathToolWrapper('git')
 
     # Check if branch points to hash
     # If so, we can do a shallow clone
-    lsremote_process = popen.PopenPiped(['git', 'ls-remote', self.remote,
-                                         branch])
-    (out, err) = lsremote_process.communicate()
-    if lsremote_process.returncode:
-      print 'Trouble checking remote {} for package {}: {}'.format(
-          self.remote, self.name, err)
-      return False
+    try:
+      out, _ = git.run(['ls-remote', self.remote, branch], piped=True)
+    except tool.Error as e:
+      raise RemoteError(e)
     if out.startswith(branch_hash):
       shallow = True
       depth = '--depth=1'
@@ -376,44 +387,37 @@ class GitPackage(Package):
       depth = '--single-branch'
 
     # Clone the repo.
-    clone_process = popen.PopenPiped(['git', 'clone', depth,
-                                      '--branch=' + branch, self.remote,
-                                      self.directory])
-    (out, err) = clone_process.communicate()
-
-    if clone_process.returncode:
-      print 'Trouble downloading package {} from {}: {}'.format(
-          self.name, self.remote, err)
-      return False
+    try:
+      git.run(['clone', depth, '--branch=' + branch,
+               self.remote, self.directory])
+    except tool.Error as e:
+      raise DownloadError('from {}: {}'.format(self.remote, e))
 
     # Checkout the right revision if necessary
     if not shallow:
-      checkout_process = popen.PopenPiped(['git', 'reset', '--hard',
-                                           branch_hash], cwd=self.directory)
-      (out, err) = checkout_process.communicate()
-      if checkout_process.returncode:
-        print ('Trouble checking out correct version '
-               'of package {} from {}: {}'.format(
-                   self.name, self.remote, err))
-        return False
+      try:
+        git.set_cwd(self.directory)
+        git.run(['reset', '--hard', branch_hash])
+        git.set_cwd(None)
+      except tool.Error as e:
+        raise VersionError(e)
 
     # Check that the correct revision was downloaded.
     git_dir = os.path.join(self.directory, '.git')
-    hash_check_process = popen.PopenPiped(['git', '--git-dir=' + git_dir,
-                                           'rev-parse', branch])
-    (out, err) = hash_check_process.communicate()
-    if hash_check_process.returncode:
-      print ('Trouble checking version of '
-             'package {} from {}: {}'.format(self.name, self.remote, err))
-      return False
-    elif not out.startswith(branch_hash):
-      print ('Remote package {} from {} '
-             'is the incorrect version.'.format(self.name, self.remote))
-      return False
-
-    # Done. Clean up and return
+    try:
+      out, _ = git.run(['--git-dir=' + git_dir, 'rev-parse', branch],
+                       piped=True)
+    except tool.Error as e:
+      raise VersionError(e)
+
+    if not out.startswith(branch_hash):
+      raise VersionError('Remote package from {} '
+                         'is the incorrect version '
+                         '{} (expected {}).'.format(
+                             self.remote, out, branch_hash))
+
+    # Everything went well. Clean up the download; we don't need git history.
     shutil.rmtree(git_dir)
-    return True
 
 
 class TarPackage(Package):
@@ -421,8 +425,11 @@ class TarPackage(Package):
   @classmethod
   def get_tarball_version(cls, tarball_file):
     """Gets the tarball version (sha256) of a tarball file."""
-    with open(tarball_file) as tar:
-      tar_hash = hashlib.sha256(tar.read()).hexdigest()
+    try:
+      with open(tarball_file) as tar:
+        tar_hash = hashlib.sha256(tar.read()).hexdigest()
+    except IOError as e:
+      raise VersionError(e)
     return tar_hash
 
   def _type_specific_download(self, tarball_file=None):
@@ -431,38 +438,35 @@ class TarPackage(Package):
     # Store the source for error messages.
     tar_source = tarball_file if tarball_provided else self.remote
 
+    tool_runner = tool.PathToolRunner()
+
     # Get the tarball if it wasn't provided
     if not tarball_provided:
       # Setup.
       tarball_file = os.path.join(self.directory, DEFAULT_TAR_FILE)
 
       # Download the tarball.
-      fetch_process = popen.PopenPiped(['curl', '-o', tarball_file,
-                                        self.remote])
-      (_, err) = fetch_process.communicate()
-      if fetch_process.returncode:
-        print 'Trouble downloading package from {}: {}'.format(self.remote, err)
-        return False
-
-    # Check version of the tarball by using a checksum
-    tar_hash = self.get_tarball_version(tarball_file)
-    if not tar_hash.startswith(self.version):
-      print ('Tarball for package {} from {} does not match '
-             'expected checksum.'.format(self.name, tar_source))
-      return False
-
-    # Unzip the tarball
-    unzip_process = popen.PopenPiped(['tar', '-C', self.directory, '-xzf',
-                                      tarball_file])
-    (_, err) = unzip_process.communicate()
-    if unzip_process.returncode:
-      print 'Trouble unzipping package {} from {}.'.format(
-          self.name, tar_source)
-      return False
-
-    # Clean up and return.
-    # TODO(b/27930909): have this as a finally once Errors are being thrown
-    #   instead of early returns (conditional on being downloaded, of course).
-    if not tarball_provided:
-      os.remove(tarball_file)
-    return True
+      try:
+        tool_runner.run('curl', ['-o', tarball_file, self.remote])
+      except tool.Error as e:
+        raise DownloadError(e)
+
+    try:
+      # Check version of the tarball by using a checksum
+      tar_hash = self.get_tarball_version(tarball_file)
+      if not tar_hash.startswith(self.version):
+        raise VersionError('Tarball from {} is the incorrect version {} '
+                           '(expected {})'.format(
+                               tar_source, tar_hash, self.version))
+
+      # Unzip the tarball
+      try:
+        tool_runner.run('tar', ['-C', self.directory, '-xzf', tarball_file])
+      except tool.Error as e:
+        raise UnzipError(e)
+
+    finally:
+      # Whether extraction succeeds or fails,
+      # we should remove the downloaded tarball.
+      if not tarball_provided:
+        os.remove(tarball_file)
diff --git a/cli/lib/bsp/package_unittest.py b/cli/lib/bsp/package_unittest.py
index 998e544..939dadb 100644
--- a/cli/lib/bsp/package_unittest.py
+++ b/cli/lib/bsp/package_unittest.py
@@ -44,7 +44,7 @@ class PackageTest(unittest.TestCase):
     package.open = self.stub_open.open
     package.shutil = self.stub_shutil
     package.hashlib = self.stub_hashlib
-    package.popen.subprocess = self.stub_subprocess
+    package.tool.subprocess = self.stub_subprocess
     package.user_config = self.stub_user_config
     package.util = self.stub_util
 
@@ -241,6 +241,11 @@ class PackageTest(unittest.TestCase):
     self.assertEqual(package.TarPackage.get_tarball_version('file1'),
                      '<hash_of_file1>')
 
+  def test_tarball_invalid_version(self):
+    self.stub_os.path.should_exist = []
+    with self.assertRaises(package.VersionError):
+      package.TarPackage.get_tarball_version('file1')
+
   def test_already_downloaded(self):
     self.stub_os.path.should_be_dir = [self.pkg.directory]
     # Shouldn't do any downloading - no dirs should be made.
@@ -261,81 +266,89 @@ class PackageTest(unittest.TestCase):
     # Note: we don't test that tmp_dir/.git exists, because that's
     # hard to write cleanly; so we assume it is created and exists
     # as expected by clone/rev-parse.
-    self.stub_subprocess.AddCommand(['git', 'ls-remote'], ret_out='hash')
-    self.stub_subprocess.AddFileCommand(['git', 'clone'], pos_out=[-1])
-    self.stub_subprocess.AddCommand(['git', 'rev-parse'], ret_out='hash')
+    ls_remote = self.stub_subprocess.AddCommand(ret_out='hash')
+    clone = self.stub_subprocess.AddFileCommand(pos_out=[-1])
+    rev_parse = self.stub_subprocess.AddCommand(ret_out='hash')
 
     self.git_package.download()
     self.assertTrue(self.stub_os.path.exists(self.git_package.directory))
+    ls_remote.AssertCallContained(['git', 'ls-remote'])
+    clone.AssertCallContained(['git', 'clone'])
+    rev_parse.AssertCallContained(['git', 'rev-parse'])
 
   def test_git_download_full(self):
     self.stub_os.should_makedirs = [self.git_package.directory]
     # Note: we don't test that tmp_dir/.git exists, because that's
     # hard to write cleanly; so we assume it is created and exists
     # as expected by clone/rev-parse.
-    self.stub_subprocess.AddCommand(['git', 'ls-remote'], ret_out='wronghash')
-    self.stub_subprocess.AddFileCommand(['git', 'clone'], pos_out=[-1])
-    self.stub_subprocess.AddCommand(['git', 'reset'])
-    self.stub_subprocess.AddCommand(['git', 'rev-parse'], ret_out='hash')
+    ls_remote = self.stub_subprocess.AddCommand(ret_out='wronghash')
+    clone = self.stub_subprocess.AddFileCommand(pos_out=[-1])
+    reset = self.stub_subprocess.AddCommand()
+    rev_parse = self.stub_subprocess.AddCommand(ret_out='hash')
 
     self.git_package.download()
     self.assertTrue(self.stub_os.path.exists(self.git_package.directory))
+    ls_remote.AssertCallContained(['git', 'ls-remote'])
+    clone.AssertCallContained(['git', 'clone'])
+    reset.AssertCallContained(['git', 'reset'])
+    rev_parse.AssertCallContained(['git', 'rev-parse'])
 
   def test_git_failed_lsremote(self):
     self.stub_os.should_makedirs = [self.git_package.directory]
-    self.stub_subprocess.AddCommand(['git', 'ls-remote'], ret_code=-1)
+    ls_remote = self.stub_subprocess.AddCommand(ret_code=-1)
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaises(package.RemoteError):
         self.git_package.download()
-      self.assertIn('Trouble checking remote', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.git_package.directory))
+    ls_remote.AssertCallContained(['git', 'ls-remote'])
 
   def test_git_failed_download(self):
     self.stub_os.should_makedirs = [self.git_package.directory]
-    self.stub_subprocess.AddCommand(['git', 'ls-remote'], ret_out='hash')
-    self.stub_subprocess.AddFileCommand(['git', 'clone'], pos_out=[-1],
+    ls_remote = self.stub_subprocess.AddCommand(ret_out='hash')
+    clone = self.stub_subprocess.AddFileCommand(pos_out=[-1],
                                         ret_code=-1)
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaises(package.DownloadError):
         self.git_package.download()
-      self.assertIn('Trouble downloading', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.git_package.directory))
+    ls_remote.AssertCallContained(['git', 'ls-remote'])
+    clone.AssertCallContained(['git', 'clone'])
 
   def test_git_failed_reset(self):
     self.stub_os.should_makedirs = [self.git_package.directory]
     # Note: we don't test that tmp_dir/.git exists, because that's
     # hard to write cleanly; so we assume it is created and exists
     # as expected by clone/rev-parse.
-    self.stub_subprocess.AddCommand(['git', 'ls-remote'], ret_out='wronghash')
-    self.stub_subprocess.AddFileCommand(['git', 'clone'], pos_out=[-1])
-    self.stub_subprocess.AddCommand(['git', 'reset'], ret_code=-1)
+    ls_remote = self.stub_subprocess.AddCommand(ret_out='wronghash')
+    clone = self.stub_subprocess.AddFileCommand(pos_out=[-1])
+    reset = self.stub_subprocess.AddCommand(ret_code=-1)
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaises(package.VersionError):
         self.git_package.download()
-      self.assertIn('Trouble checking out correct version', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.git_package.directory))
+    ls_remote.AssertCallContained(['git', 'ls-remote'])
+    clone.AssertCallContained(['git', 'clone'])
+    reset.AssertCallContained(['git', 'reset'])
 
   def test_git_wrong_version(self):
     self.stub_os.should_makedirs = [self.git_package.directory]
     # Note: we don't test that tmp_dir/.git exists, because that's
     # hard to write cleanly; so we assume it is created and exists
     # as expected by clone/rev-parse.
-    self.stub_subprocess.AddCommand(['git', 'ls-remote'], ret_out='hash')
-    self.stub_subprocess.AddFileCommand(['git', 'clone'], pos_out=[-1])
-    self.stub_subprocess.AddCommand(['git', 'rev-parse'], ret_out='wronghash')
+    ls_remote = self.stub_subprocess.AddCommand(ret_out='hash')
+    clone = self.stub_subprocess.AddFileCommand(pos_out=[-1])
+    rev_parse = self.stub_subprocess.AddCommand(ret_out='wronghash')
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaises(package.VersionError):
         self.git_package.download()
-      self.assertIn('incorrect version', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.git_package.directory))
+    ls_remote.AssertCallContained(['git', 'ls-remote'])
+    clone.AssertCallContained(['git', 'clone'])
+    rev_parse.AssertCallContained(['git', 'rev-parse'])
 
   def test_git_tarball(self):
     self.stub_os.should_makedirs = [self.git_package.directory]
@@ -347,64 +360,66 @@ class PackageTest(unittest.TestCase):
 
   def test_tar_download(self):
     self.stub_os.should_makedirs = [self.tar_package.directory]
-    self.stub_subprocess.AddFileCommand(['curl'], out_args=['-o'])
+    curl = self.stub_subprocess.AddFileCommand(out_args=['-o'])
     # Again, side effects of popen are hard to encode, so this doesn't
     # reflect that the '-C' output directory will get populated by tar.
-    self.stub_subprocess.AddFileCommand(['tar'], in_args=['-C', '-xzf'])
+    tar = self.stub_subprocess.AddFileCommand(in_args=['-C', '-xzf'])
     self.stub_hashlib.should_return = self.tar_package.version
 
     self.tar_package.download()
     self.assertTrue(self.stub_os.path.exists(self.tar_package.directory))
+    curl.AssertCallContained(['curl', '-o'])
+    tar.AssertCallContained(['tar', '-C', '-xzf'])
 
   def test_tar_failed_download(self):
     self.stub_os.should_makedirs = [self.tar_package.directory]
-    self.stub_subprocess.AddFileCommand(['curl'], out_args=['-o'], ret_code=-1)
+    curl = self.stub_subprocess.AddFileCommand(out_args=['-o'], ret_code=-1)
     self.stub_hashlib.should_return = self.tar_package.version
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaises(package.DownloadError):
         self.tar_package.download()
-      self.assertIn('Trouble downloading', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.tar_package.directory))
+    curl.AssertCallContained(['curl', '-o'])
 
   def test_tar_wrong_version(self):
     self.stub_os.should_makedirs = [self.tar_package.directory]
-    self.stub_subprocess.AddFileCommand(['curl'], out_args=['-o'])
-    self.stub_hashlib.should_return = 'wrong_version'
+    curl = self.stub_subprocess.AddFileCommand(out_args=['-o'])
+    self.stub_hashlib.should_return = '0xWRONG'
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaisesRegexp(package.VersionError,
+                                 'incorrect version 0xWRONG'):
         self.tar_package.download()
-      self.assertIn('does not match expected checksum', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.tar_package.directory))
+    curl.AssertCallContained(['curl', '-o'])
 
   def test_tar_bad_unzip(self):
     self.stub_os.should_makedirs = [self.tar_package.directory]
-    self.stub_subprocess.AddFileCommand(['curl'], out_args=['-o'])
-    self.stub_subprocess.AddFileCommand(['tar'], in_args=['-C', '-xzf'],
+    curl = self.stub_subprocess.AddFileCommand(out_args=['-o'])
+    tar = self.stub_subprocess.AddFileCommand(in_args=['-C', '-xzf'],
                                         ret_code=-1)
     self.stub_hashlib.should_return = self.tar_package.version
 
-    with test_util.OutputCapture() as output:
-      with self.assertRaises(package.DownloadError):
+    with self.assertRaises(package.UnzipError):
         self.tar_package.download()
-      self.assertIn('Trouble unzipping', output.stdout)
     # Leave in a clean state
     self.assertFalse(self.stub_os.path.exists(self.tar_package.directory))
+    curl.AssertCallContained(['curl', '-o'])
+    tar.AssertCallContained(['tar', '-C', '-xzf'])
 
   def test_tar_tarball(self):
     self.stub_os.path.should_exist = ['tar_file']
     self.stub_open.files['tar_file'] = stubs.StubFile()
     self.stub_os.should_makedirs = [self.tar_package.directory]
-    self.stub_subprocess.AddFileCommand(['tar'], in_args=['-C', '-xzf'])
+    tar = self.stub_subprocess.AddFileCommand(in_args=['-C', '-xzf'])
     self.stub_hashlib.should_return = self.tar_package.version
 
     self.tar_package.download('tar_file')
     self.assertTrue(self.stub_os.path.exists(self.tar_package.directory))
     # Make sure the tar file is still where it was.
     self.assertTrue(self.stub_os.path.exists('tar_file'))
+    tar.AssertCallContained(['tar', '-C', '-xzf'])
 
   def test_uninstall(self):
     dirs = [self.pkg.directory,
diff --git a/cli/lib/commands/bsp/download.py b/cli/lib/commands/bsp/download.py
index 7c5524f..75be397 100644
--- a/cli/lib/commands/bsp/download.py
+++ b/cli/lib/commands/bsp/download.py
@@ -102,6 +102,10 @@ class Update(clicommand.Command):
 
         Returns:
             A dictionary mapping { package_name : tarball }.
+
+        Raises:
+            bsp.package.VersionError if there is a problem getting any version
+            (such as a file not being found).
         """
         mapping = {}
         for tarball in tarballs:
diff --git a/cli/lib/commands/bsp/refresh.py b/cli/lib/commands/bsp/refresh.py
index 137bd66..9c39546 100644
--- a/cli/lib/commands/bsp/refresh.py
+++ b/cli/lib/commands/bsp/refresh.py
@@ -85,6 +85,10 @@ class Refresh(clicommand.Command):
 
         Returns:
             A dictionary mapping { package_name : tarball }.
+
+        Raises:
+            bsp.package.VersionError if there is a problem getting any version
+            (such as a file not being found).
         """
 
         mapping = {}
diff --git a/cli/lib/core/provision_unittest.py b/cli/lib/core/provision_unittest.py
index 7d7ecc9..fd91115 100644
--- a/cli/lib/core/provision_unittest.py
+++ b/cli/lib/core/provision_unittest.py
@@ -100,6 +100,7 @@ class ProvisionDeviceTest(unittest.TestCase):
     #   * ANDROID_PRODUCT_OUT point to our temporary product dir.
     command.AssertCallWas(
         [self._PROVISION_DEVICE_PATH], shell=False, cwd=None,
+        stdout=None, stderr=None,
         env={'PATH': self.stub_os.path.dirname(self._FASTBOOT_PATH),
              'ANDROID_BUILD_TOP': self.stub_util.GetOSPath(
                  self.target.os_version),
diff --git a/cli/lib/core/tool.py b/cli/lib/core/tool.py
index 939827e..699dbef 100644
--- a/cli/lib/core/tool.py
+++ b/cli/lib/core/tool.py
@@ -132,15 +132,20 @@ class ToolWrapper(object):
   def exists(self):
     return os.path.isfile(self._tool_path)
 
-  def run(self, arg_array=None):
+  def run(self, arg_array=None, piped=False):
     """Executes the tool and blocks until completion.
 
     Args:
       arg_array: list of string arguments to pass to the tool.
+      piped: If true, send stdout and stderr to pipes.
 
     Raises:
       ExecuteError: if execution fails.
       ReturnError: if execution returns a non-0 exit code.
+
+    Returns:
+      (out, err): The output to stdout and stderr of the called tool.
+        Will both be None if piped=False.
     """
     # Remove -- passthrough indicator from arg_array.
     if arg_array and arg_array[0] == '--':
@@ -150,21 +155,33 @@ class ToolWrapper(object):
     if self._cwd:
       self.environment['PWD'] = os.path.abspath(self._cwd)
 
+    stdout = None
+    stderr = None
+    if piped:
+      stdout = subprocess.PIPE
+      stderr = subprocess.PIPE
+
     try:
-      ret = subprocess.call([self._tool_path] + (arg_array or []),
-                            env=self.environment, shell=False, cwd=self._cwd)
+      tool_process = subprocess.Popen([self._tool_path] + (arg_array or []),
+                                      env=self.environment, shell=False,
+                                      cwd=self._cwd, stdout=stdout,
+                                      stderr=stderr)
+      (out, err) = tool_process.communicate()
     except OSError as e:
       # Catch and re-raise so we can include the tool path in the message.
       raise ExecuteError('"{}": {} [{}]'.format(
           self._tool_path, e.errno, e.strerror))
 
     # Exiting via signal gives negative return codes.
+    ret = tool_process.returncode
     if ret < 0:
       # Return the normal shell exit mask for being signaled.
       ret = 128 - ret
     if ret != 0:
-      raise ReturnError('"{}": {}'.format(self._tool_path, ret), errno=ret)
+      raise ReturnError('"{}": {} ({})'.format(self._tool_path, ret, err),
+                        errno=ret)
 
+    return (out, err)
 
 class HostToolWrapper(ToolWrapper):
   """Wraps a tool from out/host/<arch>/bin/."""
@@ -194,7 +211,23 @@ class HostToolRunner(object):
 
   def run(self, path, args):
     host_tool = HostToolWrapper(path, self._build_out)
-    host_tool.run(args)
+    return host_tool.run(args)
+
+
+class PathToolWrapper(ToolWrapper):
+  """Wraps a tool expected to be in the user PATH."""
+
+  def __init__(self, program, env=None):
+    super(PathToolWrapper, self).__init__(program, env)
+    self.add_caller_env_path()
+
+
+class PathToolRunner(object):
+  """Serves as a PathToolWrapper factory."""
+
+  def run(self, path, args):
+    path_tool = PathToolWrapper(path)
+    return path_tool.run(args)
 
 
 class ProvisionDeviceTool(ToolWrapper):
diff --git a/cli/lib/core/tool_unittest.py b/cli/lib/core/tool_unittest.py
index 2923cf6..2400cf5 100644
--- a/cli/lib/core/tool_unittest.py
+++ b/cli/lib/core/tool_unittest.py
@@ -45,6 +45,17 @@ class ToolWrapperTest(unittest.TestCase):
     t.run()
     command.AssertCallContained(['/my/tool'])
 
+  def test_run_piped(self):
+    """Tests a basic piped tool run."""
+    t = tool.ToolWrapper('/my/tool')
+    command = self.stub_subprocess.AddCommand(['/my/tool'], ret_out='out',
+                                              ret_err='err')
+    command2 = self.stub_subprocess.AddCommand(['/my/tool'], ret_out='out',
+                                               ret_err='err')
+    self.assertEqual(t.run(), (None, None))
+    self.assertEqual(t.run(piped=True), ('out', 'err'))
+
+
   def test_cwd(self):
     """Tests changing the tool working directory."""
     t = tool.ToolWrapper('/my/tool')
@@ -142,6 +153,13 @@ class ToolWrapperTest(unittest.TestCase):
 
     t.run()
 
+  def test_path_tool_wrapper(self):
+    self.stub_os.environ = {'foo': 'bar', 'baz': 'bip', 'PATH': '/usr/bin'}
+    t = tool.PathToolWrapper('tool')
+    command = self.stub_subprocess.AddCommand(['tool'])
+    t.run()
+    command.AssertCallContained(['tool'], env={'PATH': '/usr/bin'})
+
   def test_provision_device_tool(self):
     """Tests ProvisionDeviceTool construction."""
     self.stub_util.arch = 'test_arch'
@@ -192,6 +210,8 @@ class TestBrunchToolRun(BrunchToolWrapperTest):
     command = self.stub_subprocess.AddCommand(ret_code=0)
     t.run(['arg1'])
     command.AssertCallWas(['/my/tool', 'arg1'], shell=False, cwd=None,
+                          stdout=None,
+                          stderr=None,
                           env={'PATH': 'abc:123', 'BAZ': 'B "A" R'})
 
     command = self.stub_subprocess.AddCommand(ret_code=1)
@@ -199,6 +219,8 @@ class TestBrunchToolRun(BrunchToolWrapperTest):
       t.run(['arg1'])
     self.assertEqual(1, e.exception.errno)
     command.AssertCallWas(['/my/tool', 'arg1'], shell=False, cwd=None,
+                          stdout=None,
+                          stderr=None,
                           env={'PATH': 'abc:123', 'BAZ': 'B "A" R'})
 
 
@@ -230,6 +252,8 @@ class TestBrunchToolEnvironment(BrunchToolWrapperTest):
 
     t.run(['arg1'])
     command.AssertCallWas(['/my/tool', 'arg1'], shell=False, cwd=None,
+                          stdout=None,
+                          stderr=None,
                           env={'PATH': 'abc:123', 'BAZ': 'B "A" R'})
 
   def test_no_path(self):
diff --git a/cli/lib/error.py b/cli/lib/error.py
index 644b266..85878a2 100644
--- a/cli/lib/error.py
+++ b/cli/lib/error.py
@@ -27,6 +27,9 @@ class Error(Exception):
   description = 'BDK Error'
 
   def __init__(self, message='', errno=GENERIC_ERRNO):
+    if errno == 0:
+      raise ValueError('Error numbers can not be 0.')
+
     full_message = self.description
     if message is not None:
       # Just in case someone passes a non-string.
diff --git a/cli/lib/test/stubs.py b/cli/lib/test/stubs.py
index df79b04..6d11a35 100644
--- a/cli/lib/test/stubs.py
+++ b/cli/lib/test/stubs.py
@@ -607,7 +607,13 @@ class StubSubprocess(object):
 
             self._ran = True
             self.returncode = self._ret_code
-            return (self._ret_out, self._ret_err)
+            out = None
+            err = None
+            if self._kwargs.get('stdout') == self.parent.PIPE:
+                out = self._ret_out
+            if self._kwargs.get('stderr') == self.parent.PIPE:
+                err = self._ret_err
+            return (out, err)
 
         def wait(self):
             """Stubs Popen.wait(). Returns returncode."""
diff --git a/cli/lib/test/stubs_unittest.py b/cli/lib/test/stubs_unittest.py
index 9e6fb8f..c999b63 100644
--- a/cli/lib/test/stubs_unittest.py
+++ b/cli/lib/test/stubs_unittest.py
@@ -67,9 +67,20 @@ class StubSubprocessTest(unittest.TestCase):
         self.stub_subprocess.AddCommand(ret_out='foo', ret_err='bar',
                                         ret_code=5)
         popen = self.stub_subprocess.Popen(['echo', 'foo'])
-        self.assertEqual(('foo', 'bar'), popen.communicate())
+        # Not piped, shouldn't return the string values.
+        self.assertEqual((None, None), popen.communicate())
         self.assertEqual(5, popen.returncode)
 
+        self.stub_subprocess.AddCommand(ret_out='baz', ret_err='bip',
+                                        ret_code=6)
+        popen = self.stub_subprocess.Popen(
+            ['a command'], stdout=self.stub_subprocess.PIPE,
+            stderr=self.stub_subprocess.PIPE)
+        # Piped, should return the string values.
+        self.assertEqual(('baz', 'bip'), popen.communicate())
+        self.assertEqual(6, popen.returncode)
+
+
     def test_specified_command(self):
         """Tests specifying the command args at creation."""
         self.stub_subprocess.AddCommand(['echo', 'foo'])