AdbCommands structure change, Pycryptodome support, python3 fixes #82

greateggsgreg · 2018-01-31T19:03:22Z

Interactive Shell support (instead of single shell commands)
Changed the AdbCommands static class paradigm to an object oriented design
Added support for pycryptodome
Python3 compatibility fixes
Misc adb protocol bug fixes

…s and python3 compatibility issues

googlebot · 2018-01-31T19:03:26Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

greateggsgreg · 2018-01-31T19:15:43Z

I signed it!

googlebot · 2018-01-31T19:15:46Z

CLAs look good, thanks!

greateggsgreg · 2018-01-31T19:17:18Z

EDIT: merged with master

fahhem

It's really hard to review this because you changed the spacing as well as the code, so all I see is a wall of red and then green instead of line-by-line changes. Any chance you can undo the formatting changes? I'm not opposed to them, but I'd prefer them as a separate pull-request

fahhem · 2018-02-01T00:22:55Z

adb/adb_commands.py

+
+        conn = self._get_service_connection(b'shell:')
+
+        return self.protocol_handler.InteractiveShellCommand(conn, command=command, strip_command=strip_command,


Can you bring this under 80 chars easily?

Happy to make these formatting changes so its easier to see the diffs, give me a day or so to revisit it and I'll push up the changes.

fahhem · 2018-02-01T00:30:23Z

I actually have a branch somewhere at home changing this to be a class, that staticmethods-on-a-class approach is definitely one of those "man, what was I thinking..." choices. I'm definitely in favor of fixing that to be a normal object-oriented design. In fact, I think it's necessary for a few things I've been wanting to do, like tracking multiple open connections (you've started that here).

…a bit more work to do

greateggsgreg · 2018-02-03T21:24:32Z

adb/adb_protocol.py


-        signed_token = rsa_key.Sign(str(banner))
+        # Do not mangle the banner property here by converting it to a string
+        signed_token = rsa_key.Sign(banner)


Before this was sending a mangled banner to the device instead of the true string. Removing the string cast fixed this.

greateggsgreg · 2018-02-03T21:31:24Z

adb/adb_commands.py

      destination_dir = '/data/local/tmp/'
    basename = os.path.basename(apk_path)
-    destination_path = destination_dir + basename
+    destination_path = os.path.join(destination_dir, basename)


Concatenating the string often got the path wrong ('/data/local/tmptest' vs '/data/local/tmp/test'). Using os.path.join to ensure it produces a correct path.

The problem with os.path.join is that if you're on Windows, it'll use \ instead of /. We can use posixpath still though:

import posixpath; destination_path = posixpath.join(destination_dir, basename)

greateggsgreg · 2018-02-03T21:32:36Z

adb/adb_commands.py

+      source_file = open(source_file, 'rb')

-    connection = self.protocol_handler.Open(
+    with source_file:


This is a bit cleaner than the current should_close variable solution in master. Allows an external caller to pass any file-like object and ensure its properly closed.

greateggsgreg · 2018-02-03T21:33:36Z

CONTRIBUTORS

 Jamey Hicks <jamey.hicks@gmail.com>
 Marc-Antoine Ruel <maruel@chromium.org>
 Max Borghino <fmborghino@gmail.com>
+Mohammad Abu-Garbeyyeh <github@mohammadag.com>


Manual merge from master

greateggsgreg · 2018-02-03T21:33:56Z

adb/adb_debug.py

      subparsers, parents, adb_commands.AdbCommands.Remount)
  common_cli.MakeSubparser(subparsers, parents, adb_commands.AdbCommands.Root)
+  common_cli.MakeSubparser(subparsers, parents, adb_commands.AdbCommands.EnableVerity)
+  common_cli.MakeSubparser(subparsers, parents, adb_commands.AdbCommands.DisableVerity)


Manual merge from master

greateggsgreg · 2018-02-03T21:34:30Z

adb/common.py

  def PortPathMatcher(cls, port_path):
    """Returns a device matcher for the given port path."""
-    if isinstance(port_path, basestring):
+    if isinstance(port_path, str):


basestring was removed in Python3. Using str here instead for py2/3 compatibility.

greateggsgreg · 2018-02-03T21:35:08Z

