-
Notifications
You must be signed in to change notification settings - Fork 80
add type annotations and docstrings to devlib #715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,7 @@ devlib/bin/scripts/shutils | |
doc/_build/ | ||
build/ | ||
dist/ | ||
.venv/ | ||
.vscode/ | ||
venv/ | ||
.history/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
# Copyright 2024 ARM Limited | ||
# Copyright 2024-2025 ARM Limited | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
|
@@ -17,15 +17,24 @@ | |
Target runner and related classes are implemented here. | ||
""" | ||
|
||
import logging | ||
import os | ||
import time | ||
|
||
from platform import machine | ||
from typing import Optional, cast, Protocol, TYPE_CHECKING, Union | ||
from typing_extensions import NotRequired, LiteralString, TypedDict | ||
from devlib.platform import Platform | ||
if TYPE_CHECKING: | ||
from _typeshed import StrPath, BytesPath | ||
else: | ||
StrPath = str | ||
BytesPath = bytes | ||
|
||
from devlib.exception import (TargetStableError, HostError) | ||
from devlib.target import LinuxTarget | ||
from devlib.utils.misc import get_subprocess, which | ||
from devlib.target import LinuxTarget, Target | ||
from devlib.utils.misc import get_subprocess, which, get_logger | ||
from devlib.utils.ssh import SshConnection | ||
from devlib.utils.annotation_helpers import SubprocessCommand, SshUserConnectionSettings | ||
|
||
|
||
class TargetRunner: | ||
|
@@ -36,16 +45,14 @@ class TargetRunner: | |
(e.g., :class:`QEMUTargetRunner`). | ||
|
||
:param target: Specifies type of target per :class:`Target` based classes. | ||
:type target: Target | ||
""" | ||
|
||
def __init__(self, | ||
target): | ||
target: Target) -> None: | ||
self.target = target | ||
self.logger = get_logger(self.__class__.__name__) | ||
|
||
self.logger = logging.getLogger(self.__class__.__name__) | ||
|
||
def __enter__(self): | ||
def __enter__(self) -> 'TargetRunner': | ||
return self | ||
|
||
def __exit__(self, *_): | ||
|
@@ -58,29 +65,25 @@ class SubprocessTargetRunner(TargetRunner): | |
|
||
:param runner_cmd: The command to start runner process (e.g., | ||
``qemu-system-aarch64 -kernel Image -append "console=ttyAMA0" ...``). | ||
:type runner_cmd: list(str) | ||
|
||
:param target: Specifies type of target per :class:`Target` based classes. | ||
:type target: Target | ||
|
||
:param connect: Specifies if :class:`TargetRunner` should try to connect | ||
target after launching it, defaults to True. | ||
:type connect: bool or None | ||
|
||
:param boot_timeout: Timeout for target's being ready for SSH access in | ||
seconds, defaults to 60. | ||
:type boot_timeout: int or None | ||
|
||
:raises HostError: if it cannot execute runner command successfully. | ||
|
||
:raises TargetStableError: if Target is inaccessible. | ||
""" | ||
|
||
def __init__(self, | ||
runner_cmd, | ||
target, | ||
connect=True, | ||
boot_timeout=60): | ||
runner_cmd: SubprocessCommand, | ||
target: Target, | ||
connect: bool = True, | ||
boot_timeout: int = 60): | ||
super().__init__(target=target) | ||
|
||
self.boot_timeout = boot_timeout | ||
|
@@ -90,7 +93,7 @@ def __init__(self, | |
try: | ||
self.runner_process = get_subprocess(runner_cmd) | ||
except Exception as ex: | ||
raise HostError(f'Error while running "{runner_cmd}": {ex}') from ex | ||
raise HostError(f'Error while running "{runner_cmd!r}": {ex}') from ex | ||
|
||
if connect: | ||
self.wait_boot_complete() | ||
|
@@ -107,16 +110,16 @@ def __exit__(self, *_): | |
|
||
self.terminate() | ||
|
||
def wait_boot_complete(self): | ||
def wait_boot_complete(self) -> None: | ||
""" | ||
Wait for target OS to finish boot up and become accessible over SSH in at most | ||
``SubprocessTargetRunner.boot_timeout`` seconds. | ||
Wait for the target OS to finish booting and become accessible within | ||
:attr:`boot_timeout` seconds. | ||
|
||
:raises TargetStableError: In case of timeout. | ||
:raises TargetStableError: If the target is inaccessible after the timeout. | ||
""" | ||
|
||
start_time = time.time() | ||
elapsed = 0 | ||
elapsed: float = 0.0 | ||
while self.boot_timeout >= elapsed: | ||
try: | ||
self.target.connect(timeout=self.boot_timeout - elapsed) | ||
|
@@ -132,9 +135,9 @@ def wait_boot_complete(self): | |
self.terminate() | ||
raise TargetStableError(f'Target is inaccessible for {self.boot_timeout} seconds!') | ||
|
||
def terminate(self): | ||
def terminate(self) -> None: | ||
""" | ||
Terminate ``SubprocessTargetRunner.runner_process``. | ||
Terminate the subprocess associated with this runner. | ||
""" | ||
|
||
self.logger.debug('Killing target runner...') | ||
|
@@ -147,10 +150,9 @@ class NOPTargetRunner(TargetRunner): | |
Class for implementing a target runner which does nothing except providing .target attribute. | ||
|
||
:param target: Specifies type of target per :class:`Target` based classes. | ||
:type target: Target | ||
""" | ||
|
||
def __init__(self, target): | ||
def __init__(self, target: Target) -> None: | ||
super().__init__(target=target) | ||
|
||
def __enter__(self): | ||
|
@@ -159,11 +161,63 @@ def __enter__(self): | |
def __exit__(self, *_): | ||
pass | ||
|
||
def terminate(self): | ||
def terminate(self) -> None: | ||
""" | ||
Nothing to terminate for NOP target runners. | ||
Defined to be compliant with other runners (e.g., ``SubprocessTargetRunner``). | ||
""" | ||
pass | ||
|
||
|
||
class QEMUTargetUserSettings(TypedDict, total=False): | ||
kernel_image: str | ||
arch: NotRequired[str] | ||
cpu_type: NotRequired[str] | ||
initrd_image: str | ||
mem_size: NotRequired[int] | ||
num_cores: NotRequired[int] | ||
num_threads: NotRequired[int] | ||
cmdline: NotRequired[str] | ||
enable_kvm: NotRequired[bool] | ||
|
||
|
||
class QEMUTargetRunnerSettings(TypedDict): | ||
kernel_image: str | ||
arch: str | ||
cpu_type: str | ||
initrd_image: str | ||
mem_size: int | ||
num_cores: int | ||
num_threads: int | ||
cmdline: str | ||
enable_kvm: bool | ||
|
||
|
||
# TODO - look into which params can be made NotRequired and Optional | ||
# TODO - use pydantic for dynamic type checking | ||
class SshConnectionSettings(TypedDict): | ||
username: str | ||
password: str | ||
keyfile: Optional[Union[LiteralString, StrPath, BytesPath]] | ||
host: str | ||
port: int | ||
timeout: float | ||
platform: 'Platform' | ||
sudo_cmd: str | ||
strict_host_check: bool | ||
use_scp: bool | ||
poll_transfers: bool | ||
start_transfer_poll_delay: int | ||
total_transfer_timeout: int | ||
transfer_poll_period: int | ||
|
||
|
||
class QEMUTargetRunnerTargetFactory(Protocol): | ||
""" | ||
Protocol for Lambda function for creating :class:`Target` based object. | ||
""" | ||
def __call__(self, *, connect: bool, conn_cls, connection_settings: SshConnectionSettings) -> Target: | ||
... | ||
|
||
|
||
class QEMUTargetRunner(SubprocessTargetRunner): | ||
|
@@ -177,7 +231,7 @@ class QEMUTargetRunner(SubprocessTargetRunner): | |
|
||
* ``arch``: Architecture type. Defaults to ``aarch64``. | ||
|
||
* ``cpu_types``: List of CPU ids for QEMU. The list only contains ``cortex-a72`` by | ||
* ``cpu_type``: List of CPU ids for QEMU. The list only contains ``cortex-a72`` by | ||
default. This parameter is valid for Arm architectures only. | ||
|
||
* ``initrd_image``: This points to the location of initrd image (e.g., | ||
|
@@ -197,36 +251,37 @@ class QEMUTargetRunner(SubprocessTargetRunner): | |
* ``enable_kvm``: Specifies if KVM will be used as accelerator in QEMU or not. | ||
Enabled by default if host architecture matches with target's for improving | ||
QEMU performance. | ||
:type qemu_settings: Dict | ||
|
||
:param connection_settings: the dictionary to store connection settings | ||
of ``Target.connection_settings``, defaults to None. | ||
:type connection_settings: Dict or None | ||
|
||
:param make_target: Lambda function for creating :class:`Target` based object. | ||
:type make_target: func or None | ||
|
||
:Variable positional arguments: Forwarded to :class:`TargetRunner`. | ||
|
||
:raises FileNotFoundError: if QEMU executable, kernel or initrd image cannot be found. | ||
""" | ||
|
||
def __init__(self, | ||
qemu_settings, | ||
connection_settings=None, | ||
make_target=LinuxTarget, | ||
**args): | ||
qemu_settings: QEMUTargetUserSettings, | ||
connection_settings: Optional[SshUserConnectionSettings] = None, | ||
make_target: QEMUTargetRunnerTargetFactory = cast(QEMUTargetRunnerTargetFactory, LinuxTarget), | ||
**kwargs) -> None: | ||
|
||
self.connection_settings = { | ||
default_connection_settings = { | ||
'host': '127.0.0.1', | ||
'port': 8022, | ||
'username': 'root', | ||
'password': 'root', | ||
'strict_host_check': False, | ||
} | ||
self.connection_settings = {**self.connection_settings, **(connection_settings or {})} | ||
# TODO - use pydantic for dynamic type checking. that can avoid casting and ensure runtime type compatibility | ||
self.connection_settings: SshConnectionSettings = cast(SshConnectionSettings, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit disappointing from mypy that it cannot type-check this without an explicit cast(). If we are going to have a lot of that, it could make sense to depend on pydantic and at least provide a runtime check: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried using pydantic at this point. But then, the issue is that 'Platform' field is defined only for TYPE_CHECKING. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The usual way to deal with those recursive imports is to move them closer to where they are used, so typically inside the function. It's not pretty but it usually works. The overhead is negligible as modules are de-facto singletons so re-executing an import statement multiple times will import the first time and then just check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in this case, though the one we get the issue for is part of the SshConnectionSettings TypedDict. we need th 'Platform' to be used as the type for platform attribute. and the SshConnection type is further used in the file even outside functions. so how do we handle this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case you can probably use a string as annotation like in what you just shared. Tooling should be able to deal with that, there even was a PEP that got accepted (then last minute reverted) to make that the default even when string syntax is not used, precisely to speed up import times and avoid circular imports. Starting from 3.14, that issue should probably be fixed by https://peps.python.org/pep-0649/ and https://peps.python.org/pep-0749/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, i have currently put the import only under if TYPE_CHECKING case, and used the string 'Platform' as the annotation. But if we want to use this SshConnectionSettings type with pydantic, we would need to have the import outside of the if TYPE_CHECKING case. That would cause the circular import issue. We are still using older python version, and even to come to 3.10, as we are waiting on consensus, should we defer using pydantic to a later time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I lost track of the fact we were discussing about pydantic, so facilities available to static type checkers are not really relevant here. What is the import cycle involved ? It seems odd as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i remember facing some import errors when i tried it before, but now i dont seem to get that. I went ahead to try and use pydantic, but it gives different kinds of errors like "pydantic.errors.PydanticSchemaGenerationError:" unable to generate core schema. i am trying to debug these, but it takes some time. can we consider using pydantic as an exercise for the future? and for now just use the cast? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, just leave a #TODO mentioning pydantic for now so we can find the spots easily in the future. Maybe also open a github issue stating that it would be a possible improvement and to look for the TODOs in question so we don't forget about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated a TODO comment mentioning pydantic and also raised issue #721 to keep track of this later. Also pulled out the import of Platform from under TYPE_CHECKING flag as we discovered in the above experiment that it is not really causing any recursive imports issue. |
||
**default_connection_settings, | ||
**(connection_settings or {}) | ||
}) | ||
|
||
qemu_args = { | ||
qemu_default_args = { | ||
'arch': 'aarch64', | ||
'cpu_type': 'cortex-a72', | ||
'mem_size': 512, | ||
|
@@ -235,7 +290,8 @@ def __init__(self, | |
'cmdline': 'console=ttyAMA0', | ||
'enable_kvm': True, | ||
} | ||
qemu_args = {**qemu_args, **qemu_settings} | ||
# TODO - same as above, use pydantic. | ||
qemu_args: QEMUTargetRunnerSettings = cast(QEMUTargetRunnerSettings, {**qemu_default_args, **qemu_settings}) | ||
arjpur01 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
qemu_executable = f'qemu-system-{qemu_args["arch"]}' | ||
qemu_path = which(qemu_executable) | ||
|
@@ -281,4 +337,4 @@ def __init__(self, | |
|
||
super().__init__(runner_cmd=qemu_cmd, | ||
target=target, | ||
**args) | ||
**kwargs) |
Uh oh!
There was an error while loading. Please reload this page.