Re: [PATCH v2 1/3] kunit: tool: add subscripts for type annotations where appropriate

[SeongJae Park <sj@kernel.org>]

Hi Daniel,

On Sun, 30 Apr 2023 14:34:09 -0700 Daniel Latypov <dlatypov@google.com> wrote:

> On Sun, Apr 30, 2023 at 11:15 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > Hi Daniel,
> >
> > On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <dlatypov@google.com> wrote:
> >
> > > E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> > > returns strings, or `text=False` where it returns bytes.
> > > To differentiate, you can annotate types as `Popen[str]` or
> > > `Popen[bytes]`.
> > >
> > > This patch should add subscripts in all the places we were missing them.
> >
> > I just found this patch is in the latest mainline tree, and it causes kunit
> > failure on my test machine like below.
> >
> >     $ python3 --version
> >     Python 3.8.10
> >     $
> >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> >     Traceback (most recent call last):
> >       File "./tools/testing/kunit/kunit.py", line 24, in <module>
> >         import kunit_kernel
> >       File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
> >         class LinuxSourceTreeOperations:
> >       File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
> >         def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> >     TypeError: 'type' object is not subscriptable
> >     $
> >
> > I further confirmed reverting this patch makes it run again.  Do you have any
> > idea?
> 
> It seems like support for the subscript wasn't added until Python 3.9.
> 
> I know support for subscripting other types like re.Pattern was added
> in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
> Popen there...
> This patch also added typing.IO[str] and concurrent.Future[None], so
> those might be problematic too.
> 
> Can you check if the typing.IO and concurrent.Future[None] changes
> cause problems?
> (I don't have an easy way of testing against older Python versions currently).

Thank you for quick reply.  Reverting Popen changes only as below fixed my
issue.  So seems typing.IO and concurrent.Future[None] chages doesn't cause
problems at least for my use case.

    diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
    index f01f94106129..7f648802caf6 100644
    --- a/tools/testing/kunit/kunit_kernel.py
    +++ b/tools/testing/kunit/kunit_kernel.py
    @@ -92,7 +92,7 @@ class LinuxSourceTreeOperations:
     		if stderr:  # likely only due to build warnings
     			print(stderr.decode())
     
    -	def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
    +	def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
     		raise RuntimeError('not implemented!')
     
     
    @@ -113,7 +113,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
     		kconfig.merge_in_entries(base_kunitconfig)
     		return kconfig
     
    -	def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
    +	def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
     		kernel_path = os.path.join(build_dir, self._kernel_path)
     		qemu_command = ['qemu-system-' + self._qemu_arch,
     				'-nodefaults',
    @@ -142,7 +142,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
     		kconfig.merge_in_entries(base_kunitconfig)
     		return kconfig
     
    -	def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
    +	def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
     		"""Runs the Linux UML binary. Must be named 'linux'."""
     		linux_bin = os.path.join(build_dir, 'linux')
     		params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])

> 
> If so, we should revert the patch.
> If not, we can undo just the Popen changes.
> 
> And in either case, we'll need to update ./tools/testing/kunit/run_checks.py.
> Currently, it runs `mypy --strict` which will start failing if we
> revert any part of this patch.

Those make sense.


Thanks,
SJ

> 
> Thanks,
> Daniel