On Fri, 17 Mar 2023 at 06:06, Daniel Latypov dlatypov@google.com 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 dlatypov@google.com

While personally, I don't lose sleep over the occasional unused variable, these (and particularly the imports) seem sensible.

Reviewed-by: David Gow davidgow@google.com

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 dlatypov@google.com 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 johannes.berg@intel.com Link: https://lore.kernel.org/linux-kselftest/9a172b50457f4074af41fe1dc8e55dcaf479... Signed-off-by: Daniel Latypov dlatypov@google.com

---

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 davidgow@google.com

-- 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 Sun, 30 Apr 2023 14:34:09 -0700 Daniel Latypov dlatypov@google.com wrote:

On Sun, Apr 30, 2023 at 11:15 AM SeongJae Park sj@kernel.org wrote:

...

Hi Daniel,

On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov dlatypov@google.com 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