From: Daniel Latypov <dlatypov@google.com>
To: SeongJae Park <sj38.park@gmail.com>
Cc: brendanhiggins@google.com, linux-kselftest@vger.kernel.org,
kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org,
SeongJae Park <sjpark@amazon.de>
Subject: Re: [PATCH v2] kunit: tool: Assert the version requirement
Date: Tue, 22 Jun 2021 16:55:21 -0700 [thread overview]
Message-ID: <CAGS_qxoPq1f+dcaf43xyjbDhW-ASG3gZez-b0Pv_s17JU3hePw@mail.gmail.com> (raw)
In-Reply-To: <CAGS_qxofnnP7Ju15iaZ_Szr+aqmHNxU51Kiv723bkd8w9g+Jkg@mail.gmail.com>
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 annotations
>
> Chatted 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=kunit-fixes
>
> 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 defined
> >
> > This 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 old
> >
> > Signed-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
> >
next prev parent reply other threads:[~2021-06-22 23:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-16 9:40 [PATCH] kunit: tool: Assert version requirement SeongJae Park
2021-06-16 21:14 ` Daniel Latypov
2021-06-17 7:39 ` SeongJae Park
2021-06-17 7:46 ` [PATCH v2] kunit: tool: Assert the " SeongJae Park
2021-06-22 23:28 ` Daniel Latypov
2021-06-22 23:55 ` Daniel Latypov [this message]
2021-06-28 13:37 ` SeongJae Park
2021-06-28 19:51 ` Brendan Higgins
2021-06-28 19:41 ` Brendan Higgins
2021-07-12 19:42 ` Shuah Khan
2021-07-12 19:47 ` SeongJae Park
2021-07-12 19:52 ` [PATCH v3] " SeongJae Park
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGS_qxoPq1f+dcaf43xyjbDhW-ASG3gZez-b0Pv_s17JU3hePw@mail.gmail.com \
--to=dlatypov@google.com \
--cc=brendanhiggins@google.com \
--cc=kunit-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=sj38.park@gmail.com \
--cc=sjpark@amazon.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).