adb/filesync_protocol.py

    """
    if data:
-      if not isinstance(data, bytes):
+      if isinstance(data, str):


Strings are also bytes in Python3. Using str here instead for py2/3 compatibility then always encoding as bytes.

With this change, unicode in Python won't be encoded and we'll try to send unicode over the wire.

What was wrong with the previous line? We need bytes, so if it's not bytes, we encode using utf8 to get bytes.

greateggsgreg · 2018-02-03T21:36:26Z

setup.py


 from setuptools import setup

+# Figure out if the system already has a supported Crypto library


Not sure if you'll like this, but I wanted a way to not force M2Crypto to be installed in my environment if I had an alternative crypto library that was supported (pycryptodome). Happy to live with this not making it into master.

nah, but I wonder if there's an OR option in setuptools?

Couldn't find an OR option in setuptools. Most just perform logic like this in the actual setup.py.

greateggsgreg · 2018-02-03T21:36:40Z

test/adb_test.py

    self.assertEqual(b''.join(responses).decode('utf8'),
                     adb_commands.Shell(command))

+  def testUninstall(self):


Manual merge form master.

coveralls · 2018-02-04T02:44:34Z

Coverage decreased (-4.4%) to 42.691% when pulling 5badca3 on greateggsgreg:python3-and-misc-fixes into c3f93fa on google:master.

greateggsgreg · 2018-02-04T02:49:24Z

@fahhem I've updated all tests as well as Fastboot to conform to the new object model. All tests are passing again.

fahhem

Thanks for the hard work! Sorry for the delays, but I hope to get back to this next weekend.

If there's anything you don't want to do, just put a TODO on me to do it instead :)

fahhem · 2018-02-26T04:41:35Z

adb/adb_commands.py

+    # Connection table tracks each open AdbConnection objects per service type
+    # By default, the only service connections that make sense to hold open is (interactive) shell
+    self._service_connections = {
+        b'shell:': None


falsey contents of this dict don't affect anything, why put anything in it? Also, the comment makes it seems like only interactive shells would be kept open, but that's not true.

Okay, I've read more and it looks like _get_service_connection is only used by shell: connections, so the logic in it is repeated. How about adding a cache=False kwarg to it and using it everywhere self.protocol_handler.Open() is called?

This becomes a lot more useful for another branch I'm working on right now for implementing the 'adb forward' functionality. There, we'll need to hold/cache the service connection for our proxy while the TCP connection is open. I'll push that up when we're happy with this service connections implementation

Having a cache param in Open() isn't a bad idea, but since the Protocol Handler (AdbMessage) is basically a static class, it cannot hold state data like our get_service_connections method within AdbCommands.

I could refactor calls to protocol_handler.Open() and make them pass through a CreateServiceConnection() sort of method, with a cache=False parameter. Or, leave it to the individual program functions to handle it on their own (as it is now). Fine with either way, let me know what you think.

I've also updated the comments around self._service_connections so its a bit more clear that any service that needs to be persisted can take advantage of it, not just InteractiveShell. Again, this will be much more clear when the tcp:forward service uses this alongside shell:

fahhem · 2018-02-26T04:45:10Z

adb/adb_commands.py

      destination_dir = '/data/local/tmp/'
    basename = os.path.basename(apk_path)
-    destination_path = destination_dir + basename
+    destination_path = os.path.join(destination_dir, basename)


The problem with os.path.join is that if you're on Windows, it'll use \ instead of /. We can use posixpath still though:

import posixpath; destination_path = posixpath.join(destination_dir, basename)

fahhem · 2018-02-26T04:47:11Z

adb/adb_commands.py

        self.handle, destination=b'sync:', timeout_ms=timeout_ms)
-    self.filesync_handler.Push(connection, source_file, device_filename,
+      self.filesync_handler.Push(connection, source_file, device_filename,
                               mtime=int(mtime), progress_callback=progress_callback)


indentation

This is intentional so that the Push() call is within the 'with source_file' block, since Push() uses source_file

fahhem · 2018-02-26T04:51:53Z

adb/adb_commands.py

-        handle = common.TcpHandle(serial, timeout_ms=default_timeout_ms)
+
+    # If there isnt a handle override (used by tests), build one here
+    if not kwargs.get('handle', None):


if 'handle' in kwargs:

fahhem · 2018-02-26T04:52:52Z

adb/adb_commands.py

-            DeviceIsAvailable, port_path=port_path, serial=serial,
-            timeout_ms=default_timeout_ms)
-    return cls.Connect(handle, **kwargs)
+      self.handle = kwargs.pop('handle')


not in the docstring here or for fastboot. Also, if you reorder this a bit, you can combine the if/elifs:

if 'handle' in kwargs: self.handle = kwargs.pop('handle') elif serial and b':' in serial: self.handle = common.TcpHandle(serial, timeout_ms=default_timeout_ms) else: self.handle = common.UsbHandle.FindAndOpen( DeviceIsAvailable, port_path=port_path, serial=serial, timeout_ms=default_timeout_ms)

Fixed, thanks for the suggestion!

fahhem · 2018-02-26T05:42:38Z

setup.py

 from setuptools import setup

+# Figure out if the system already has a supported Crypto library
+rsa_signer_library = 'M2Crypto>=0.21.1,<=0.26.4'


this is a no-op, set it to None here to avoid the double mention of the version string

Wasn't able to find an 'OR' option in setup tools without writing the logic in. I've removed one of the version strings to the order of preference is kept for the supported cryptolibraries (1. M2Crypto, 2. Python-Rsa, 3. Pycryptodome)

fahhem · 2018-02-26T05:43:09Z

setup.py


 from setuptools import setup

+# Figure out if the system already has a supported Crypto library


nah, but I wonder if there's an OR option in setuptools?

fahhem · 2018-02-26T05:45:04Z

setup.py


-    install_requires = ['libusb1>=1.0.16', 'M2Crypto>=0.21.1,<=0.26.4'],
+    install_requires = [
+        'future',


where is this used?

Removed, see comments below

Same as above

fahhem · 2018-02-26T05:46:02Z

adb/adb_protocol.py

 import time
-
+from io import BytesIO
+from future.utils import iteritems


http://python-future.org/reference.html?highlight=iteritems#future.utils.iteritems

Use viewitems?

Just reverted to items below. Now there is no reliance on the future library

fahhem · 2018-02-26T05:50:12Z

adb/adb_protocol.py

      for cmd_id in ids
  }
-  wire_to_id = {wire: cmd_id for cmd_id, wire in id_to_wire.items()}
+  wire_to_id = {wire: cmd_id for cmd_id, wire in iteritems(id_to_wire)}


why go from items to iteritems? This isn't a big object so the overhead of the copy is small, and it's correct in py 3 anyway

You're right, no real benefit to iterates here. Reverted to items()

…astboot_debug where chunk_kb was not being passed through

…to stderr

…ng, made build_prop a public property, improved documentation for ConnectDevice

greateggsgreg · 2018-02-27T00:22:31Z

@fahhem I believe I've addressed every one of comments and made changes where appropriate. Please let me know if you have any additional questions and let me know if there's more I can do.

Thanks!

fahhem · 2018-03-01T20:21:10Z

setup.py


-    install_requires = ['libusb1>=1.0.16', 'M2Crypto>=0.21.1,<=0.26.4'],
+    install_requires = [
+        'future',


Same as above

fahhem · 2018-03-01T20:46:14Z

adb/adb_commands.py

-              DeviceIsAvailable, port_path=port_path, serial=serial,
-              timeout_ms=default_timeout_ms)
-
+    if 'handle' in kwargs:


Calling ConnectDevice(handle=...) is equivalent to Connect(handle), so why is Connect() private? I'm not a big fan of name-mangling-private in Python anyway, nothing's really private in Python anyway, so why not just use one _ and let anyone who wants to hang themselves easier access to the rope?

Agreed, updated the method to be protected ('_') and the documentation around it and its relationship to ConnectDevice

fahhem · 2018-03-01T21:10:43Z

One last comment before it's ready, thanks for the diligent work!

fahhem · 2018-03-01T21:11:13Z

Oh, and include a change to CONTRIBUTORS adding your info to it

…called externally. Fixed documentation around the method and its relationship to ConnectDevice()

greateggsgreg · 2018-03-03T20:44:07Z

@fahhem Thanks! Made the change to _Connect and contributors.

Edit: Just noticed your comment in setup.py about the 'future' bit, or were your referring to the M2Crypto requirement? Future has been removed from the requirements as its no long used, and I've kept the conditional logic for the crypto library based on whats already installed, defaulting to M2Crypto. Did you want that removed for the previous behavior and just keep other AuthSigners around for customization? Happy either way!

fahhem

Ah, that was a comment I accidentally left on the previous version of your code where 'future' was still a requirement, ignore it :)

fahhem · 2018-03-03T23:48:27Z

If you want to do the formatting changes as the next PR, that would be great :)

I'll make a new release once the Py3 compatibility PRs are merged or closed, hopefully this week

greateggsgreg added 4 commits January 31, 2018 13:26

Added pycharm .idea/ folder to gitignore

70fd2dd

Added authsigner for pycryptodome

6e4e466

Added support for retrieving the reason for a command failure

54b3ed8

Revised AdbCommands model to be an instantiated class. Fixed misc bug…

fd30265

…s and python3 compatibility issues

fahhem suggested changes Feb 1, 2018

View reviewed changes

greateggsgreg added 10 commits February 3, 2018 15:08

Merge adb_test and CONTRIBUTORS from master

3f7eefb

Additional formatting cleanup for common_cli

0eca1c7

Fixed formatting in filesync_protocol

a3b6c22

common.py merges with master and spacing fixes

7b1c115

Fixed spacing issues in adb_protocol. Removed unused code

7b39bc3

Added future to the requirements for py3 compatibility

240279e

Fixed spacing issues in adb_debug

14a0f2f

Fixed up adb_commands spacing

13d75f8

Minor spacing change to requirements list

60ecb1c

Updated InteractiveShellCommand to handle changing delimiters, still …

71aa542

…a bit more work to do

greateggsgreg commented Feb 3, 2018

View reviewed changes

Dynamic detection of rsa library in setup.py

dc7bafd

greateggsgreg commented Feb 3, 2018

View reviewed changes

Updated adb_test to support py2/3. Fixed exit bug in adb_commands

8420ba3

greateggsgreg added 4 commits February 17, 2018 14:35

Fixed merge conflicts with master

45937ce

added .DS_Store files to gitignore

855b33d

Fixed minor syntax issue in function declaration

821d962

Fixed merge issues

550b39e

fahhem reviewed Feb 26, 2018

View reviewed changes

greateggsgreg added 12 commits February 26, 2018 18:04

removed reliance on future library

fd09a4f

Improved FastbootCommands.ConnectDevice documentation. Fixed bug in f…

06babd6

…astboot_debug where chunk_kb was not being passed through

FilesyncProtocol: Simplified handling of utf-8 decoding errors

e555496

Imported __future__.print_function for python2/3 compatible printing …

b6284bf

…to stderr

AdbProtocol: Reverted reliance on future.iteritems

257b3ad

Made handle a protected member, removed unused code, fixed line spaci…

636ceba

…ng, made build_prop a public property, improved documentation for ConnectDevice

Simplified logic in fastboot.ConnectDevice

a308704

Fixed merge conflicts

d063e33

setup.py: Removed duplicate version string for M2Crypto requirements

6d7404e

Fixed check for bytes in filesync_protocol

7a1a1c7

Updated documentation around self._service_connections

45f0f0a

Additional docstrings for progress_callback parameter

c8fcbc4

fahhem reviewed Mar 1, 2018

View reviewed changes

greateggsgreg added 2 commits March 3, 2018 15:42

Updated adb_commands._Connect to be a protected member that could be …

2eea6b9

…called externally. Fixed documentation around the method and its relationship to ConnectDevice()

Updated contributors

5badca3

fahhem approved these changes Mar 3, 2018

View reviewed changes

fahhem merged commit 854be63 into google:master Mar 3, 2018


		conn = self._get_service_connection(b'shell:')

		return self.protocol_handler.InteractiveShellCommand(conn, command=command, strip_command=strip_command,


		from setuptools import setup

		# Figure out if the system already has a supported Crypto library

AdbCommands structure change, Pycryptodome support, python3 fixes #82

AdbCommands structure change, Pycryptodome support, python3 fixes #82

Uh oh!

Conversation

greateggsgreg commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jan 31, 2018

Uh oh!

greateggsgreg commented Jan 31, 2018

Uh oh!

googlebot commented Jan 31, 2018

Uh oh!

greateggsgreg commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fahhem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fahhem commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greateggsgreg commented Feb 4, 2018

Uh oh!

fahhem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greateggsgreg Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

greateggsgreg commented Jan 31, 2018 •

edited

Loading

greateggsgreg commented Jan 31, 2018 •

edited

Loading

coveralls commented Feb 4, 2018 •

edited

Loading

greateggsgreg Feb 26, 2018 •

edited

Loading

greateggsgreg Feb 26, 2018 •

edited

Loading