Migrate device utils WriteFile to AdbWrapper

build/android/pylib/device/device_utils.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Provides a variety of device interactions based on adb.
 
 Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=W0613
 
 import logging
 import multiprocessing
 import os
 import re
 import sys
 import tempfile
 import time
 import zipfile
 
 import pylib.android_commands
 from pylib import cmd_helper
 from pylib.device import adb_wrapper
 from pylib.device import decorators
 from pylib.device import device_errors
 from pylib.device.commands import install_commands
 from pylib.utils import apk_helper
+from pylib.utils import device_temp_file
 from pylib.utils import host_utils
 from pylib.utils import parallelizer
 from pylib.utils import timeout_retry
 
 _DEFAULT_TIMEOUT = 30
 _DEFAULT_RETRIES = 3
 
 
 @decorators.WithExplicitTimeoutAndRetries(
     _DEFAULT_TIMEOUT, _DEFAULT_RETRIES)
@@ skipping 822 matching lines @@
     # the implementation switch, and if file not found should raise exception.
     if as_root:
       if not self.old_interface.CanAccessProtectedFileContents():
         raise device_errors.CommandFailedError(
           'Cannot read from %s with root privileges.' % device_path)
       return self.old_interface.GetProtectedFileContents(device_path)
     else:
       return self.old_interface.GetFileContents(device_path)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
