On Thu, May 19, 2022 at 09:54:56AM +0200, Kevin Wolf wrote:
> Am 18.05.2022 um 20:21 hat John Snow geschrieben:
> > To wire it up to "make check" by *default*, I believe I need to expand the
> > configure script to poll for certain requisites and then create some
> > wrapper script of some kind that only engages the python tests if the
> > requisites were met ... and I lose some control over the mypy/pylint
> > versioning windows. I have to tolerate a wider versioning, or it'll never
> > get run in practice.
> >
> > I have some reluctance to doing this, because pylint and mypy change so
> > frequently that I don't want "make check" to fail spuriously in the future.
> >
> > (In practice, these failures occur 100% of the time when I am on vacation.)
>
> So we seem to agree that it's something that we do expect to fail from
> time to time. Maybe this is how I could express my point better: If it's
> a hard failure, it should fail as early as possible - i.e. ideally
> before the developer sends a patch, but certainly before failing a pull
> request.
At least with pylint we can make an explicit list of which lint
checks we want to run, so we should not get new failures when a
new pylint is released. If there are rare cases where we none
the less see a new failure from a new release, then so be it,
whoever hits it first can send a patch. IOW, I think we should
just enable pylint all the time with a fixed list of tests we
care about. Over time we can enable more of its checks when
desired.
Yeh, this might help a bit. If we use system packages by default, we'll also generally avoid using bleeding edge packages and I'll (generally) catch those myself via check-tox before people run into them organically.
I don't know enough about mypy to know if it can provide similar
level of control. Possibly the answer for "should we run it by default"
will be different for pylint vs mypy.
Yeah, we can probably do different things. mypy is actually much more stable than pylint IMO, it's probably actually okay to just let that one behave as-is.
(I know I have a fix for 0.950 in my recent rfc series, but anecdotally I feel mypy changes behavior a lot less often than pylint. isort and flake8 have basically never ever broken on update for me, either.)
Still, none of this is all that different from the case where
new GCC or CLang are released and developers find new warnings
have arrived. People just send patches when they hit this.
Given python is a core part of QEMU's dev tooling, I think it
is reasonable to expect developers to cope with this for python
too, as long as the frequency of problems is not unreasonably
high.
To some extent, though it's still a bummer to get warnings and errors that have nothing to do with your changes. I have made sure I test a wide matrix to the best of my ability, so it should be fine. I guess I'm just super conservative about it ...
(Well, and even when I had the check-tox test set to allow failure, the yellow exclamation mark still annoyed people. I'm just keen to avoid more nastygrams.)
> > That said ... maybe I can add a controlled venv version of "check-python"
> > and just have a --disable-check-python or something that spec files can opt
> > into. Maybe that will work well enough?
> >
> > i.e. maybe configure can check for the presence of pip, the python venv
> > module (debian doesnt ship it standard...), and PyPI connectivity and if
> > so, enables the test. Otherwise, we skip it.
>
> I think this should work. If detecting the right environment is hard, I
> don't think there is even a requirement to do so. You can make
> --enable-check-python the default and if people don't want it, they can
> explicitly disable it. (I understand that until you run 'make check', it
> doesn't make a difference anyway, so pure users would never have to
> change the option, right?)
I think it should just be the default too. Contributors have to accept
that python is a core part of our project and we expect such code to
pass various python quality control tests, on the wide variety of OS
platforms we run on.
I meant that I'd have the default be "auto", but if you're arguing for the default to be "on", I suppose I could. I have a weak preference for keeping the min requisites for a no-option configure set small. This should be trivial to change in either direction, though.
The requisites aren't steep: you just need python and the venv stdlib module. If you have python, you meet that requisite on every platform except debian/ubuntu, which ships venv separately. In practice, it probably will be enabled for most people by default.
> > Got it. I'll see what I can come up with that checks the boxes for
> > everyone, thanks for clarifying yours.
> >
> > I want to make everything "just work" but I'm also afraid of writing too
> > much magic crap that could break and frustrate people who have no desire to
> > understand python packaging junk, so I'm trying to balance that.
>
> Yes, sounds like we need to find some balance there. Test infrastructure
> breaking locally for no obvious reason can be quite frustrating. But
> sending a patch and getting it queued, only to be notified that it's
> dropped again because of a mypy problem two weeks later when the
> maintainer sends the pull request, can be equally (if not even more)
> frustrating.
The other benefit to having the checks turned on by default for everyone
is that it removes John as the centralized point responsible for finding,
investigating and addressing python style issues, and distributes it
amongst all the QEMU contributors.
Haha. I won't hold my breath for anyone else to have patience enough to track it down, but I appreciate the sentiment :)
Thanks for the feedback. I'm still working on my RFC for the actual unit testing venv but ran into some difficulties with vm tests and Avocado tests being a little flaky, and testing has been slow.
So: v2 RFC coming up soon and I'll put code and test results to all these worrrrrrrrrds.
--js