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:28:34 -0700	[thread overview]
Message-ID: <CAGS_qxofnnP7Ju15iaZ_Szr+aqmHNxU51Kiv723bkd8w9g+Jkg@mail.gmail.com> (raw)
In-Reply-To: <20210617074638.16583-1-sjpark@amazon.de>

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?

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.

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>
> ---
>
> 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:28 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 [this message]
2021-06-22 23:55         ` Daniel Latypov
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_qxofnnP7Ju15iaZ_Szr+aqmHNxU51Kiv723bkd8w9g+Jkg@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).