Migrate device utils WriteFile to AdbWrapper

build/android/pylib/device/device_utils.py

Index: build/android/pylib/device/device_utils.py
diff --git a/build/android/pylib/device/device_utils.py b/build/android/pylib/device/device_utils.py
index b75a41a2f1a2401e98982ad044386ec517984fff..4c6a81fdb55b01b4a4829b25ae50617a3268be27 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -24,6 +24,7 @@ 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
@@ -866,16 +867,19 @@ class DeviceUtils(object):
       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
 
@@ -884,39 +888,31 @@ class DeviceUtils(object):
       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):