Android Emulator Driver #437
Conversation
WalkthroughThis change introduces the new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AndroidClient
participant AdbClient
participant ScrcpyClient
participant TcpPortforwardAdapter
participant AdbServer
participant AndroidEmulatorPower
User->>AndroidClient: Run adb/scrcpy/power command
AndroidClient->>AdbClient: For adb command
AdbClient->>TcpPortforwardAdapter: Forward remote ADB server to local port
TcpPortforwardAdapter-->>AdbClient: Local ADB port available
AdbClient->>AdbServer: (If start-server/kill-server) Control ADB server
AdbClient->>User: Execute local adb CLI with forwarded server
AndroidClient->>ScrcpyClient: For scrcpy command
ScrcpyClient->>TcpPortforwardAdapter: Forward remote ADB server to local port
TcpPortforwardAdapter-->>ScrcpyClient: Local ADB port available
ScrcpyClient->>User: Execute local scrcpy CLI with forwarded server
AndroidClient->>AndroidEmulatorPower: For power command
AndroidEmulatorPower->>AndroidEmulatorPower: Start/stop emulator process
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@mangelajo this should be ready for review now if you want to try it out. I've tested it on Mac, but I don't have a Linux machine with the full Android SDK downloaded on it, so we'll need to test it there. We also probably need to build (or find) a good Android SDK / Emulator container image to use this in a cluster. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/options.py (1)
201-203: 🛠️ Refactor suggestionRemove deprecated Pydantic configuration.
The
validate_assignmentoption is deprecated in Pydantic v2. Since this appears to be a new implementation, consider using Pydantic v2 patterns or verify compatibility with your Pydantic version.class Config: - validate_assignment = True + # Remove deprecated validate_assignment + passIf you need assignment validation in Pydantic v2, use
model_config:from pydantic import ConfigDict class EmulatorOptions(BaseModel): model_config = ConfigDict(validate_assignment=True) # ... rest of the class🧰 Tools
🪛 Pylint (3.3.7)
[convention] 201-201: Missing class docstring
(C0115)
[refactor] 201-201: Too few public methods (0/2)
(R0903)
♻️ Duplicate comments (1)
packages/jumpstarter-driver-android/README.md (1)
11-11: Use the correct package name.The package name should be
jumpstarter-driver-androidto match the actual package structure.-pip3 install --extra-index-url https://pkg.jumpstarter.dev/simple/ jumpstarter-android +pip3 install --extra-index-url https://pkg.jumpstarter.dev/simple/ jumpstarter-driver-android
🧹 Nitpick comments (25)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/scrcpy.py (1)
1-13: Add class docstring and remove unnecessary pass statementThe implementation is appropriately minimal for a stub driver, but could benefit from documentation and cleanup.
+""" +Scrcpy driver for Android screen mirroring. + +This driver acts as a stub TcpNetwork driver that manages ADB forwarding +internally to support the scrcpy tool for screen mirroring. +""" from dataclasses import dataclass from jumpstarter_driver_network.driver import TcpNetwork @dataclass(kw_only=True) class Scrcpy(TcpNetwork): + """ + Scrcpy driver for Android screen mirroring. + + Acts as a stub TcpNetwork driver with internal ADB forwarding management. + """ + @classmethod def client(cls) -> str: + """Return the fully qualified client class name.""" return "jumpstarter_driver_android.client.ScrcpyClient" - - pass🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'jumpstarter_driver_network.driver'
(E0401)
[convention] 7-7: Missing class docstring
(C0115)
[convention] 9-9: Missing function or method docstring
(C0116)
[warning] 12-12: Unnecessary pass statement
(W0107)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/__init__.py (1)
1-13: Add module docstring for better documentation.Consider adding a module docstring to describe the purpose of this driver package:
+""" +Android driver package for Jumpstarter. + +This package provides drivers and configuration options for Android devices and emulators, +including ADB server management, emulator power control, and scrcpy integration. +""" from .adb import AdbServer🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/device.py (3)
18-20: Add docstring for the client method.The client method should have a docstring to explain its purpose:
@classmethod def client(cls) -> str: + """Return the client class identifier for this driver.""" return "jumpstarter_driver_android.client.AndroidClient"🧰 Tools
🪛 Pylint (3.3.7)
[convention] 19-19: Missing function or method docstring
(C0116)
31-33: Fix line length violation.The line exceeds the 100-character limit. Consider splitting the constructor call:
- self.children["adb"] = AdbServer( - host=self.adb.host, port=self.adb.port, adb_path=self.adb.adb_path, log_level=self.log_level - ) + self.children["adb"] = AdbServer( + host=self.adb.host, + port=self.adb.port, + adb_path=self.adb.adb_path, + log_level=self.log_level, + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 32-32: Line too long (108/100)
(C0301)
[error] 31-33: Unexpected keyword argument 'log_level' in constructor call
(E1123)
35-35: Fix line length violation.The line exceeds the 100-character limit. Consider splitting the constructor call:
- self.children["scrcpy"] = Scrcpy(host=self.adb.host, port=self.adb.port, log_level=self.log_level) + self.children["scrcpy"] = Scrcpy( + host=self.adb.host, port=self.adb.port, log_level=self.log_level + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 35-35: Line too long (110/100)
(C0301)
[error] 35-35: Unexpected keyword argument 'host' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'port' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'log_level' in constructor call
(E1123)
packages/jumpstarter-driver-android/pyproject.toml (1)
4-4: Add package description.The description field is empty. Consider adding a meaningful description:
-description = "" +description = "Android device and emulator drivers for Jumpstarter"packages/jumpstarter/jumpstarter/common/utils.py (1)
48-49: Fix line length violations.Both prompt constant definitions exceed the 100-character limit:
-BASH_PROMPT = f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{{context}} {ANSI_YELLOW}➤{ANSI_RESET} " -ZSH_PROMPT = "%F{grey}%~ %F{yellow}⚡%F{white}{{context}} %F{yellow}➤%f " +BASH_PROMPT = ( + f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{{context}} " + f"{ANSI_YELLOW}➤{ANSI_RESET} " +) +ZSH_PROMPT = "%F{grey}%~ %F{yellow}⚡%F{white}{{context}} %F{yellow}➤%f "🧰 Tools
🪛 Pylint (3.3.7)
[convention] 48-48: Line too long (106/100)
(C0301)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb_test.py (2)
26-26: Consider breaking this long line for better readability.The line exceeds 100 characters. You can improve readability by breaking it into multiple lines.
- call(["/usr/bin/adb", "version"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True), + call( + ["/usr/bin/adb", "version"], + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 26-26: Line too long (117/100)
(C0301)
53-53: Consider breaking this long line for better readability.The line exceeds 100 characters. For consistency with the suggested change above, consider breaking this line as well.
- call(["/usr/bin/adb", "version"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True), + call( + ["/usr/bin/adb", "version"], + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 53-53: Line too long (117/100)
(C0301)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py (3)
32-55: Consider error handling strategy for start_server return value.The method returns
self.porteven when the ADB server fails to start (line 54). This could be misleading to callers who might interpret a returned port as successful startup.Consider either:
- Raising an exception on failure instead of just logging
- Returning
Noneor a tuple(port, success)to indicate status
48-48: Inconsistent stderr logging between start_server and kill_server.In
start_server, stderr is logged at debug level (line 48), but inkill_serverit's logged at error level (line 72). For consistency, consider using the same approach in both methods.- self._print_output(result.stderr, error=True) + self._print_output(result.stderr, debug=True)Also applies to: 72-72
101-101: Consider breaking this long line for better readability.The line exceeds 100 characters limit.
- [self.adb_path, "version"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True + [self.adb_path, "version"], + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True🧰 Tools
🪛 Pylint (3.3.7)
[convention] 101-101: Line too long (113/100)
(C0301)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator_test.py (1)
152-152: Fix line length exceeding 100 characters.The function definition exceeds the 100-character line limit.
Split the function definition across multiple lines:
-def test_emulator_arguments(scrcpy: MagicMock, adb: MagicMock, mock_thread: MagicMock, mock_popen: MagicMock): +def test_emulator_arguments( + scrcpy: MagicMock, adb: MagicMock, mock_thread: MagicMock, mock_popen: MagicMock +):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 152-152: Line too long (110/100)
(C0301)
[convention] 152-152: Missing function or method docstring
(C0116)
[warning] 152-152: Unused argument 'mock_thread'
(W0613)
docs/source/reference/package-apis/drivers/android.md (3)
81-81: Fix missing possessive apostrophe.The phrase should use the possessive form.
-- `adb` - `AdbServer` instance configured to tunnel the Android devices ADB connection. +- `adb` - `AdbServer` instance configured to tunnel the Android device's ADB connection.🧰 Tools
🪛 LanguageTool
[uncategorized] ~81-~81: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...stance configured to tunnel the Android devices ADB connection. -scrcpy-Scrcpyi...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
91-91: Fix missing possessive apostrophe.The phrase should use the possessive form.
-- `adb` - `AdbServer` instance configured to tunnel the Android devices ADB connection. +- `adb` - `AdbServer` instance configured to tunnel the Android device's ADB connection.🧰 Tools
🪛 LanguageTool
[uncategorized] ~91-~91: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...stance configured to tunnel the Android devices ADB connection. -scrcpy-Scrcpyi...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
93-93: Fix typo in "emulator".-- `power` - `AndroidEmulatorPower` instance to turn on/off an emualtor instance. +- `power` - `AndroidEmulatorPower` instance to turn on/off an emulator instance.packages/jumpstarter-driver-android/README.md (4)
87-87: Fix typo in "start/stop".-interface to remotely start/top an android emulator instance running on the exporter. +interface to remotely start/stop an android emulator instance running on the exporter.
81-81: Fix missing possessive apostrophe.The phrase should use the possessive form.
-- `adb` - `AdbServer` instance configured to tunnel the Android devices ADB connection. +- `adb` - `AdbServer` instance configured to tunnel the Android device's ADB connection.
91-91: Fix missing possessive apostrophe.The phrase should use the possessive form.
-- `adb` - `AdbServer` instance configured to tunnel the Android devices ADB connection. +- `adb` - `AdbServer` instance configured to tunnel the Android device's ADB connection.
93-93: Fix typo in "emulator".-- `power` - `AndroidEmulatorPower` instance to turn on/off an emualtor instance. +- `power` - `AndroidEmulatorPower` instance to turn on/off an emulator instance.packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator.py (4)
83-268: Consider adding a docstring to document the command construction logic.While the method is long, the implementation correctly maps all emulator options to command line arguments. The straightforward mapping makes it maintainable despite the length.
Add a docstring to help future maintainers:
def _make_emulator_command(self) -> list[str]: - """Construct the command to start the Android emulator.""" + """ + Construct the command to start the Android emulator. + + Maps EmulatorOptions fields to their corresponding command line arguments + following the Android emulator CLI interface. + + Returns: + list[str]: Complete command line arguments for starting the emulator. + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 153-153: Line too long (101/100)
(C0301)
[convention] 154-154: Line too long (101/100)
(C0301)
[convention] 157-157: Line too long (105/100)
(C0301)
[convention] 168-168: Line too long (102/100)
(C0301)
[convention] 169-169: Line too long (108/100)
(C0301)
[convention] 173-173: Line too long (105/100)
(C0301)
[convention] 177-177: Line too long (114/100)
(C0301)
[convention] 179-179: Line too long (116/100)
(C0301)
[convention] 229-229: Line too long (117/100)
(C0301)
[convention] 239-239: Line too long (104/100)
(C0301)
[convention] 249-249: Line too long (111/100)
(C0301)
[refactor] 83-83: Too many statements (142/50)
(R0915)
305-359: Add docstring and fix async method signature.The shutdown logic is thorough and handles edge cases well, but needs to match the interface signature.
@export -def off(self) -> None: # noqa: C901 +async def off(self) -> None: # noqa: C901 + """ + Power off the Android emulator gracefully. + + Attempts graceful shutdown via 'adb emu kill' command first, then + falls back to forcibly killing the process if it doesn't terminate + within 20 seconds. Also ensures logging threads are cleaned up. + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 311-311: Line too long (109/100)
(C0301)
[convention] 335-335: Line too long (111/100)
(C0301)
[convention] 341-341: Line too long (106/100)
(C0301)
[convention] 306-306: Missing function or method docstring
(C0116)
[warning] 328-328: Catching too general exception Exception
(W0718)
[refactor] 306-306: Too many branches (13/12)
(R0912)
360-364: Document that this is an intentional stub implementation.Since emulators don't have real power measurements, returning zero values is appropriate.
@export async def read(self) -> AsyncGenerator[PowerReading, None]: + """ + Yield dummy power readings for the emulator. + + Emulators don't have real power consumption, so this always + yields zero values to satisfy the PowerInterface contract. + """ yield PowerReading(voltage=0.0, current=0.0) return🧰 Tools
🪛 Pylint (3.3.7)
[convention] 361-361: Missing function or method docstring
(C0116)
365-367: Simple and correct cleanup implementation.Properly ensures the emulator is powered off during cleanup.
Consider making this async to match the
off()method:-def close(self): +async def close(self): + """Ensure the emulator is powered off during cleanup.""" - self.off() + await self.off()🧰 Tools
🪛 Pylint (3.3.7)
[convention] 365-365: Missing function or method docstring
(C0116)
packages/jumpstarter-driver-android/jumpstarter_driver_android/client.py (1)
18-22: Remove unnecessary pass statement.The class already has a docstring, so the pass statement is redundant.
class AndroidClient(CompositeClient): """Generic Android client for controlling Android devices/emulators.""" - - pass🧰 Tools
🪛 Pylint (3.3.7)
[warning] 21-21: Unnecessary pass statement
(W0107)
[refactor] 18-18: Too few public methods (0/2)
(R0903)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
docs/source/reference/package-apis/drivers/android.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(3 hunks)packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-android/README.md(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/client.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/__init__.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb_test.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/device.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator_test.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/options.py(1 hunks)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/scrcpy.py(1 hunks)packages/jumpstarter-driver-android/pyproject.toml(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(1 hunks)packages/jumpstarter/jumpstarter/common/utils.py(3 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (4)
packages/jumpstarter-driver-corellium/jumpstarter_driver_corellium/driver.py (2)
url(209-231)url(234-235)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
connect(55-69)packages/jumpstarter/jumpstarter/client/lease.py (1)
connect(198-201)packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver.py (1)
connect(77-80)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/scrcpy.py (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(35-60)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb_test.py (2)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py (3)
AdbServer(13-105)start_server(33-54)kill_server(57-80)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ConfigurationError(37-40)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator.py (5)
packages/jumpstarter-driver-power/jumpstarter_driver_power/common.py (1)
PowerReading(4-10)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (1)
PowerInterface(8-20)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/device.py (1)
AndroidDevice(13-35)packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/options.py (1)
EmulatorOptions(19-202)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(43-251)
🪛 Pylint (3.3.7)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/scrcpy.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'jumpstarter_driver_network.driver'
(E0401)
[convention] 7-7: Missing class docstring
(C0115)
[convention] 9-9: Missing function or method docstring
(C0116)
[warning] 12-12: Unnecessary pass statement
(W0107)
packages/jumpstarter/jumpstarter/common/utils.py
[convention] 48-48: Line too long (106/100)
(C0301)
[convention] 69-69: Line too long (107/100)
(C0301)
[warning] 69-69: Redefining name 'env' from outer scope (line 12)
(W0621)
[convention] 78-78: Line too long (115/100)
(C0301)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb_test.py
[convention] 26-26: Line too long (117/100)
(C0301)
[convention] 53-53: Line too long (117/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 9-9: Unable to import 'jumpstarter.common.exceptions'
(E0401)
[convention] 14-14: Missing function or method docstring
(C0116)
[convention] 41-41: Missing function or method docstring
(C0116)
[convention] 67-67: Missing function or method docstring
(C0116)
[convention] 72-72: Missing function or method docstring
(C0116)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/__init__.py
[convention] 1-1: Missing module docstring
(C0114)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py
[convention] 101-101: Line too long (113/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'jumpstarter_driver_network.driver'
(E0401)
[error] 8-8: Unable to import 'jumpstarter.common.exceptions'
(E0401)
[error] 9-9: Unable to import 'jumpstarter.driver.decorators'
(E0401)
[convention] 13-13: Missing class docstring
(C0115)
[convention] 19-19: Missing function or method docstring
(C0116)
[warning] 78-78: Catching too general exception Exception
(W0718)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/device.py
[convention] 32-32: Line too long (108/100)
(C0301)
[convention] 35-35: Line too long (110/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'pydantic.dataclasses'
(E0401)
[error] 9-9: Unable to import 'jumpstarter.driver.base'
(E0401)
[convention] 19-19: Missing function or method docstring
(C0116)
[error] 31-33: Unexpected keyword argument 'log_level' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'host' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'port' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'log_level' in constructor call
(E1123)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/options.py
[convention] 109-109: Line too long (109/100)
(C0301)
[convention] 110-110: Line too long (107/100)
(C0301)
[convention] 111-111: Line too long (107/100)
(C0301)
[convention] 115-115: Line too long (106/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'pydantic'
(E0401)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
[convention] 201-201: Missing class docstring
(C0115)
[refactor] 201-201: Too few public methods (0/2)
(R0903)
[refactor] 19-19: Too few public methods (0/2)
(R0903)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator_test.py
[convention] 152-152: Line too long (110/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 16-16: Missing function or method docstring
(C0116)
[convention] 27-27: Missing function or method docstring
(C0116)
[warning] 27-27: Redefining name 'android_emulator' from outer scope (line 15)
(W0621)
[convention] 33-33: Missing function or method docstring
(C0116)
[warning] 33-33: Redefining name 'emulator_power' from outer scope (line 27)
(W0621)
[convention] 87-87: Missing function or method docstring
(C0116)
[warning] 87-87: Redefining name 'emulator_power' from outer scope (line 27)
(W0621)
[warning] 90-90: Access to a protected member _process of a client class
(W0212)
[warning] 119-119: Access to a protected member _process of a client class
(W0212)
[warning] 120-120: Access to a protected member _log_thread of a client class
(W0212)
[warning] 121-121: Access to a protected member _stderr_thread of a client class
(W0212)
[convention] 125-125: Missing function or method docstring
(C0116)
[warning] 125-125: Redefining name 'emulator_power' from outer scope (line 27)
(W0621)
[warning] 132-132: Access to a protected member _process of a client class
(W0212)
[warning] 143-143: Access to a protected member _process of a client class
(W0212)
[warning] 144-144: Access to a protected member _log_thread of a client class
(W0212)
[warning] 145-145: Access to a protected member _stderr_thread of a client class
(W0212)
[warning] 125-125: Unused argument 'mock_run'
(W0613)
[convention] 152-152: Missing function or method docstring
(C0116)
[warning] 152-152: Unused argument 'mock_thread'
(W0613)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator.py
[convention] 21-21: Line too long (101/100)
(C0301)
[convention] 153-153: Line too long (101/100)
(C0301)
[convention] 154-154: Line too long (101/100)
(C0301)
[convention] 157-157: Line too long (105/100)
(C0301)
[convention] 168-168: Line too long (102/100)
(C0301)
[convention] 169-169: Line too long (108/100)
(C0301)
[convention] 173-173: Line too long (105/100)
(C0301)
[convention] 177-177: Line too long (114/100)
(C0301)
[convention] 179-179: Line too long (116/100)
(C0301)
[convention] 229-229: Line too long (117/100)
(C0301)
[convention] 239-239: Line too long (104/100)
(C0301)
[convention] 249-249: Line too long (111/100)
(C0301)
[convention] 298-298: Line too long (113/100)
(C0301)
[convention] 311-311: Line too long (109/100)
(C0301)
[convention] 335-335: Line too long (111/100)
(C0301)
[convention] 341-341: Line too long (106/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 8-8: Unable to import 'jumpstarter_driver_power.common'
(E0401)
[error] 9-9: Unable to import 'jumpstarter_driver_power.driver'
(E0401)
[error] 10-10: Unable to import 'pydantic.dataclasses'
(E0401)
[error] 15-15: Unable to import 'jumpstarter.driver'
(E0401)
[convention] 35-35: Missing class docstring
(C0115)
[refactor] 83-83: Too many statements (142/50)
(R0915)
[convention] 271-271: Missing function or method docstring
(C0116)
[refactor] 288-295: Consider using 'with' for resource-allocating operations
(R1732)
[convention] 306-306: Missing function or method docstring
(C0116)
[warning] 328-328: Catching too general exception Exception
(W0718)
[refactor] 306-306: Too many branches (13/12)
(R0912)
[convention] 361-361: Missing function or method docstring
(C0116)
[convention] 365-365: Missing function or method docstring
(C0116)
packages/jumpstarter-driver-android/jumpstarter_driver_android/client.py
[convention] 63-63: Line too long (113/100)
(C0301)
[convention] 84-84: Line too long (113/100)
(C0301)
[convention] 85-85: Line too long (116/100)
(C0301)
[convention] 112-112: Line too long (113/100)
(C0301)
[convention] 116-116: Line too long (117/100)
(C0301)
[convention] 117-117: Line too long (116/100)
(C0301)
[convention] 120-120: Line too long (102/100)
(C0301)
[convention] 121-121: Line too long (106/100)
(C0301)
[convention] 124-124: Line too long (110/100)
(C0301)
[convention] 127-127: Line too long (116/100)
(C0301)
[convention] 129-129: Line too long (108/100)
(C0301)
[convention] 134-134: Line too long (114/100)
(C0301)
[convention] 146-146: Line too long (117/100)
(C0301)
[convention] 169-169: Line too long (111/100)
(C0301)
[convention] 180-180: Line too long (113/100)
(C0301)
[convention] 181-181: Line too long (116/100)
(C0301)
[convention] 198-198: Line too long (102/100)
(C0301)
[convention] 201-201: Line too long (118/100)
(C0301)
[convention] 202-202: Line too long (111/100)
(C0301)
[convention] 221-221: Line too long (112/100)
(C0301)
[convention] 232-232: Line too long (111/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'adbutils'
(E0401)
[error] 12-12: Unable to import 'jumpstarter_driver_composite.client'
(E0401)
[error] 13-13: Unable to import 'jumpstarter_driver_network.adapters'
(E0401)
[error] 15-15: Unable to import 'jumpstarter.client'
(E0401)
[warning] 21-21: Unnecessary pass statement
(W0107)
[refactor] 18-18: Too few public methods (0/2)
(R0903)
[convention] 82-82: Missing function or method docstring
(C0116)
[refactor] 98-98: Too many arguments (9/5)
(R0913)
[refactor] 98-98: Too many positional arguments (9/5)
(R0917)
[refactor] 98-98: Too many local variables (16/15)
(R0914)
[refactor] 148-160: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 169-169: Consider using 'with' for resource-allocating operations
(R1732)
[convention] 178-178: Missing function or method docstring
(C0116)
[refactor] 232-232: Consider using 'with' for resource-allocating operations
(R1732)
🪛 LanguageTool
docs/source/reference/package-apis/drivers/index.md
[uncategorized] ~65-~65: A punctuation mark might be missing here.
Context: ...er-qemu) - QEMU virtualization platform * **[Corellium](corellium.md)** (jumpstarter...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[uncategorized] ~67-~67: A punctuation mark might be missing here.
Context: ...) - Corellium virtualization platform * **[Android](android.md)** (jumpstarter-dri...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
packages/jumpstarter-driver-android/README.md
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...daemon server running on the exporter. This driver implements the TcpNetwork driv...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/source/reference/package-apis/drivers/android.md
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...daemon server running on the exporter. This driver implements the TcpNetwork driv...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~81-~81: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...stance configured to tunnel the Android devices ADB connection. - scrcpy - Scrcpy i...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[uncategorized] ~91-~91: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...stance configured to tunnel the Android devices ADB connection. - scrcpy - Scrcpy i...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🔇 Additional comments (17)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
244-246: LGTM: Docstring style improvementGood change to use double quotes for docstrings, which aligns with PEP 257 conventions and improves consistency across the codebase.
253-255: LGTM: Consistent docstring styleThe docstring style change maintains consistency with the class-level docstring update.
pyproject.toml (1)
10-10: LGTM: Proper workspace integrationThe Android driver package is correctly added to the workspace sources in alphabetical order, following the established pattern for other driver packages.
packages/jumpstarter-all/pyproject.toml (1)
15-15: LGTM: Meta-package dependency properly addedThe Android driver is correctly included in the jumpstarter-all meta-package dependencies, maintaining alphabetical order and ensuring comprehensive installation coverage.
docs/source/reference/package-apis/drivers/index.md (3)
18-19: LGTM: Logical categorization of Android device controlGood addition of Android device control to the System Control Drivers section, properly describing ADB-based device management.
61-69: LGTM: Well-structured virtualization drivers sectionExcellent reorganization creating a dedicated "Virtualization Drivers" section. The separation of QEMU, Corellium, and Android emulator functionality makes the documentation more intuitive and easier to navigate.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: A punctuation mark might be missing here.
Context: ...er-qemu) - QEMU virtualization platform * **[Corellium](corellium.md)** (jumpstarter...(AI_EN_LECTOR_MISSING_PUNCTUATION)
[uncategorized] ~67-~67: A punctuation mark might be missing here.
Context: ...) - Corellium virtualization platform * **[Android](android.md)** (jumpstarter-dri...(AI_EN_LECTOR_MISSING_PUNCTUATION)
90-90: LGTM: Proper toctree integrationThe android.md file is correctly added to the toctree in alphabetical order, ensuring proper documentation navigation.
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/device.py (1)
26-35: Good implementation of conditional driver composition.The
__post_init__method correctly implements conditional initialization of child drivers based on the disable flags. This provides flexibility to users who may only need ADB or scrcpy functionality.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 32-32: Line too long (108/100)
(C0301)
[convention] 35-35: Line too long (110/100)
(C0301)
[error] 31-33: Unexpected keyword argument 'log_level' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'host' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'port' in constructor call
(E1123)
[error] 35-35: Unexpected keyword argument 'log_level' in constructor call
(E1123)
packages/jumpstarter/jumpstarter/common/utils.py (1)
55-55: Good improvement in type annotation.The type annotation change from
[str]tolist[str]follows modern Python typing conventions and is more readable.packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb_test.py (2)
66-70: Good test coverage for missing ADB executable.The test correctly verifies that a
ConfigurationErroris raised when the ADB executable is not found in PATH.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 67-67: Missing function or method docstring
(C0116)
72-81: Excellent test coverage for port validation.The test comprehensively covers invalid port scenarios including negative values, values exceeding the valid range, and non-integer types.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 72-72: Missing function or method docstring
(C0116)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py (1)
22-31: Well-structured logging helper method.The
_print_outputmethod provides a clean way to handle multi-line output with appropriate log levels.packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/options.py (1)
19-200: Comprehensive and well-structured emulator configuration model.The
EmulatorOptionsmodel provides excellent coverage of Android emulator CLI options with appropriate validation constraints, regex patterns, and type restrictions. The logical grouping of options enhances maintainability.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 109-109: Line too long (109/100)
(C0301)
[convention] 110-110: Line too long (107/100)
(C0301)
[convention] 111-111: Line too long (107/100)
(C0301)
[convention] 115-115: Line too long (106/100)
(C0301)
[refactor] 19-19: Too few public methods (0/2)
(R0903)
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator.py (2)
18-32: LGTM! Clean implementation of the AndroidEmulator driver.The class properly extends
AndroidDeviceand follows the framework's pattern for adding child drivers.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 21-21: Line too long (101/100)
(C0301)
46-82: Well-implemented log processing with proper resource management.The method correctly handles log level mapping and ensures the pipe is closed even if an error occurs.
packages/jumpstarter-driver-android/jumpstarter_driver_android/client.py (2)
24-77: Well-designed base class with proper resource management.Good use of context managers for port forwarding and ADB client creation. The port checking logic is correctly implemented.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 63-63: Line too long (113/100)
(C0301)
148-156: 🛠️ Refactor suggestionAdd explicit return statement for better flow control.
After handling kill-server, the function should return to avoid falling through to the subprocess execution.
elif "kill-server" in args: remote_port = int(self.call("kill_server")) click.echo(f"Remote adb server killed on remote port exporter:{remote_port}") return 0 +# Ensure we don't fall through after handling special commands +return 0Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 148-160: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
| JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow), | ||
| "PS1": f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} ", | ||
| } | ||
| env = os.environ | {JUMPSTARTER_HOST: host, JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable name shadowing and line length.
The local env variable shadows the imported env from line 12, and the line is too long:
- env = os.environ | {JUMPSTARTER_HOST: host, JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow)}
+ shell_env = os.environ | {
+ JUMPSTARTER_HOST: host,
+ JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow),
+ }Then update the references to use shell_env instead of env in the rest of the function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env = os.environ | {JUMPSTARTER_HOST: host, JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow)} | |
| shell_env = os.environ | { | |
| JUMPSTARTER_HOST: host, | |
| JMP_DRIVERS_ALLOW: "UNSAFE" if unsafe else ",".join(allow), | |
| } |
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 69-69: Line too long (107/100)
(C0301)
[warning] 69-69: Redefining name 'env' from outer scope (line 12)
(W0621)
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/common/utils.py at line 69, rename the local
variable `env` to `shell_env` to avoid shadowing the imported `env` from line
12. Also, split the long line into multiple lines for better readability.
Finally, update all subsequent references in the function from `env` to
`shell_env` to maintain consistency.
| env["PS1"] = f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} " | ||
| elif cmd[0].endswith("zsh"): | ||
| cmd.append("-f") | ||
| cmd.append("-i") | ||
| env["PROMPT"] = "%F{grey}%~ %F{yellow}⚡%F{white}local %F{yellow}➤%f " |
There was a problem hiding this comment.
Fix critical bug: hardcoded context value.
Line 82 uses hardcoded "local" instead of the context parameter, and there are line length issues:
- env["PS1"] = f"{ANSI_GRAY}{PROMPT_CWD} {ANSI_YELLOW}⚡{ANSI_WHITE}{context} {ANSI_YELLOW}➤{ANSI_RESET} "
+ shell_env["PS1"] = BASH_PROMPT.format(context=context)
elif cmd[0].endswith("zsh"):
cmd.append("-f")
cmd.append("-i")
- env["PROMPT"] = "%F{grey}%~ %F{yellow}⚡%F{white}local %F{yellow}➤%f "
+ shell_env["PROMPT"] = ZSH_PROMPT.format(context=context)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 78-78: Line too long (115/100)
(C0301)
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/common/utils.py around lines 78 to 82,
replace the hardcoded string "local" in the zsh prompt assignment with the
dynamic `context` variable to correctly reflect the context. Also, adjust the
line to ensure it does not exceed the maximum line length by breaking it
appropriately or simplifying the string formatting.
| cmd.append("-i") | ||
| env["PROMPT"] = "%F{grey}%~ %F{yellow}⚡%F{white}local %F{yellow}➤%f " | ||
|
|
||
| process = Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update environment variable reference.
After fixing the variable shadowing, update this line to use the new variable name:
- process = Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env)
+ process = Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=shell_env)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process = Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | |
| process = Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=shell_env) |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 84-84: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/common/utils.py at line 84, the environment
variable passed to Popen is still using the old variable name that caused
shadowing. Update the env parameter to use the new variable name that was
introduced to avoid shadowing, ensuring the correct environment dictionary is
passed to the subprocess.
| if self.adb_path == "adb": | ||
| self.adb_path = shutil.which("adb") | ||
| if not self.adb_path: | ||
| raise ConfigurationError(f"ADB executable '{self.adb_path}' not found in PATH.") | ||
|
|
There was a problem hiding this comment.
Fix incorrect error message variable.
The error message on line 97 will always show "adb" instead of the actual path that was not found. The original self.adb_path was already overwritten on line 95.
if self.adb_path == "adb":
- self.adb_path = shutil.which("adb")
- if not self.adb_path:
- raise ConfigurationError(f"ADB executable '{self.adb_path}' not found in PATH.")
+ adb_path = shutil.which("adb")
+ if not adb_path:
+ raise ConfigurationError("ADB executable 'adb' not found in PATH.")
+ self.adb_path = adb_path🤖 Prompt for AI Agents
In packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py
around lines 94 to 98, the error message uses self.adb_path after it has been
overwritten by None when the adb executable is not found, causing the message to
incorrectly show 'None' instead of the intended 'adb'. To fix this, store the
original value of self.adb_path before overwriting it, and use that original
value in the error message to accurately indicate which executable was not
found.
| except Exception as e: | ||
| self.logger.error(f"Unexpected error while stopping ADB server: {e}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching broad Exception.
Catching the generic Exception class can hide unexpected errors and make debugging difficult. Consider catching more specific exceptions or re-raising after logging.
- except Exception as e:
- self.logger.error(f"Unexpected error while stopping ADB server: {e}")
+ except (OSError, ValueError) as e:
+ self.logger.error(f"Unexpected error while stopping ADB server: {e}")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| self.logger.error(f"Unexpected error while stopping ADB server: {e}") | |
| except (OSError, ValueError) as e: | |
| self.logger.error(f"Unexpected error while stopping ADB server: {e}") | |
| raise |
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 78-78: Catching too general exception Exception
(W0718)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/adb.py
around lines 78 to 79, avoid catching the broad Exception class as it can
obscure unexpected errors. Identify and catch more specific exceptions related
to stopping the ADB server, such as subprocess or OS-related exceptions. After
logging the error, re-raise the exception to ensure it is not silently
swallowed, aiding in debugging and error handling.
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/options.py
Show resolved
Hide resolved
| @export | ||
| def on(self) -> None: | ||
| if self._process is not None: | ||
| self.logger.warning("Android emulator is already powered on, ignoring request.") | ||
| return | ||
|
|
||
| # Create the emulator command line options | ||
| cmdline = self._make_emulator_command() | ||
|
|
||
| # Prepare environment variables | ||
| env = dict(os.environ) | ||
| env.update(self.parent.emulator.env) | ||
|
|
||
| # Set the ADB server address and port | ||
| env["ANDROID_ADB_SERVER_PORT"] = str(self.parent.adb.port) | ||
| env["ANDROID_ADB_SERVER_ADDRESS"] = self.parent.adb.host | ||
|
|
||
| self.logger.info(f"Starting Android emulator with command: {' '.join(cmdline)}") | ||
| self._process = subprocess.Popen( | ||
| cmdline, | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| universal_newlines=False, # Keep as bytes for proper encoding handling | ||
| env=env, | ||
| ) | ||
|
|
||
| # Process logs in separate threads | ||
| self._log_thread = threading.Thread(target=self._process_logs, args=(self._process.stdout,), daemon=True) | ||
| self._stderr_thread = threading.Thread( | ||
| target=self._process_logs, args=(self._process.stderr, True), daemon=True | ||
| ) | ||
| self._log_thread.start() | ||
| self._stderr_thread.start() | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add docstring and ensure thread safety for concurrent calls.
The power-on implementation is correct, but needs documentation and thread safety consideration.
Add a docstring and consider thread safety:
@export
-def on(self) -> None:
+async def on(self) -> None:
+ """
+ Power on the Android emulator by starting the emulator process.
+
+ Starts the emulator subprocess and spawns daemon threads to process
+ stdout and stderr logs. If the emulator is already running, logs a
+ warning and returns without action.
+ """Note: The method signature shows def on() but the PowerInterface abstract method declares async def on(). This should be an async method to match the interface.
🌐 Web query:
Does Python allow a non-async method to implement an abstract async method in an ABC?
💡 Result:
Can a Non-Async Method Implement an Abstract Async Method in Python ABCs?
Summary:
Python allows you to define abstract async methods in Abstract Base Classes (ABCs) using the @abstractmethod decorator. While concrete subclasses can technically implement these methods as either async or non-async methods, this leads to inconsistent and potentially problematic behavior.
Practical Behavior in Python
-
If you declare an abstract async method in an ABC:
from abc import ABC, abstractmethod class MyABC(ABC): @abstractmethod async def foo(self): pass
-
You can technically implement it in a subclass as a regular (non-async) method:
class NotReallyAsync(MyABC): def foo(self): # not async! print("Not actually async")
-
Python will NOT raise a TypeError at subclass definition or instantiation time just because you used a non-async method to implement an abstract async one. The ABC mechanism simply checks that a method with the expected name exists, not its async-ness or signature[2].
Consequences
- If your base class expects
footo be a coroutine (i.e., an async function), and you override it with a regular function, code that awaits the method (e.g.,await instance.foo()) will fail at runtime with aTypeError[2]. - This violates interface expectations and can break code depending on async behavior. It defeats a major purpose of ABCs: guaranteeing a consistent interface (see also the Liskov Substitution Principle)[2].
"This seems to defeat much of the purpose of using an abstract base class in the first place as it's kind of a violation of the Liskov Substitution Principle."[2]
Static Type Checkers
- Tools like
mypyandpyrightdo check for signature compatibility, including whether a method is async or not, and will warn you if a non-async method implements an abstract async method[1]. However, these are not enforced at runtime.
Best Practice
- You should always implement an abstract async method using an
async defin concrete subclasses. - If you don’t, users of your class cannot safely rely on the method being awaitable, leading to runtime errors.
Table: Implementation and Behavior
| Abstract ABC Declares | Subclass Implementation | Result |
|---|---|---|
@abstractmethod async def |
async def (coroutine) |
Correct, fully compatible |
@abstractmethod async def |
def (regular function) |
No error at definition time, but calls to await will crash at runtime |
References
- Defining and overriding abstract async methods: behavior and caveats[2].
- Static typing tools catch this mismatch; Python runtime does not[1].
In summary:
Python’s ABC mechanism does not enforce whether an overriding method is async or not. You can implement an abstract async method with a non-async method, but you shouldn’t—runtime errors will result if the method is awaited[2][1]. Always match async in concrete implementations of async abstract methods.
Citations:
- 1: How to properly type class abstract async methods and override them in child classes microsoft/pyright#4741
- 2: https://www.codependentcodr.com/asyncio-part-2-now-more-abstract.html
- 3: https://docs.python.org/3/library/collections.abc.html
- 4: https://peps.python.org/pep-0492/
- 5: http://docs.aiohttp.org/en/stable/abc.html
- 6: https://discuss.python.org/t/is-there-a-way-to-define-a-function-that-can-be-used-as-a-nomal-function-and-an-async-function/5262
- 7: https://dev.to/leapcell/elegant-abstractions-mastering-abcs-in-advanced-python-4k6d
- 8: https://labex.io/tutorials/python-how-to-use-abstract-base-classes-in-python-programming-398077
Ensure on matches the async interface, is documented, and is safe under concurrent calls
Python’s ABC mechanism won’t enforce async-ness at runtime, but callers who await instance.on() will get a TypeError if on isn’t a coroutine. To align with PowerInterface.async def on(), plus improve clarity and guard against races:
• Update the signature to async def on(self) -> None.
• Add a descriptive docstring.
• Protect the “check then start” logic with an asyncio.Lock (or other sync primitive) so simultaneous calls don’t spawn multiple emulator processes.
Suggested diff:
@export
-def on(self) -> None:
+async def on(self) -> None:
+ """
+ Power on the Android emulator by starting the emulator process.
+
+ Starts the emulator subprocess and spawns daemon threads to process
+ stdout and stderr logs. If the emulator is already running, logs a
+ warning and returns without action.
+ """
+ # Ensure only one coroutine enters this block at a time
+ async with self._on_lock:
+ if self._process is not None:
+ self.logger.warning("Android emulator is already powered on, ignoring request.")
+ return
# Create the emulator command line options
cmdline = self._make_emulator_command()(Be sure to initialize self._on_lock = asyncio.Lock() in the constructor.)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 298-298: Line too long (113/100)
(C0301)
[convention] 271-271: Missing function or method docstring
(C0116)
[refactor] 288-295: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In
packages/jumpstarter-driver-android/jumpstarter_driver_android/driver/emulator.py
lines 270 to 304, the on() method must be updated to match the async abstract
method signature by changing it to async def on(self) -> None. Add a clear
docstring explaining its purpose. To ensure thread safety and prevent concurrent
calls from starting multiple emulator processes, protect the critical section
that checks and starts the process with an asyncio.Lock, which should be
initialized in the constructor as self._on_lock = asyncio.Lock(). Use async with
self._on_lock to guard the logic inside on().
| # Forward the ADB server address and port and call scrcpy executable with args | ||
| with self.forward_adb(host, port) as addr: | ||
| # Scrcpy uses ADB_SERVER_SOCKET environment variable | ||
| socket_path = f"tcp:{addr[0]}:{addr[1]}" | ||
| env = os.environ | { | ||
| "ADB_SERVER_SOCKET": socket_path, | ||
| } | ||
| cmd = [scrcpy, *args] | ||
| process = subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | ||
| return process.wait() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper subprocess management for resource cleanup.
Similar to the AdbClient, the subprocess should be properly managed.
# Forward the ADB server address and port and call scrcpy executable with args
with self.forward_adb(host, port) as addr:
# Scrcpy uses ADB_SERVER_SOCKET environment variable
socket_path = f"tcp:{addr[0]}:{addr[1]}"
env = os.environ | {
"ADB_SERVER_SOCKET": socket_path,
}
cmd = [scrcpy, *args]
- process = subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env)
- return process.wait()
+ with subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) as process:
+ return process.wait()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Forward the ADB server address and port and call scrcpy executable with args | |
| with self.forward_adb(host, port) as addr: | |
| # Scrcpy uses ADB_SERVER_SOCKET environment variable | |
| socket_path = f"tcp:{addr[0]}:{addr[1]}" | |
| env = os.environ | { | |
| "ADB_SERVER_SOCKET": socket_path, | |
| } | |
| cmd = [scrcpy, *args] | |
| process = subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | |
| return process.wait() | |
| # Forward the ADB server address and port and call scrcpy executable with args | |
| with self.forward_adb(host, port) as addr: | |
| # Scrcpy uses ADB_SERVER_SOCKET environment variable | |
| socket_path = f"tcp:{addr[0]}:{addr[1]}" | |
| env = os.environ | { | |
| "ADB_SERVER_SOCKET": socket_path, | |
| } | |
| cmd = [scrcpy, *args] | |
| with subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) as process: | |
| return process.wait() |
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 232-232: Line too long (111/100)
(C0301)
[refactor] 232-232: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-android/jumpstarter_driver_android/client.py
around lines 224 to 234, the subprocess.Popen call is not properly managed,
which can lead to resource leaks. Replace the subprocess.Popen and
process.wait() with a subprocess.run call using the same command, stdin, stdout,
stderr, and env parameters to ensure the subprocess is properly waited on and
resources are cleaned up automatically.
| # Forward the ADB server address and port and call ADB executable with args | ||
| with self.forward_adb(host, port) as addr: | ||
| env = os.environ | { | ||
| "ANDROID_ADB_SERVER_ADDRESS": addr[0], | ||
| "ANDROID_ADB_SERVER_PORT": str(addr[1]), | ||
| } | ||
| cmd = [adb, *args] | ||
| process = subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | ||
| return process.wait() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper subprocess management for resource cleanup.
The subprocess should be managed properly to ensure cleanup even if an exception occurs.
# Forward the ADB server address and port and call ADB executable with args
with self.forward_adb(host, port) as addr:
env = os.environ | {
"ANDROID_ADB_SERVER_ADDRESS": addr[0],
"ANDROID_ADB_SERVER_PORT": str(addr[1]),
}
cmd = [adb, *args]
- process = subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env)
- return process.wait()
+ with subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) as process:
+ return process.wait()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Forward the ADB server address and port and call ADB executable with args | |
| with self.forward_adb(host, port) as addr: | |
| env = os.environ | { | |
| "ANDROID_ADB_SERVER_ADDRESS": addr[0], | |
| "ANDROID_ADB_SERVER_PORT": str(addr[1]), | |
| } | |
| cmd = [adb, *args] | |
| process = subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) | |
| return process.wait() | |
| # Forward the ADB server address and port and call ADB executable with args | |
| with self.forward_adb(host, port) as addr: | |
| env = os.environ | { | |
| "ANDROID_ADB_SERVER_ADDRESS": addr[0], | |
| "ANDROID_ADB_SERVER_PORT": str(addr[1]), | |
| } | |
| cmd = [adb, *args] | |
| with subprocess.Popen(cmd, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr, env=env) as process: | |
| return process.wait() |
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 169-169: Line too long (111/100)
(C0301)
[refactor] 169-169: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-android/jumpstarter_driver_android/client.py
around lines 162 to 171, the subprocess.Popen call is not managed with a context
that ensures proper cleanup on exceptions. Replace the current subprocess.Popen
usage with a context manager (using subprocess.Popen as a context) or ensure the
process is properly terminated and waited on in a finally block to guarantee
resource cleanup even if an error occurs.
kirkbrauer
force-pushed
the
driver_android
branch
from
714507b to
3e90beb
Compare
|
|
||
| ### Virtualization Drivers | ||
|
|
||
| Drivers for running virtual machines and systems: |
There was a problem hiding this comment.
This should be a symlink to packages/jumpstarter-driver-android/README.md
| create an ADB client and forward the ADB server address and port. | ||
| """ | ||
|
|
||
| def _check_port_in_use(self, host: str, port: int) -> bool: |
There was a problem hiding this comment.
This should also be removed after some experimentation, not reliable across platforms.
| def forward_adb(self, host: str, port: int) -> Generator[str, None, None]: | ||
| """ | ||
| Port-forward remote ADB server to local host and port. | ||
| If the port is already bound, yields the existing address instead. |
There was a problem hiding this comment.
Yeah, this was an attempt to fix port issues, should be removed.
| ] | ||
|
|
||
| for arg in args: | ||
| for unsupported in unsupported_args: |
There was a problem hiding this comment.
Or print a warning and ignore the argument? Then the same scrcpy invocation can be used with or without jumpstarter.
There was a problem hiding this comment.
These arguments don't really work when using remote scrcpy which itself is also an adb wrapper 🙄. I can switch this to warnings instead.
| if not isinstance(self.port, int): | ||
| raise ConfigurationError(f"Port must be an integer: {self.port}") | ||
|
|
||
| if self.port < 0 or self.port > 65535: |
There was a problem hiding this comment.
These validation logic can be handled by pydantic, by replacing the builtin dataclass implementation with the pydantic one: https://docs.pydantic.dev/latest/concepts/dataclasses/
| disable_scrcpy: bool = field(default=False) | ||
| disable_adb: bool = field(default=False) |
There was a problem hiding this comment.
What about making them enable options and True by default? Double negative can get confusing.
There was a problem hiding this comment.
Agreed, will change this, maybe we should disable scrcpy by default, it can cause performance impacts on some virtual targets which is not idea. We should really be using VNC through QEMU by default, but I did not have time to implement this.
|
@mangelajo I'm not sure if we should shelve this in favor of a Cuttlefish-based driver or try to merge what we have now. I don't really want to include the entire Android SDK in a container image, so maybe it's best to wait for Cuttlefish. |
I see you concern, I don't know, even with the known limitations, should we merge it anyway? , then we can make a better version later in time with cuttlefish? It was awesome work, would probably attract people to jumpstarter IMHO! |
This PR includes a full implementation of an Android Driver providing emulation through the Android emulator CLI, debugging through
adb, and remote screen access through thescrcpytool.Drivers
AndroidEmulator- Top-level driver for running Android emulators.AndroidEmulatorPower- Power driver implementation for stopping/starting android emulators.AdbServer- ADB server implementation to start/stop the ADB daemon in the background and port-forward ADB over Jumpstarter.Scrcpy- Scrcpy driver based on theAdbServerto provide a "stub" for the Scrcpy client to connect to.Clients
AndroidClient- Composite client for Android devices/emulators.AdbClient- Python and CLI ADB client that provides a thin wrapper around theadbcommand with automatic port-forwarding and supports theadbutilsPython package for pure Python implementation.ScrcpyClient- CLI Scrcpy client that provides a thin wrapper aroundscrcpyto enable automatic port-forwarding on the client side. Requires thescrcpyCLI tool.Usage
Example
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores