On Tue, Jun 22, 2021 at 4:28 PM Daniel Latypov dlatypov@google.com wrote:
On Thu, Jun 17, 2021 at 12:46 AM SeongJae Park sj38.park@gmail.com wrote:
Commit 87c9c1631788 ("kunit: tool: add support for QEMU") on the 'next' tree adds 'from __future__ import annotations' in 'kunit_kernel.py'. Because it is supported on only >=3.7 Python, people using older Python will get below error:
Traceback (most recent call last): File "./tools/testing/kunit/kunit.py", line 20, in <module> import kunit_kernel File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 9 from __future__ import annotationsChatted offline with David about this. He was thinking if we could instead drop the minimal version back to 3.6.
I think we can do so, see below. Perhaps we should drop the import and then chain this patch on top of that, specifying a minimum version of 3.6?
Actually, now I've gotten python3.6 installed on my machine, I see we have another issue.
We pass text=true to subprocess. That didn't exist back in 3.6, see https://docs.python.org/3.6/library/subprocess.html
We can workaround that, but there's more chance of subtle bugs that I'd rather we don't touch it.
Checking out https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/?h...
The offending "annotations" import is related to type annotations. Specifically https://www.python.org/dev/peps/pep-0563/
So let's see how the two most popular typecheckers fare.
pytype is happy with or without import. mypy has the same issues with or without the import.
$ mypy tools/testing/kunit/*.py tools/testing/kunit/kunit_kernel.py:227: error: Item "_Loader" of "Optional[_Loader]" has no attribute "exec_module" tools/testing/kunit/kunit_kernel.py:227: error: Item "None" of "Optional[_Loader]" has no attribute "exec_module" tools/testing/kunit/kunit_kernel.py:228: error: Module has no attribute "QEMU_ARCH" tools/testing/kunit/kunit_kernel.py:229: error: Module has no attribute "QEMU_ARCH"
So clearly it's not doing anything for them.
Taking a look over 87c9c1631788 ("kunit: tool: add support for QEMU") next then... I don't see anything that would warrant the import, so we should probably drop it.
Also, using 3.6 now I have it installed, I found what it was added for. But it doesn't need to be there.
This patch drops it and makes things work, afaict: diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index e1951fa60027..5987d5b1b874 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -6,15 +6,13 @@ # Author: Felix Guo felixguoxiuping@gmail.com # Author: Brendan Higgins brendanhiggins@google.com
-from __future__ import annotations import importlib.util import logging import subprocess import os import shutil import signal -from typing import Iterator -from typing import Optional +from typing import Iterator, Optional, Tuple
from contextlib import ExitStack
@@ -208,7 +206,7 @@ def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceT raise ConfigError(arch + ' is not a valid arch')
def get_source_tree_ops_from_qemu_config(config_path: str, - cross_compile: Optional[str]) -> tuple[ + cross_compile: Optional[str]) -> Tuple[ str, LinuxSourceTreeOperations]: # The module name/path has very little to do with where the actual file # exists (I learned this through experimentation and could not find it
In that case, the minimum supported version should drop back down to 3.6. We use enum.auto, which is from 3.6 https://docs.python.org/3/library/enum.html#enum.auto
We could consider stopping using that, and I think we might be then 3.5-compatible. Maybe we have a chain of 3 patches then, drop the import, drop auto, and then add in a >=3.5 version check?
^ SyntaxError: future feature annotations is not definedThis commit adds a version assertion in 'kunit.py', so that people get more explicit error message like below:
Traceback (most recent call last): File "./tools/testing/kunit/kunit.py", line 15, in <module> assert sys.version_info >= (3, 7), "Python version is too old" AssertionError: Python version is too oldSigned-off-by: SeongJae Park sjpark@amazon.de Acked-by: Daniel Latypov dlatypov@google.com
Reviewed-by: Daniel Latypov dlatypov@google.com
As mentioned above, we do actually need 3.7, and not just for the extra import. Now I know that, I feel more strongly that this patch should go in, as-is.
Changes from v1
- Add assertion failure message (Daniel Latypov)
- Add Acked-by: Daniel Latypov dlatypov@google.com
tools/testing/kunit/kunit.py | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index be8d8d4a4e08..6276ce0c0196 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -12,6 +12,8 @@ import sys import os import time
+assert sys.version_info >= (3, 7), "Python version is too old"
from collections import namedtuple from enum import Enum, auto
-- 2.17.1