-  def WriteFile(self, device_path, contents, as_root=False, timeout=None,
-                retries=None):
+  def WriteFile(self, device_path, contents, as_root=False, force_push=False,
+                timeout=None, retries=None):
     """Writes |contents| to a file on the device.
 
     Args:
       device_path: A string containing the absolute path to the file to write
-                   on the device.
+          on the device.
       contents: A string containing the data to write to the device.
       as_root: A boolean indicating whether the write should be executed with
-               root privileges.
+          root privileges (if available).
+      force_push: A boolean indicating whether to force the operation to be

[Sami, 2014/11/12 14:14:14]

Is force_push needed by anything other than tests? If not, we should either
rename it to force_push_for_testing for write the tests so that they trigger the
appropriate code path automatically. (tip: you can easily create long strings in
Python with multiplication, e.g., 'foo' * 64).

[perezju, 2014/11/12 17:37:03]

Well, yes, certainly it's useful for testing, but I thought that the option also
makes sense, e.g., if one is trying to push binary data into the device and one
is worried about the data being mangled by the shell (I think it "should" work
anyway in most cases, I tested it at least with unicode chars and they made it
through when passed by echo, but can't be sure about more complicated cases).

Thanks for the tip anyway, I'll add some test cases using long contents rather
than forcing the push with an option.

+          performed by pushing a file to the device. The default is, when the
+          contents are short, to pass the contents using a shell script instead.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       CommandFailedError if the file could not be written on the device.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    if as_root:
-      if not self.old_interface.CanAccessProtectedFileContents():
-        raise device_errors.CommandFailedError(
-            'Cannot write to %s with root privileges.' % device_path)
-      self.old_interface.SetProtectedFileContents(device_path, contents)
+    if len(contents) < 512 and not force_push:
+      # When as_root=True and su is needed, trying to execute
+      #     su -c echo -n {contents} > {device_path}
+      # won't work, because echo is interpreted by the shell and not an actual

[Sami, 2014/11/12 14:14:14]

nit: I presume it's not the echo that gets executed but rather the output
redirection (">").

[perezju, 2014/11/12 17:37:02]

No. "su -c cmd > /some/file" works fine, because the whole thing will be passed
to adb shell, and the redirection is interpreted correctly. The problem is that
"su" will attempt to execute "cmd" without starting a (new) shell, since in most
cases it's not needed, and because echo does not exist as a command, it fails
with the error:

su: exec failed for echo Error:No such file or directory

+      # command on the device. So instead we must make sure that in those cases
+      # the command to execute becomes:
+      #     su -c sh -c 'echo -n {contents}' > {device_path}
+      cmd = 'echo -n %s' % cmd_helper.SingleQuote(contents)
+      sh_cmd = 'sh -c %s > %s' % (cmd_helper.SingleQuote(cmd),
+                                  cmd_helper.SingleQuote(device_path))
+      self.RunShellCommand(sh_cmd, as_root=as_root, check_return=True)

[perezju, 2014/11/12 01:31:24]

Not sure why this never hit us before! Probably WriteTextFile was only used on
rooted devices, and the problem wouldn't show up.

[jbudorick, 2014/11/12 01:42:37]

Wow, nice catch. This is a nasty little corner case. I wonder if there are other
cases where we'd try to use echo on the device and, if so, if we should provide
a utility function -- probably in here, but possibly in AdbWrapper -- to handle
it.

[perezju, 2014/11/12 17:37:03]

Indeed, echo (and sometimes with as_root=True!) is used in a couple other places
https://code.google.com/p/chromium/codesearch#search/&q=adb%20echo%20file:%5C...

Most of the times it's used to write something to a file, or sometimes to append
to it. We should (in another CL) replace those clients with calls to WriteFile,
and maybe think about an "append" option or an AppendFile method. When the
output is not sent to a file, most likely it's being used to read the value from
an environment variable, so we may also want to write a dedicated method for
that at some point.

     else:
-      self.old_interface.SetFileContents(device_path, contents)
-
-  @decorators.WithTimeoutAndRetriesFromInstance()
-  def WriteTextFile(self, device_path, text, as_root=False, timeout=None,
-                    retries=None):
-    """Writes |text| to a file on the device.
-
-    Assuming that |text| is a small string, this is typically more efficient
-    than |WriteFile|, as no files are pushed into the device.
-
-    Args:
-      device_path: A string containing the absolute path to the file to write
-                   on the device.
-      text: A short string of text to write to the file on the device.
-      as_root: A boolean indicating whether the write should be executed with
-               root privileges.
-      timeout: timeout in seconds
-      retries: number of retries
-
-    Raises:
-      CommandFailedError if the file could not be written on the device.
-      CommandTimeoutError on timeout.
-      DeviceUnreachableError on missing device.
-    """
-    cmd = 'echo %s > %s' % (cmd_helper.SingleQuote(text),
-                            cmd_helper.SingleQuote(device_path))
-    self.RunShellCommand(cmd, as_root=as_root, check_return=True)
+      with tempfile.NamedTemporaryFile() as host_temp:
+        host_temp.write(contents)
+        host_temp.flush()
+        if as_root and self.NeedsSU():
+          with device_temp_file.DeviceTempFile(self) as device_temp:
+            self.adb.Push(host_temp.name, device_temp.name)
+            # Here we need 'cp' rather than 'mv' because the temp and
+            # destination files might be on different file systems (e.g.
+            # on internal storage and an external sd card)
+            self.RunShellCommand(['cp', device_temp.name, device_path],
+                                 as_root=True, check_return=True)

[perezju, 2014/11/12 01:31:24]

So, I had to resort to 'cp' rather than 'mv' in the end. Still this makes me
wonder, for the most common use cases, should temp files be written to
/data/local/tmp or to external storage (as done now)?

[jbudorick, 2014/11/12 01:42:37]

While I think we should try to move away from using /data/local/tmp (as it's
only readable from APKs, not writable), it would probably be ok to use it for
DeviceTempFile. It'd be interesting to see if there's a noticeable difference in
speed.

However, as usual, not in this CL.

+        else:
+          self.adb.Push(host_temp.name, device_path)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def Ls(self, device_path, timeout=None, retries=None):
     """Lists the contents of a directory on the device.
 
     Args:
       device_path: A string containing the path of the directory on the device
                    to list.
       timeout: timeout in seconds
       retries: number of retries
@@ skipping 220 matching lines @@
     Returns:
       A Parallelizer operating over |devices|.
     """
     if not devices or len(devices) == 0:
       devices = pylib.android_commands.GetAttachedDevices()
     parallelizer_type = (parallelizer.Parallelizer if async
                          else parallelizer.SyncParallelizer)
     return parallelizer_type([
         d if isinstance(d, DeviceUtils) else DeviceUtils(d)
         for d in devices])