linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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
> >

  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 " 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 \
    --subject='Re: [PATCH v2] kunit: tool: Assert the version requirement' \
    /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

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).