On Fri, Aug 30, 2024 at 7:29 AM Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Fri, Aug 30, 2024 at 01:20:35PM +0200, Markus Armbruster wrote: > > John Snow <js...@redhat.com> writes: > > > > > This is being done for the sake of unifying the linting and static type > > > analysis configurations between scripts/qapi and python/qemu/*. > > > > > > With this change, the qapi module will now be checked by mypy, flake8, > > > pylint, isort etc under all python versions from 3.8 through 3.13 under > > > a variety of different dependency configurations in the GitLab testing > > > pipelines. > > > > > > The tests can be run locally, as always: > > > > > >> cd qemu.git/python > > >> make check-minreqs > > >> make check-tox > > >> make check-dev > > > > > > "check-minreqs" is the must-pass GitLab test. > > > "check-tox" is the optional allowed-to-fail GitLab test. > > > > > > Signed-off-by: John Snow <js...@redhat.com> > > > > I don't understand why we have to keep Python code in its own directory > > just to get it checked. We wouldn't do that say for Rust code, would > > we? Anyway, if it's the price of checking, I'll pay[*]. > > The 'check-tox' target is defined in python/Makefile, and is > written to only check code below that location, which is a > somewhat arbitrary choice. > > Having this in "make" at all is a bit outdated. Ideally all > the targets in python/Makefile should be converted into meson > targets and/or tests, in a "python" suite. > Yes, ideally, but there are still gaps between the python tooling ecosystem and our own conventions in the QEMU source tree. Saying "everything under python/ should adhere to python ecosystem conventions" was just an extremely convenient way to make python tooling not throw a hissy fit over stuff in that directory. The "make" targets in that directory are really just simple script invocations that we don't really need "make" for at all, it's just so I don't have to re-explain how to invoke the tests as they stand. I don't think it's "outdated" in that sense, it's just a nice little syntactic shorthand using a tool people are familiar with - not too far out from what we do for the VM tests. One of the barriers to implementing this in "make check" et al is setting up the package installation needed for e.g. mypy and the optional feature packages (like urwid, fusepy, etc) in a way that is unobtrusive to the build system. The build-time configure venv was a step in that direction, but the work remains unfinished. I am in the process (right now!) of setting up mkvenv to install the in-tree python packages to the configure venv - one of the barriers *here* is that this is a little bit slow with older setuptools, and due to very tumultuous changes in Python packaging in the last several years (fallout from PEP-517, PEP-518 and PEP-660) is that it is taking me a good long while to ensure it's rock solid across all of our supported platforms while supporting isolated offline builds. (Python stuff was really, really, really not designed to cope very well with that restriction, I have learned in the last few years.) Another reason for the decision to treat the python/ subdirectory as a "mini-repository" was purely for sake of ease with splitting out subpackages that became useful beyond the confines of our castle walls: qemu.qmp in particular. By structuring everything here like an upstream package, it becomes pretty trivial to fork things out into standalone packages, because they're already structured identically to how standalone packages would be. TLDR: "It makes my life easier to follow python ecosystem norms in this directory, but I still want to integrate it more formally to the QEMU build system and am working on that." > > IOW, we should make 'check-tox' a target in meson.build at the > top level, and have it check any .py file in the source tree, > with an exclude list for files we know are not "clean" yet, > so we don't have to move stuff around as we clean up individual > files. > Maybe someday. Right now, I consider anything under python/ to be "maintained" for the python ecosystem and anything outside of this to be "good luck!". I do not volunteer to apply the same level of effort I do for python/ to *every last python script in the tree*. The standards I apply in python/ are vastly more strict than those that could apply to everything else; I don't think I could handle the workload of polishing every last python thing in the tree up to the same standard. qemu.qmp, qemu.machine and qapi are just vastly more important than the majority of one-off scripts that automate niche tasks I have no domain expertise in. For any python scripts with import dependencies on other in-tree modules, it is useful to structure those imported modules as normal packages so that mypy, pylint, etc can function with minimal fuss and without needing lots of prone-to-breakage environment hacks and custom launcher scripts to make them work. Anything that *doesn't* need to import anything else from the tree is relatively easy to check where it lives, I just don't do that at the moment. It's possible we could make a separate test that applies a much lower bar of type checking and linting that really only catches *hard* errors and applies to everything in the tree automatically, but I don't think I want to lower the bar for the stuff already in python/. That said, I *do* want to move the python tests into our usual build testing infrastructure, there's just some more work to do with mkvenv to bridge the gap, which I'm working on currently. > > > > > [...] > > > > > diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py > > > index f3518d29a54..42912c91716 100644 > > > --- a/scripts/qapi-gen.py > > > +++ b/scripts/qapi-gen.py > > > @@ -11,9 +11,11 @@ > > > execution environment. > > > """ > > > > > > +import os > > > import sys > > > > > > -from qapi import main > > > +sys.path.append(os.path.join(os.path.dirname(__file__), '..', > 'python')) > > > +from qemu.qapi import main > > > > > > if __name__ == '__main__': > > > sys.exit(main.main()) > > > > Suggest to use the opportunity to rename to just qapi-gen (no .py) and > > chmod +x, possibly in a separate patch. > > > > [...] > > > > > > [*] Grudgingly. > > > > With regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| > >
