Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210617073937.16281-1-sjpark@amazon.de> <20210617074638.16583-1-sjpark@amazon.de>
 <CAGS_qxofnnP7Ju15iaZ_Szr+aqmHNxU51Kiv723bkd8w9g+Jkg@mail.gmail.com>
In-Reply-To: <CAGS_qxofnnP7Ju15iaZ_Szr+aqmHNxU51Kiv723bkd8w9g+Jkg@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Tue, 22 Jun 2021 16:55:21 -0700
Message-ID: <CAGS_qxoPq1f+dcaf43xyjbDhW-ASG3gZez-b0Pv_s17JU3hePw@mail.gmail.com>
Subject: Re: [PATCH v2] kunit: tool: Assert the version requirement
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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