Re: [PATCH RFC 2/3] iotests: split 'linters.py' off from 297

[John Snow]

On 6/5/21 10:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 04.06.2021 19:39, John Snow wrote:
> > Refactor the core function of the linting configuration out of 297 and
> > into a new file called linters.py.
> >
> > Now, linters.py represents an invocation of the linting scripts that
> > more resembles a "normal" execution of pylint/mypy, like you'd expect to
> > use if 'qemu' was a bona-fide package you obtained from PyPI.
> >
> > 297, by contrast, now represents the iotests-specific configuration bits
> > you need to get it to function correctly as a part of iotests, and with
> > 'qemu' as a namespace package that isn't "installed" to the current
> > environment, but just lives elsewhere in our source tree.
> >
> > By doing this, we will able to run the same linting configuration from
> > the Python CI tests without calling iotest logging functions or messing
> > around with PYTHONPATH / MYPYPATH.
> >
> > iotest 297 continues to operate in a standalone fashion for now --
> > presumably, it's convenient for block maintainers and contributors to
> > run in this manner.
> >
> > See the following commit for how this is used from the Python 
> > packaging side.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > - It's a big glob of a patch. Sorry. I can work it into smaller pieces
> >    if the idea is well received.
>
> If at least split movement from other refactoring, would be good

Sure. Refactor first and then move seems like the way to go here. I'll 
do that.

> > - I change the invocations of mypy/pylint to "python3 -m pylint" and
> >    "python3 -m mypy" respectively, which causes these linters to use the
> >    virtual environment's preferred version. This forces the test to 
> > use the
> >    test environments curated by the CI jobs.
> >
> > - If you have installed Fedora's pylint package that provides
> >    "pylint-3", the above trick will still work correctly.
> >
> > - Checking for "pylint-3" specifically in 297 was left
> >    alone. Theoretically, this check could be broadened to simply look for
> >    the presence of a 'pylint' module to allow it to be more permissive.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297        |  88 ++++-------------------
> >   tests/qemu-iotests/linters.py | 130 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 143 insertions(+), 75 deletions(-)
> >   create mode 100644 tests/qemu-iotests/linters.py
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 433b732336..5c753279fc 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -17,98 +17,36 @@
> >   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >   import os
> > -import re
> >   import shutil
>
> [..]
>
> > -    # fixed (in tests, at least)
> >       env = os.environ.copy()
> > -    qemu_module_path = os.path.join(os.path.dirname(__file__),
> > -                                    '..', '..', 'python')
> > +    qemu_module_path = os.path.join(
> > +        os.path.dirname(__file__),
> > +        '..', '..', 'python'
> > +    )
>
> Hmm, you made 3 lines from 2 :) ... If rename to python_path it will fit 
> into one line. I'm not sure that it's better.

I'll try it. I don't expect these args to change often so I don't insist 
on the args-as-code-block thing here.

> > +
> >       try:
> >           env['PYTHONPATH'] += os.pathsep + qemu_module_path
> >       except KeyError:
> >           env['PYTHONPATH'] = qemu_module_path
> > -    subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', 
> > *files),
> > -                   env=env, check=False)
> > -    print('=== mypy ===')
> > -    sys.stdout.flush()
> > -
> > -    # We have to call mypy separately for each file.  Otherwise, it
> > -    # will interpret all given files as belonging together (i.e., they
> > -    # may not both define the same classes, etc.; most notably, they
> > -    # must not both define the __main__ module).
> >       env['MYPYPATH'] = env['PYTHONPATH']
> > -    for filename in files:
> > -        p = subprocess.run(('mypy',
> > -                            '--warn-unused-configs',
> > -                            '--disallow-subclassing-any',
> > -                            '--disallow-any-generics',
> > -                            '--disallow-incomplete-defs',
> > -                            '--disallow-untyped-decorators',
> > -                            '--no-implicit-optional',
> > -                            '--warn-redundant-casts',
> > -                            '--warn-unused-ignores',
> > -                            '--no-implicit-reexport',
> > -                            '--namespace-packages',
> > -                            filename),
> > -                           env=env,
> > -                           check=False,
> > -                           stdout=subprocess.PIPE,
> > -                           stderr=subprocess.STDOUT,
> > -                           universal_newlines=True)
> > -        if p.returncode != 0:
> > -            print(p.stdout)
> > +    for linter in ('pylint-3', 'mypy'):
> > +        if shutil.which(linter) is None:
> > +            iotests.notrun(f'{linter} not found')
> > +    iotests.script_main(lambda: linters.run_linters(files, env=env))
>
> Why to use lambda, and not just pass main to script_main?

No strong reason, just a messy draft. I can clean it up as you suggest.

> Or, may be, use iotests.script_initialize() at top, and keep the whole 
> script at top indentation level?

That works too, but it's more churn.

---

I need to look into the mypy invocation and see if I can't figure out a 
way for it to work for everything at once instead of needing to run one 
at a time. Maybe that's something to worry about later, though.

Eventually, the custom linter invocation here should be stored in more 
traditional configuration files (pylintrc, mypy.ini) as much as is possible.

The environment hacking stuff will need to remain here as long as 
iotests does not run in a virtual environment, however. I'd like to 
eventually change that, too.

--js