E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
returns strings, or `text=False` where it returns bytes.
To differentiate, you can annotate types as `Popen[str]` or
`Popen[bytes]`.
This patch should add subscripts in all the places we were missing them.
Reported-by: Johannes Berg <[email protected]>
Link: https://lore.kernel.org/linux-kselftest/20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid/
Signed-off-by: Daniel Latypov <[email protected]>
---
Note: this is unchanged, just added a 3rd patch to this series.
---
tools/testing/kunit/kunit_kernel.py | 6 +++---
tools/testing/kunit/kunit_printer.py | 2 +-
tools/testing/kunit/run_checks.py | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 53e90c335834..e6fc8fcb071a 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -92,7 +92,7 @@ class LinuxSourceTreeOperations:
if stderr: # likely only due to build warnings
print(stderr.decode())
- def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
+ def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
raise RuntimeError('not implemented!')
@@ -112,7 +112,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
kconfig.merge_in_entries(base_kunitconfig)
return kconfig
- def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
+ def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
kernel_path = os.path.join(build_dir, self._kernel_path)
qemu_command = ['qemu-system-' + self._qemu_arch,
'-nodefaults',
@@ -141,7 +141,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
kconfig.merge_in_entries(base_kunitconfig)
return kconfig
- def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
+ def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
"""Runs the Linux UML binary. Must be named 'linux'."""
linux_bin = os.path.join(build_dir, 'linux')
params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
diff --git a/tools/testing/kunit/kunit_printer.py b/tools/testing/kunit/kunit_printer.py
index 5f1cc55ecdf5..015adf87dc2c 100644
--- a/tools/testing/kunit/kunit_printer.py
+++ b/tools/testing/kunit/kunit_printer.py
@@ -15,7 +15,7 @@ _RESET = '\033[0;0m'
class Printer:
"""Wraps a file object, providing utilities for coloring output, etc."""
- def __init__(self, output: typing.IO):
+ def __init__(self, output: typing.IO[str]):
self._output = output
self._use_color = output.isatty()
diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
index 066e6f938f6d..61cece1684df 100755
--- a/tools/testing/kunit/run_checks.py
+++ b/tools/testing/kunit/run_checks.py
@@ -37,7 +37,7 @@ def main(argv: Sequence[str]) -> None:
if argv:
raise RuntimeError('This script takes no arguments')
- future_to_name: Dict[futures.Future, str] = {}
+ future_to_name: Dict[futures.Future[None], str] = {}
executor = futures.ThreadPoolExecutor(max_workers=len(commands))
for name, argv in commands.items():
if name in necessary_deps and shutil.which(necessary_deps[name]) is None:
base-commit: 2c6a96dad5797e57b4cf04101d6c8d5c7a571603
--
2.40.0.rc1.284.g88254d51c5-goog
We don't run a linter regularly over kunit.py code (the default settings
on most don't like kernel style, e.g. tabs) so some of these imports
didn't get removed when they stopped being used.
Signed-off-by: Daniel Latypov <[email protected]>
---
Note: this is unchanged, just added a 3rd patch to this series.
---
tools/testing/kunit/kunit.py | 2 +-
tools/testing/kunit/kunit_config.py | 2 +-
tools/testing/kunit/kunit_kernel.py | 1 -
tools/testing/kunit/kunit_parser.py | 1 -
tools/testing/kunit/kunit_tool_test.py | 2 +-
5 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 741f15420467..52853634ba23 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -123,7 +123,7 @@ def _suites_from_test_list(tests: List[str]) -> List[str]:
parts = t.split('.', maxsplit=2)
if len(parts) != 2:
raise ValueError(f'internal KUnit error, test name should be of the form "<suite>.<test>", got "{t}"')
- suite, case = parts
+ suite, _ = parts
if not suites or suites[-1] != suite:
suites.append(suite)
return suites
diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index 48b5f34b2e5d..9f76d7b89617 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -8,7 +8,7 @@
from dataclasses import dataclass
import re
-from typing import Dict, Iterable, List, Set, Tuple
+from typing import Dict, Iterable, List, Tuple
CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index e6fc8fcb071a..775842b912d8 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -18,7 +18,6 @@ import threading
from typing import Iterator, List, Optional, Tuple
import kunit_config
-from kunit_printer import stdout
import qemu_config
KCONFIG_PATH = '.config'
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index a225799f6b1b..fbc094f0567e 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -12,7 +12,6 @@
from __future__ import annotations
from dataclasses import dataclass
import re
-import sys
import textwrap
from enum import Enum, auto
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 0c2190514103..be35999bb84f 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -328,7 +328,7 @@ class KUnitParserTest(unittest.TestCase):
def test_parse_subtest_header(self):
ktap_log = test_data_path('test_parse_subtest_header.log')
with open(ktap_log) as file:
- result = kunit_parser.parse_run_tests(file.readlines())
+ kunit_parser.parse_run_tests(file.readlines())
self.print_mock.assert_any_call(StrContains('suite (1 subtest)'))
def test_show_test_output_on_failure(self):
--
2.40.0.rc1.284.g88254d51c5-goog
Basically, get this command to be happy and make run_checks.py happy
$ mypy --strict --exclude '_test.py$' --exclude qemu_configs/ ./tools/testing/kunit/
Primarily the changes are
* add `-> None` return type annotations
* add all the missing argument type annotations
Previously, we had false positives from mypy in `main()`, see commit
09641f7c7d8f ("kunit: tool: surface and address more typing issues").
But after commit 2dc9d6ca52a4 ("kunit: kunit.py extract handlers")
refactored things, the variable name reuse mypy hated is gone.
Note: mypy complains we don't annotate the types the unused args in our
signal handler. That's silly.
But to make it happy, I've copy-pasted an appropriate annotation from
https://github.com/python/typing/discussions/1042#discussioncomment-2013595.
Reported-by: Johannes Berg <[email protected]>
Link: https://lore.kernel.org/linux-kselftest/[email protected]/
Signed-off-by: Daniel Latypov <[email protected]>
---
tools/testing/kunit/kunit.py | 24 ++++++++++++------------
tools/testing/kunit/kunit_config.py | 4 ++--
tools/testing/kunit/kunit_kernel.py | 29 +++++++++++++++--------------
tools/testing/kunit/run_checks.py | 4 ++--
4 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 52853634ba23..3905c43369c3 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -269,7 +269,7 @@ def massage_argv(argv: Sequence[str]) -> Sequence[str]:
def get_default_jobs() -> int:
return len(os.sched_getaffinity(0))
-def add_common_opts(parser) -> None:
+def add_common_opts(parser: argparse.ArgumentParser) -> None:
parser.add_argument('--build_dir',
help='As in the make command, it specifies the build '
'directory.',
@@ -320,13 +320,13 @@ def add_common_opts(parser) -> None:
help='Additional QEMU arguments, e.g. "-smp 8"',
action='append', metavar='')
-def add_build_opts(parser) -> None:
+def add_build_opts(parser: argparse.ArgumentParser) -> None:
parser.add_argument('--jobs',
help='As in the make command, "Specifies the number of '
'jobs (commands) to run simultaneously."',
type=int, default=get_default_jobs(), metavar='N')
-def add_exec_opts(parser) -> None:
+def add_exec_opts(parser: argparse.ArgumentParser) -> None:
parser.add_argument('--timeout',
help='maximum number of seconds to allow for all tests '
'to run. This does not include time taken to build the '
@@ -351,7 +351,7 @@ def add_exec_opts(parser) -> None:
type=str,
choices=['suite', 'test'])
-def add_parse_opts(parser) -> None:
+def add_parse_opts(parser: argparse.ArgumentParser) -> None:
parser.add_argument('--raw_output', help='If set don\'t parse output from kernel. '
'By default, filters to just KUnit output. Use '
'--raw_output=all to show everything',
@@ -386,7 +386,7 @@ def tree_from_args(cli_args: argparse.Namespace) -> kunit_kernel.LinuxSourceTree
extra_qemu_args=qemu_args)
-def run_handler(cli_args):
+def run_handler(cli_args: argparse.Namespace) -> None:
if not os.path.exists(cli_args.build_dir):
os.mkdir(cli_args.build_dir)
@@ -405,7 +405,7 @@ def run_handler(cli_args):
sys.exit(1)
-def config_handler(cli_args):
+def config_handler(cli_args: argparse.Namespace) -> None:
if cli_args.build_dir and (
not os.path.exists(cli_args.build_dir)):
os.mkdir(cli_args.build_dir)
@@ -421,7 +421,7 @@ def config_handler(cli_args):
sys.exit(1)
-def build_handler(cli_args):
+def build_handler(cli_args: argparse.Namespace) -> None:
linux = tree_from_args(cli_args)
request = KunitBuildRequest(build_dir=cli_args.build_dir,
make_options=cli_args.make_options,
@@ -434,7 +434,7 @@ def build_handler(cli_args):
sys.exit(1)
-def exec_handler(cli_args):
+def exec_handler(cli_args: argparse.Namespace) -> None:
linux = tree_from_args(cli_args)
exec_request = KunitExecRequest(raw_output=cli_args.raw_output,
build_dir=cli_args.build_dir,
@@ -450,10 +450,10 @@ def exec_handler(cli_args):
sys.exit(1)
-def parse_handler(cli_args):
+def parse_handler(cli_args: argparse.Namespace) -> None:
if cli_args.file is None:
- sys.stdin.reconfigure(errors='backslashreplace') # pytype: disable=attribute-error
- kunit_output = sys.stdin
+ sys.stdin.reconfigure(errors='backslashreplace') # type: ignore
+ kunit_output = sys.stdin # type: Iterable[str]
else:
with open(cli_args.file, 'r', errors='backslashreplace') as f:
kunit_output = f.read().splitlines()
@@ -475,7 +475,7 @@ subcommand_handlers_map = {
}
-def main(argv):
+def main(argv: Sequence[str]) -> None:
parser = argparse.ArgumentParser(
description='Helps writing and running KUnit tests.')
subparser = parser.add_subparsers(dest='subcommand')
diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index 9f76d7b89617..eb5dd01210b1 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -8,7 +8,7 @@
from dataclasses import dataclass
import re
-from typing import Dict, Iterable, List, Tuple
+from typing import Any, Dict, Iterable, List, Tuple
CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'
@@ -34,7 +34,7 @@ class Kconfig:
def __init__(self) -> None:
self._entries = {} # type: Dict[str, str]
- def __eq__(self, other) -> bool:
+ def __eq__(self, other: Any) -> bool:
if not isinstance(other, self.__class__):
return False
return self._entries == other._entries
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 775842b912d8..a3321a991f11 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -16,6 +16,7 @@ import shutil
import signal
import threading
from typing import Iterator, List, Optional, Tuple
+from types import FrameType
import kunit_config
import qemu_config
@@ -56,7 +57,7 @@ class LinuxSourceTreeOperations:
def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> kunit_config.Kconfig:
return base_kunitconfig
- def make_olddefconfig(self, build_dir: str, make_options) -> None:
+ def make_olddefconfig(self, build_dir: str, make_options: Optional[List[str]]) -> None:
command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, 'olddefconfig']
if self._cross_compile:
command += ['CROSS_COMPILE=' + self._cross_compile]
@@ -70,7 +71,7 @@ class LinuxSourceTreeOperations:
except subprocess.CalledProcessError as e:
raise ConfigError(e.output.decode())
- def make(self, jobs, build_dir: str, make_options) -> None:
+ def make(self, jobs: int, build_dir: str, make_options: Optional[List[str]]) -> None:
command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, '--jobs=' + str(jobs)]
if make_options:
command.extend(make_options)
@@ -132,7 +133,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
"""An abstraction over command line operations performed on a source tree."""
- def __init__(self, cross_compile=None):
+ def __init__(self, cross_compile: Optional[str]=None):
super().__init__(linux_arch='um', cross_compile=cross_compile)
def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> kunit_config.Kconfig:
@@ -215,7 +216,7 @@ def _get_qemu_ops(config_path: str,
if not hasattr(config, 'QEMU_ARCH'):
raise ValueError('qemu_config module missing "QEMU_ARCH": ' + config_path)
- params: qemu_config.QemuArchParams = config.QEMU_ARCH # type: ignore
+ params: qemu_config.QemuArchParams = config.QEMU_ARCH
if extra_qemu_args:
params.extra_qemu_params.extend(extra_qemu_args)
return params.linux_arch, LinuxSourceTreeOperationsQemu(
@@ -229,10 +230,10 @@ class LinuxSourceTree:
build_dir: str,
kunitconfig_paths: Optional[List[str]]=None,
kconfig_add: Optional[List[str]]=None,
- arch=None,
- cross_compile=None,
- qemu_config_path=None,
- extra_qemu_args=None) -> None:
+ arch: Optional[str]=None,
+ cross_compile: Optional[str]=None,
+ qemu_config_path: Optional[str]=None,
+ extra_qemu_args: Optional[List[str]]=None) -> None:
signal.signal(signal.SIGINT, self.signal_handler)
if qemu_config_path:
self._arch, self._ops = _get_qemu_ops(qemu_config_path, extra_qemu_args, cross_compile)
@@ -275,7 +276,7 @@ class LinuxSourceTree:
logging.error(message)
return False
- def build_config(self, build_dir: str, make_options) -> bool:
+ def build_config(self, build_dir: str, make_options: Optional[List[str]]) -> bool:
kconfig_path = get_kconfig_path(build_dir)
if build_dir and not os.path.exists(build_dir):
os.mkdir(build_dir)
@@ -303,7 +304,7 @@ class LinuxSourceTree:
old_kconfig = kunit_config.parse_file(old_path)
return old_kconfig != self._kconfig
- def build_reconfig(self, build_dir: str, make_options) -> bool:
+ def build_reconfig(self, build_dir: str, make_options: Optional[List[str]]) -> bool:
"""Creates a new .config if it is not a subset of the .kunitconfig."""
kconfig_path = get_kconfig_path(build_dir)
if not os.path.exists(kconfig_path):
@@ -319,7 +320,7 @@ class LinuxSourceTree:
os.remove(kconfig_path)
return self.build_config(build_dir, make_options)
- def build_kernel(self, jobs, build_dir: str, make_options) -> bool:
+ def build_kernel(self, jobs: int, build_dir: str, make_options: Optional[List[str]]) -> bool:
try:
self._ops.make_olddefconfig(build_dir, make_options)
self._ops.make(jobs, build_dir, make_options)
@@ -328,7 +329,7 @@ class LinuxSourceTree:
return False
return self.validate_config(build_dir)
- def run_kernel(self, args=None, build_dir='', filter_glob='', timeout=None) -> Iterator[str]:
+ def run_kernel(self, args: Optional[List[str]]=None, build_dir: str='', filter_glob: str='', timeout: Optional[int]=None) -> Iterator[str]:
if not args:
args = []
if filter_glob:
@@ -339,7 +340,7 @@ class LinuxSourceTree:
assert process.stdout is not None # tell mypy it's set
# Enforce the timeout in a background thread.
- def _wait_proc():
+ def _wait_proc() -> None:
try:
process.wait(timeout=timeout)
except Exception as e:
@@ -365,6 +366,6 @@ class LinuxSourceTree:
waiter.join()
subprocess.call(['stty', 'sane'])
- def signal_handler(self, unused_sig, unused_frame) -> None:
+ def signal_handler(self, unused_sig: int, unused_frame: Optional[FrameType]) -> None:
logging.error('Build interruption occurred. Cleaning console.')
subprocess.call(['stty', 'sane'])
diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
index 61cece1684df..8208c3b3135e 100755
--- a/tools/testing/kunit/run_checks.py
+++ b/tools/testing/kunit/run_checks.py
@@ -23,7 +23,7 @@ commands: Dict[str, Sequence[str]] = {
'kunit_tool_test.py': ['./kunit_tool_test.py'],
'kunit smoke test': ['./kunit.py', 'run', '--kunitconfig=lib/kunit', '--build_dir=kunit_run_checks'],
'pytype': ['/bin/sh', '-c', 'pytype *.py'],
- 'mypy': ['/bin/sh', '-c', 'mypy *.py'],
+ 'mypy': ['mypy', '--strict', '--exclude', '_test.py$', '--exclude', 'qemu_configs/', '.'],
}
# The user might not have mypy or pytype installed, skip them if so.
@@ -73,7 +73,7 @@ def main(argv: Sequence[str]) -> None:
sys.exit(1)
-def run_cmd(argv: Sequence[str]):
+def run_cmd(argv: Sequence[str]) -> None:
subprocess.check_output(argv, stderr=subprocess.STDOUT, cwd=ABS_TOOL_PATH, timeout=TIMEOUT)
--
2.40.0.rc1.284.g88254d51c5-goog
On Fri, 17 Mar 2023 at 06:06, Daniel Latypov <[email protected]> wrote:
>
> E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> returns strings, or `text=False` where it returns bytes.
> To differentiate, you can annotate types as `Popen[str]` or
> `Popen[bytes]`.
>
> This patch should add subscripts in all the places we were missing them.
>
> Reported-by: Johannes Berg <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid/
> Signed-off-by: Daniel Latypov <[email protected]>
> ---
> Note: this is unchanged, just added a 3rd patch to this series.
> ---
Looks good.
Reviewed-by: David Gow <[email protected]>
Thanks, Johannes and Daniel!
-- David
> tools/testing/kunit/kunit_kernel.py | 6 +++---
> tools/testing/kunit/kunit_printer.py | 2 +-
> tools/testing/kunit/run_checks.py | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 53e90c335834..e6fc8fcb071a 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -92,7 +92,7 @@ class LinuxSourceTreeOperations:
> if stderr: # likely only due to build warnings
> print(stderr.decode())
>
> - def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> raise RuntimeError('not implemented!')
>
>
> @@ -112,7 +112,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
> kconfig.merge_in_entries(base_kunitconfig)
> return kconfig
>
> - def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> kernel_path = os.path.join(build_dir, self._kernel_path)
> qemu_command = ['qemu-system-' + self._qemu_arch,
> '-nodefaults',
> @@ -141,7 +141,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> kconfig.merge_in_entries(base_kunitconfig)
> return kconfig
>
> - def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> """Runs the Linux UML binary. Must be named 'linux'."""
> linux_bin = os.path.join(build_dir, 'linux')
> params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
> diff --git a/tools/testing/kunit/kunit_printer.py b/tools/testing/kunit/kunit_printer.py
> index 5f1cc55ecdf5..015adf87dc2c 100644
> --- a/tools/testing/kunit/kunit_printer.py
> +++ b/tools/testing/kunit/kunit_printer.py
> @@ -15,7 +15,7 @@ _RESET = '\033[0;0m'
> class Printer:
> """Wraps a file object, providing utilities for coloring output, etc."""
>
> - def __init__(self, output: typing.IO):
> + def __init__(self, output: typing.IO[str]):
> self._output = output
> self._use_color = output.isatty()
>
> diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
> index 066e6f938f6d..61cece1684df 100755
> --- a/tools/testing/kunit/run_checks.py
> +++ b/tools/testing/kunit/run_checks.py
> @@ -37,7 +37,7 @@ def main(argv: Sequence[str]) -> None:
> if argv:
> raise RuntimeError('This script takes no arguments')
>
> - future_to_name: Dict[futures.Future, str] = {}
> + future_to_name: Dict[futures.Future[None], str] = {}
> executor = futures.ThreadPoolExecutor(max_workers=len(commands))
> for name, argv in commands.items():
> if name in necessary_deps and shutil.which(necessary_deps[name]) is None:
>
> base-commit: 2c6a96dad5797e57b4cf04101d6c8d5c7a571603
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
On Fri, 17 Mar 2023 at 06:06, Daniel Latypov <[email protected]> wrote:
>
> We don't run a linter regularly over kunit.py code (the default settings
> on most don't like kernel style, e.g. tabs) so some of these imports
> didn't get removed when they stopped being used.
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---
While personally, I don't lose sleep over the occasional unused
variable, these (and particularly the imports) seem sensible.
Reviewed-by: David Gow <[email protected]>
Cheers,
-- David
> Note: this is unchanged, just added a 3rd patch to this series.
> ---
> tools/testing/kunit/kunit.py | 2 +-
> tools/testing/kunit/kunit_config.py | 2 +-
> tools/testing/kunit/kunit_kernel.py | 1 -
> tools/testing/kunit/kunit_parser.py | 1 -
> tools/testing/kunit/kunit_tool_test.py | 2 +-
> 5 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 741f15420467..52853634ba23 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -123,7 +123,7 @@ def _suites_from_test_list(tests: List[str]) -> List[str]:
> parts = t.split('.', maxsplit=2)
> if len(parts) != 2:
> raise ValueError(f'internal KUnit error, test name should be of the form "<suite>.<test>", got "{t}"')
> - suite, case = parts
> + suite, _ = parts
> if not suites or suites[-1] != suite:
> suites.append(suite)
> return suites
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index 48b5f34b2e5d..9f76d7b89617 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -8,7 +8,7 @@
>
> from dataclasses import dataclass
> import re
> -from typing import Dict, Iterable, List, Set, Tuple
> +from typing import Dict, Iterable, List, Tuple
>
> CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
> CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index e6fc8fcb071a..775842b912d8 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -18,7 +18,6 @@ import threading
> from typing import Iterator, List, Optional, Tuple
>
> import kunit_config
> -from kunit_printer import stdout
> import qemu_config
>
> KCONFIG_PATH = '.config'
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index a225799f6b1b..fbc094f0567e 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -12,7 +12,6 @@
> from __future__ import annotations
> from dataclasses import dataclass
> import re
> -import sys
> import textwrap
>
> from enum import Enum, auto
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 0c2190514103..be35999bb84f 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -328,7 +328,7 @@ class KUnitParserTest(unittest.TestCase):
> def test_parse_subtest_header(self):
> ktap_log = test_data_path('test_parse_subtest_header.log')
> with open(ktap_log) as file:
> - result = kunit_parser.parse_run_tests(file.readlines())
> + kunit_parser.parse_run_tests(file.readlines())
> self.print_mock.assert_any_call(StrContains('suite (1 subtest)'))
>
> def test_show_test_output_on_failure(self):
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
On Fri, 17 Mar 2023 at 06:06, Daniel Latypov <[email protected]> wrote:
>
> Basically, get this command to be happy and make run_checks.py happy
> $ mypy --strict --exclude '_test.py$' --exclude qemu_configs/ ./tools/testing/kunit/
>
> Primarily the changes are
> * add `-> None` return type annotations
> * add all the missing argument type annotations
>
> Previously, we had false positives from mypy in `main()`, see commit
> 09641f7c7d8f ("kunit: tool: surface and address more typing issues").
> But after commit 2dc9d6ca52a4 ("kunit: kunit.py extract handlers")
> refactored things, the variable name reuse mypy hated is gone.
>
> Note: mypy complains we don't annotate the types the unused args in our
> signal handler. That's silly.
> But to make it happy, I've copy-pasted an appropriate annotation from
> https://github.com/python/typing/discussions/1042#discussioncomment-2013595.
>
> Reported-by: Johannes Berg <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/[email protected]/
> Signed-off-by: Daniel Latypov <[email protected]>
> ---
While I suspect we're pretty rapidly approaching the point of
diminishing returns with some of these (like the signal handler
annotation), I don't think there's any real harm in fixing them and
enabling strict mode.
Reviewed-by: David Gow <[email protected]>
-- David
> tools/testing/kunit/kunit.py | 24 ++++++++++++------------
> tools/testing/kunit/kunit_config.py | 4 ++--
> tools/testing/kunit/kunit_kernel.py | 29 +++++++++++++++--------------
> tools/testing/kunit/run_checks.py | 4 ++--
> 4 files changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 52853634ba23..3905c43369c3 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -269,7 +269,7 @@ def massage_argv(argv: Sequence[str]) -> Sequence[str]:
> def get_default_jobs() -> int:
> return len(os.sched_getaffinity(0))
>
> -def add_common_opts(parser) -> None:
> +def add_common_opts(parser: argparse.ArgumentParser) -> None:
> parser.add_argument('--build_dir',
> help='As in the make command, it specifies the build '
> 'directory.',
> @@ -320,13 +320,13 @@ def add_common_opts(parser) -> None:
> help='Additional QEMU arguments, e.g. "-smp 8"',
> action='append', metavar='')
>
> -def add_build_opts(parser) -> None:
> +def add_build_opts(parser: argparse.ArgumentParser) -> None:
> parser.add_argument('--jobs',
> help='As in the make command, "Specifies the number of '
> 'jobs (commands) to run simultaneously."',
> type=int, default=get_default_jobs(), metavar='N')
>
> -def add_exec_opts(parser) -> None:
> +def add_exec_opts(parser: argparse.ArgumentParser) -> None:
> parser.add_argument('--timeout',
> help='maximum number of seconds to allow for all tests '
> 'to run. This does not include time taken to build the '
> @@ -351,7 +351,7 @@ def add_exec_opts(parser) -> None:
> type=str,
> choices=['suite', 'test'])
>
> -def add_parse_opts(parser) -> None:
> +def add_parse_opts(parser: argparse.ArgumentParser) -> None:
> parser.add_argument('--raw_output', help='If set don\'t parse output from kernel. '
> 'By default, filters to just KUnit output. Use '
> '--raw_output=all to show everything',
> @@ -386,7 +386,7 @@ def tree_from_args(cli_args: argparse.Namespace) -> kunit_kernel.LinuxSourceTree
> extra_qemu_args=qemu_args)
>
>
> -def run_handler(cli_args):
> +def run_handler(cli_args: argparse.Namespace) -> None:
> if not os.path.exists(cli_args.build_dir):
> os.mkdir(cli_args.build_dir)
>
> @@ -405,7 +405,7 @@ def run_handler(cli_args):
> sys.exit(1)
>
>
> -def config_handler(cli_args):
> +def config_handler(cli_args: argparse.Namespace) -> None:
> if cli_args.build_dir and (
> not os.path.exists(cli_args.build_dir)):
> os.mkdir(cli_args.build_dir)
> @@ -421,7 +421,7 @@ def config_handler(cli_args):
> sys.exit(1)
>
>
> -def build_handler(cli_args):
> +def build_handler(cli_args: argparse.Namespace) -> None:
> linux = tree_from_args(cli_args)
> request = KunitBuildRequest(build_dir=cli_args.build_dir,
> make_options=cli_args.make_options,
> @@ -434,7 +434,7 @@ def build_handler(cli_args):
> sys.exit(1)
>
>
> -def exec_handler(cli_args):
> +def exec_handler(cli_args: argparse.Namespace) -> None:
> linux = tree_from_args(cli_args)
> exec_request = KunitExecRequest(raw_output=cli_args.raw_output,
> build_dir=cli_args.build_dir,
> @@ -450,10 +450,10 @@ def exec_handler(cli_args):
> sys.exit(1)
>
>
> -def parse_handler(cli_args):
> +def parse_handler(cli_args: argparse.Namespace) -> None:
> if cli_args.file is None:
> - sys.stdin.reconfigure(errors='backslashreplace') # pytype: disable=attribute-error
> - kunit_output = sys.stdin
> + sys.stdin.reconfigure(errors='backslashreplace') # type: ignore
> + kunit_output = sys.stdin # type: Iterable[str]
> else:
> with open(cli_args.file, 'r', errors='backslashreplace') as f:
> kunit_output = f.read().splitlines()
> @@ -475,7 +475,7 @@ subcommand_handlers_map = {
> }
>
>
> -def main(argv):
> +def main(argv: Sequence[str]) -> None:
> parser = argparse.ArgumentParser(
> description='Helps writing and running KUnit tests.')
> subparser = parser.add_subparsers(dest='subcommand')
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index 9f76d7b89617..eb5dd01210b1 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -8,7 +8,7 @@
>
> from dataclasses import dataclass
> import re
> -from typing import Dict, Iterable, List, Tuple
> +from typing import Any, Dict, Iterable, List, Tuple
>
> CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
> CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'
> @@ -34,7 +34,7 @@ class Kconfig:
> def __init__(self) -> None:
> self._entries = {} # type: Dict[str, str]
>
> - def __eq__(self, other) -> bool:
> + def __eq__(self, other: Any) -> bool:
> if not isinstance(other, self.__class__):
> return False
> return self._entries == other._entries
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 775842b912d8..a3321a991f11 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -16,6 +16,7 @@ import shutil
> import signal
> import threading
> from typing import Iterator, List, Optional, Tuple
> +from types import FrameType
>
> import kunit_config
> import qemu_config
> @@ -56,7 +57,7 @@ class LinuxSourceTreeOperations:
> def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> kunit_config.Kconfig:
> return base_kunitconfig
>
> - def make_olddefconfig(self, build_dir: str, make_options) -> None:
> + def make_olddefconfig(self, build_dir: str, make_options: Optional[List[str]]) -> None:
> command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, 'olddefconfig']
> if self._cross_compile:
> command += ['CROSS_COMPILE=' + self._cross_compile]
> @@ -70,7 +71,7 @@ class LinuxSourceTreeOperations:
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> - def make(self, jobs, build_dir: str, make_options) -> None:
> + def make(self, jobs: int, build_dir: str, make_options: Optional[List[str]]) -> None:
> command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, '--jobs=' + str(jobs)]
> if make_options:
> command.extend(make_options)
> @@ -132,7 +133,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
> class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> """An abstraction over command line operations performed on a source tree."""
>
> - def __init__(self, cross_compile=None):
> + def __init__(self, cross_compile: Optional[str]=None):
> super().__init__(linux_arch='um', cross_compile=cross_compile)
>
> def make_arch_config(self, base_kunitconfig: kunit_config.Kconfig) -> kunit_config.Kconfig:
> @@ -215,7 +216,7 @@ def _get_qemu_ops(config_path: str,
>
> if not hasattr(config, 'QEMU_ARCH'):
> raise ValueError('qemu_config module missing "QEMU_ARCH": ' + config_path)
> - params: qemu_config.QemuArchParams = config.QEMU_ARCH # type: ignore
> + params: qemu_config.QemuArchParams = config.QEMU_ARCH
> if extra_qemu_args:
> params.extra_qemu_params.extend(extra_qemu_args)
> return params.linux_arch, LinuxSourceTreeOperationsQemu(
> @@ -229,10 +230,10 @@ class LinuxSourceTree:
> build_dir: str,
> kunitconfig_paths: Optional[List[str]]=None,
> kconfig_add: Optional[List[str]]=None,
> - arch=None,
> - cross_compile=None,
> - qemu_config_path=None,
> - extra_qemu_args=None) -> None:
> + arch: Optional[str]=None,
> + cross_compile: Optional[str]=None,
> + qemu_config_path: Optional[str]=None,
> + extra_qemu_args: Optional[List[str]]=None) -> None:
> signal.signal(signal.SIGINT, self.signal_handler)
> if qemu_config_path:
> self._arch, self._ops = _get_qemu_ops(qemu_config_path, extra_qemu_args, cross_compile)
> @@ -275,7 +276,7 @@ class LinuxSourceTree:
> logging.error(message)
> return False
>
> - def build_config(self, build_dir: str, make_options) -> bool:
> + def build_config(self, build_dir: str, make_options: Optional[List[str]]) -> bool:
> kconfig_path = get_kconfig_path(build_dir)
> if build_dir and not os.path.exists(build_dir):
> os.mkdir(build_dir)
> @@ -303,7 +304,7 @@ class LinuxSourceTree:
> old_kconfig = kunit_config.parse_file(old_path)
> return old_kconfig != self._kconfig
>
> - def build_reconfig(self, build_dir: str, make_options) -> bool:
> + def build_reconfig(self, build_dir: str, make_options: Optional[List[str]]) -> bool:
> """Creates a new .config if it is not a subset of the .kunitconfig."""
> kconfig_path = get_kconfig_path(build_dir)
> if not os.path.exists(kconfig_path):
> @@ -319,7 +320,7 @@ class LinuxSourceTree:
> os.remove(kconfig_path)
> return self.build_config(build_dir, make_options)
>
> - def build_kernel(self, jobs, build_dir: str, make_options) -> bool:
> + def build_kernel(self, jobs: int, build_dir: str, make_options: Optional[List[str]]) -> bool:
> try:
> self._ops.make_olddefconfig(build_dir, make_options)
> self._ops.make(jobs, build_dir, make_options)
> @@ -328,7 +329,7 @@ class LinuxSourceTree:
> return False
> return self.validate_config(build_dir)
>
> - def run_kernel(self, args=None, build_dir='', filter_glob='', timeout=None) -> Iterator[str]:
> + def run_kernel(self, args: Optional[List[str]]=None, build_dir: str='', filter_glob: str='', timeout: Optional[int]=None) -> Iterator[str]:
> if not args:
> args = []
> if filter_glob:
> @@ -339,7 +340,7 @@ class LinuxSourceTree:
> assert process.stdout is not None # tell mypy it's set
>
> # Enforce the timeout in a background thread.
> - def _wait_proc():
> + def _wait_proc() -> None:
> try:
> process.wait(timeout=timeout)
> except Exception as e:
> @@ -365,6 +366,6 @@ class LinuxSourceTree:
> waiter.join()
> subprocess.call(['stty', 'sane'])
>
> - def signal_handler(self, unused_sig, unused_frame) -> None:
> + def signal_handler(self, unused_sig: int, unused_frame: Optional[FrameType]) -> None:
> logging.error('Build interruption occurred. Cleaning console.')
> subprocess.call(['stty', 'sane'])
> diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
> index 61cece1684df..8208c3b3135e 100755
> --- a/tools/testing/kunit/run_checks.py
> +++ b/tools/testing/kunit/run_checks.py
> @@ -23,7 +23,7 @@ commands: Dict[str, Sequence[str]] = {
> 'kunit_tool_test.py': ['./kunit_tool_test.py'],
> 'kunit smoke test': ['./kunit.py', 'run', '--kunitconfig=lib/kunit', '--build_dir=kunit_run_checks'],
> 'pytype': ['/bin/sh', '-c', 'pytype *.py'],
> - 'mypy': ['/bin/sh', '-c', 'mypy *.py'],
> + 'mypy': ['mypy', '--strict', '--exclude', '_test.py$', '--exclude', 'qemu_configs/', '.'],
> }
>
> # The user might not have mypy or pytype installed, skip them if so.
> @@ -73,7 +73,7 @@ def main(argv: Sequence[str]) -> None:
> sys.exit(1)
>
>
> -def run_cmd(argv: Sequence[str]):
> +def run_cmd(argv: Sequence[str]) -> None:
> subprocess.check_output(argv, stderr=subprocess.STDOUT, cwd=ABS_TOOL_PATH, timeout=TIMEOUT)
>
>
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
Hi Daniel,
On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <[email protected]> wrote:
> E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> returns strings, or `text=False` where it returns bytes.
> To differentiate, you can annotate types as `Popen[str]` or
> `Popen[bytes]`.
>
> This patch should add subscripts in all the places we were missing them.
I just found this patch is in the latest mainline tree, and it causes kunit
failure on my test machine like below.
$ python3 --version
Python 3.8.10
$
$ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
Traceback (most recent call last):
File "./tools/testing/kunit/kunit.py", line 24, in <module>
import kunit_kernel
File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
class LinuxSourceTreeOperations:
File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
TypeError: 'type' object is not subscriptable
$
I further confirmed reverting this patch makes it run again. Do you have any
idea?
Thanks,
SJ
>
> Reported-by: Johannes Berg <[email protected]>
> Link: https://lore.kernel.org/linux-kselftest/20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid/
> Signed-off-by: Daniel Latypov <[email protected]>
> ---
> Note: this is unchanged, just added a 3rd patch to this series.
> ---
> tools/testing/kunit/kunit_kernel.py | 6 +++---
> tools/testing/kunit/kunit_printer.py | 2 +-
> tools/testing/kunit/run_checks.py | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 53e90c335834..e6fc8fcb071a 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -92,7 +92,7 @@ class LinuxSourceTreeOperations:
> if stderr: # likely only due to build warnings
> print(stderr.decode())
>
> - def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> raise RuntimeError('not implemented!')
>
>
> @@ -112,7 +112,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
> kconfig.merge_in_entries(base_kunitconfig)
> return kconfig
>
> - def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> kernel_path = os.path.join(build_dir, self._kernel_path)
> qemu_command = ['qemu-system-' + self._qemu_arch,
> '-nodefaults',
> @@ -141,7 +141,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> kconfig.merge_in_entries(base_kunitconfig)
> return kconfig
>
> - def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> """Runs the Linux UML binary. Must be named 'linux'."""
> linux_bin = os.path.join(build_dir, 'linux')
> params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
> diff --git a/tools/testing/kunit/kunit_printer.py b/tools/testing/kunit/kunit_printer.py
> index 5f1cc55ecdf5..015adf87dc2c 100644
> --- a/tools/testing/kunit/kunit_printer.py
> +++ b/tools/testing/kunit/kunit_printer.py
> @@ -15,7 +15,7 @@ _RESET = '\033[0;0m'
> class Printer:
> """Wraps a file object, providing utilities for coloring output, etc."""
>
> - def __init__(self, output: typing.IO):
> + def __init__(self, output: typing.IO[str]):
> self._output = output
> self._use_color = output.isatty()
>
> diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
> index 066e6f938f6d..61cece1684df 100755
> --- a/tools/testing/kunit/run_checks.py
> +++ b/tools/testing/kunit/run_checks.py
> @@ -37,7 +37,7 @@ def main(argv: Sequence[str]) -> None:
> if argv:
> raise RuntimeError('This script takes no arguments')
>
> - future_to_name: Dict[futures.Future, str] = {}
> + future_to_name: Dict[futures.Future[None], str] = {}
> executor = futures.ThreadPoolExecutor(max_workers=len(commands))
> for name, argv in commands.items():
> if name in necessary_deps and shutil.which(necessary_deps[name]) is None:
>
> base-commit: 2c6a96dad5797e57b4cf04101d6c8d5c7a571603
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
On Sun, Apr 30, 2023 at 11:15 AM SeongJae Park <[email protected]> wrote:
>
> Hi Daniel,
>
> On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <[email protected]> wrote:
>
> > E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> > returns strings, or `text=False` where it returns bytes.
> > To differentiate, you can annotate types as `Popen[str]` or
> > `Popen[bytes]`.
> >
> > This patch should add subscripts in all the places we were missing them.
>
> I just found this patch is in the latest mainline tree, and it causes kunit
> failure on my test machine like below.
>
> $ python3 --version
> Python 3.8.10
> $
> $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> Traceback (most recent call last):
> File "./tools/testing/kunit/kunit.py", line 24, in <module>
> import kunit_kernel
> File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
> class LinuxSourceTreeOperations:
> File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
> def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> TypeError: 'type' object is not subscriptable
> $
>
> I further confirmed reverting this patch makes it run again. Do you have any
> idea?
It seems like support for the subscript wasn't added until Python 3.9.
I know support for subscripting other types like re.Pattern was added
in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
Popen there...
This patch also added typing.IO[str] and concurrent.Future[None], so
those might be problematic too.
Can you check if the typing.IO and concurrent.Future[None] changes
cause problems?
(I don't have an easy way of testing against older Python versions currently).
If so, we should revert the patch.
If not, we can undo just the Popen changes.
And in either case, we'll need to update ./tools/testing/kunit/run_checks.py.
Currently, it runs `mypy --strict` which will start failing if we
revert any part of this patch.
Thanks,
Daniel
Hi Daniel,
On Sun, 30 Apr 2023 14:34:09 -0700 Daniel Latypov <[email protected]> wrote:
> On Sun, Apr 30, 2023 at 11:15 AM SeongJae Park <[email protected]> wrote:
> >
> > Hi Daniel,
> >
> > On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <[email protected]> wrote:
> >
> > > E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> > > returns strings, or `text=False` where it returns bytes.
> > > To differentiate, you can annotate types as `Popen[str]` or
> > > `Popen[bytes]`.
> > >
> > > This patch should add subscripts in all the places we were missing them.
> >
> > I just found this patch is in the latest mainline tree, and it causes kunit
> > failure on my test machine like below.
> >
> > $ python3 --version
> > Python 3.8.10
> > $
> > $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > Traceback (most recent call last):
> > File "./tools/testing/kunit/kunit.py", line 24, in <module>
> > import kunit_kernel
> > File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
> > class LinuxSourceTreeOperations:
> > File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
> > def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> > TypeError: 'type' object is not subscriptable
> > $
> >
> > I further confirmed reverting this patch makes it run again. Do you have any
> > idea?
>
> It seems like support for the subscript wasn't added until Python 3.9.
>
> I know support for subscripting other types like re.Pattern was added
> in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
> Popen there...
> This patch also added typing.IO[str] and concurrent.Future[None], so
> those might be problematic too.
>
> Can you check if the typing.IO and concurrent.Future[None] changes
> cause problems?
> (I don't have an easy way of testing against older Python versions currently).
Thank you for quick reply. Reverting Popen changes only as below fixed my
issue. So seems typing.IO and concurrent.Future[None] chages doesn't cause
problems at least for my use case.
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index f01f94106129..7f648802caf6 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -92,7 +92,7 @@ class LinuxSourceTreeOperations:
if stderr: # likely only due to build warnings
print(stderr.decode())
- def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
+ def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
raise RuntimeError('not implemented!')
@@ -113,7 +113,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
kconfig.merge_in_entries(base_kunitconfig)
return kconfig
- def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
+ def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
kernel_path = os.path.join(build_dir, self._kernel_path)
qemu_command = ['qemu-system-' + self._qemu_arch,
'-nodefaults',
@@ -142,7 +142,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
kconfig.merge_in_entries(base_kunitconfig)
return kconfig
- def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
+ def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
"""Runs the Linux UML binary. Must be named 'linux'."""
linux_bin = os.path.join(build_dir, 'linux')
params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
>
> If so, we should revert the patch.
> If not, we can undo just the Popen changes.
>
> And in either case, we'll need to update ./tools/testing/kunit/run_checks.py.
> Currently, it runs `mypy --strict` which will start failing if we
> revert any part of this patch.
Those make sense.
Thanks,
SJ
>
> Thanks,
> Daniel
On Mon, May 1, 2023 at 10:15 AM SeongJae Park <[email protected]> wrote:
>
[snip]
> >
> > It seems like support for the subscript wasn't added until Python 3.9.
> >
> > I know support for subscripting other types like re.Pattern was added
> > in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
> > Popen there...
> > This patch also added typing.IO[str] and concurrent.Future[None], so
> > those might be problematic too.
> >
> > Can you check if the typing.IO and concurrent.Future[None] changes
> > cause problems?
> > (I don't have an easy way of testing against older Python versions currently).
>
> Thank you for quick reply. Reverting Popen changes only as below fixed my
> issue. So seems typing.IO and concurrent.Future[None] chages doesn't cause
> problems at least for my use case.
Sounds good.
Sent https://lore.kernel.org/linux-kselftest/[email protected]
I was hoping adding `--python-version 3.7` to the `mypy --strict`
invocation would help, but it still fails for me :\
$ ./tools/testing/kunit/run_cheks.py
...
mypy: FAILED
> kunit_kernel.py:95: error: Missing type parameters for generic type "Popen" [type-arg]
> kunit_kernel.py:116: error: Missing type parameters for generic type "Popen" [type-arg]
> kunit_kernel.py:145: error: Missing type parameters for generic type "Popen" [type-arg]
> Found 3 errors in 1 file (checked 8 source files)
And here I was, hoping it would complain about code incompatible with
python 3.7...
But at the very least, that patch should fix the current problem.
Daniel