Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

New package adb-shell #167

Open
JeffLIrion opened this issue Sep 24, 2019 · 6 comments
Open

New package adb-shell #167

JeffLIrion opened this issue Sep 24, 2019 · 6 comments

Comments

@JeffLIrion
Copy link
Contributor

For anyone interested, I started a new repo/package based on this project: https://github.com/JeffLIrion/adb_shell

This package implements ADB shell functionality over TCP. You can install it via

pip install adb-shell

Highlights

  • Complete documentation
    • Some parameters that I don't understand completely, such as arg0 and arg1, have TODO labels, and the auth/keygen.py file has a lot of TODO items
  • 95% code coverage
    • The only module that is not 100% covered is auth/keygen.py
  • Python3 support and Python2 support (Python3 is the main target)
  • Linting checks to enforce code style
    • I disabled line length checks because I don't care much about that
  • All data that is sent/received is logged at the debug level to help with debugging of any issues

Pull requests from this repo included in adb_shell

Contributing

Contributions are welcome and will be reviewed and released on pypi!!! If you'd like to contribute, please

  • make sure that your code is covered with tests
    • if you're fixing a bug, your test should fail with the current code and pass with your changes
  • document your code using the existing style (numpydoc)
  • test that your updated package works correctly
@embray
Copy link

embray commented Sep 25, 2019

@JeffLIrion Thanks, that's good to know. It's a shame Google seems to have abandoned support for this (yes, yes, I know they're hard at work killing android but in the meantime some of us still develop for it :)

@JeffLIrion
Copy link
Contributor Author

@embray what I'd most like to fix is the duplicate CLSE issue (#151). I recall getting feedback that your fix didn't fully solve it. I don't have a newer ADB device, so I'm not able to reproduce the error. But with the adb-shell package, if you set the log level to debug it will log all sent and received data, making it possible to (hopefully!) fix the bug.

@fahhem
Copy link
Contributor

fahhem commented Oct 16, 2019

Hey :)

Thank you @JeffLIrion! I really appreciate the improvements you've made, and I was going to just point everyone to your repo, but I think the better option is to add you to this one. I hope you accept and we can make the official, non-official google python-adb repo much better.

Honestly, the only benefit to this approach is it says "google" in the github URL and the PyPI project name is just "adb". Let me know what your PyPI credentials are and I'll add you there too

@JeffLIrion
Copy link
Contributor Author

Prior to creating the adb-shell package, I dug into this code and tried to figure out how it works so that I could fix some bugs. Honestly, I found the code to be very confusing, in particular due to the abundance of classes and the widespread use of class methods. I created a branch that has a bunch of documentation and linting fixes, you can view that here: https://github.com/JeffLIrion/python-adb/tree/documentation

If you want, I'll squash those commits and submit a pull request. The API is the same and there should be no breaking changes.

But ultimately, I decided that it would be easier to extract the functionality that I wanted and create a new repo. You’re more than welcome to copy anything you'd like from adb-shell. The tests could be especially useful: https://github.com/JeffLIrion/adb_shell/blob/master/tests/test_adb_device.py

Also, I'm by no means an expert in ADB or Android. I don't even know what fastboot or filesync are.

@fahhem
Copy link
Contributor

fahhem commented Oct 17, 2019

I'd be very happy to accept a PR like that.

Is the the adb_shell API incompatible in a meaningful way? python-adb calls things AdbCommands, while you have an AdbDevice, but it seems there's a 1:1 mapping?

I'm not going to stand behind the naming/style of this repo, mostly because it was a straight copy out of the google3 repo that, at the time, had non-PEP8-compatible style requirements. It would be reasonably to me to have a 2.0 release that's a different API that's easy to switch to (since it's 1:1), but PEP8 compatible.

@JeffLIrion
Copy link
Contributor Author

I'll prepare that pull request that I mentioned.

The difference between the two classes is most evident in how ADB commands are sent. From memory, the processes are as follows.

AdbDevice

  1. Open a connection using the AdbDevice._open method.
  2. Create msg = AdbMessage(...) using the command's data.
  • AdbMessage is a small helper class whose only real functionalities are the pack method and the checksum property.
  1. Using the AdbDevice._send method, send msg.pack() and msg.data.
  2. Read responses.

AdbCommands

  1. Create msg = ADB message(...), passing along the USB/TCP handle attribute from the AdbCommands instance.
  2. msg opens a connection by creating a new instance of the AdbConnection class, again passing along the USB/TCP handle.
  3. A Send method is called. I forget whether this is a method of the AdbMessage class or the AdbConnection class, but I'm pretty sure both classes are involved. And this is sent using the USB/TCP handle from the AdbCommands class.
  4. Responses are read, either by the AdbMessage instance or the AdbConnection instance.

I don't know what the reasoning is behind all the classes and class methods in this package -- to be specific, I don't know whether it's necessary for some functionalities or just a design choice -- so I didn't want to change anything. But if I were to restructure the code, I'd do it as I did in the adb-shell package.

I've got generated documentation for both packages:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants