Am 22.06.2021 um 18:24 hat John Snow geschrieben: > On 6/22/21 11:52 AM, Max Reitz wrote: > > On 22.06.21 16:57, John Snow wrote: > > > Hi Kevin: > > > > > > At one point I had the idea to augment the Python linter CI jobs to > > > also run the iotest linters. I thought it would be convenient to > > > ensure that while I changed around the QMP and Machine packages that > > > I didn't introduce regressions in iotest 297 either. > > > > > > I sent an RFC, got feedback from Vladimir (Who seemed broadly in > > > favor of the idea), and then wrote a v2 that I never sent. > > > > > > RFC: Message-Id: <20210604163907.1511224-1-js...@redhat.com> > > > > > > mreitz stated (on IRC) in no uncertain terms they were not happy > > > with the idea of 297 becoming gating CI, so I held off on pursuing > > > the idea. I wanted to reach out and see if you had feelings on the > > > matter, or if I should indeed just shelve it entirely.
I like the general idea of making such checks gating CI, because if we already have them, what are we gaining by only manually finding violations? The more interesting part is defining our standards, i.e. writing config files for the tools, and we already do that for 297 anyway. The potential problem I could see is different linter versions, but you already addressed that below. > > My main point was that I don’t want to have to have an opinion on this > > topic. ;) > > > > Sorry if I put words in your mouth! I wanted to take your feedback/reaction > seriously. > > > It’s true that I’m not happy about linters being part of gating CI, but > > I also stated that I cannot defend this gut feeling, and that I feel > > like it’s “objectively” wrong. Therefore, I don’t want to be part of > > such a discussion, if I can avoid it. > > (To my defense, in virtiofsd-rs I myself made a linter part of the > > gating CI. That’s because we already had another linter in it, and > > because my gut feeling is much easier to suppress when it’s about a > > small project with few maintainers to annoy. It has nothing to do with > > me hating Python coding style guidelines, because I probably hate Rust > > coding style guidelines just as much.) > > > > 😅 > > I'll fully admit that pylint in particular is very, very annoying. My RFC > does not increase the strictness of its use for iotests, at least. Yeah, I'm not sure if pylint ever warned about something that I actually cared to get changed... Most times it's just failing to meet some questionable arbitrary style requirements. On the other hand, I assume you count mypy as a linter, too, and the messages of that one I treat more like compiler warnings or errors. They are actually useful, and if your code doesn't pass, then I usually do care about it getting fixed. I would actually prefer our mypy config to become stricter over time. > There are three linting standards for Python in the tree right now: > > 1. Those applied to scripts/qapi/ (Manually run only) > 2. Those applied to tests/iotests/ (via 297) > 3. Those applied to python/qemu/ (via CI) > > The python/qemu/ ones are the strictest and most annoying. scripts/qapi/ has > an almost identical set of rules that will be integrated to python/qemu/ > once I move the QAPI generator there. > > The iotests ones are separate and I intend to keep separate -- I think it > should remain up to the block maintainers what their own tolerance level for > annoying yappy errors are. I have no desire to change that. > > (I definitely have no desire to scrub and audit everything in iotests to > bring it up to speed with the stricter standard. They're just tests, after > all. It's not worth it.) Right, individual tests aren't that important, especially concerning style, though I feel shared files like iotests.py and the test infrastructure itself are probably worth it. > > In any case, I had understood you wanted to make 297 part of the > > non-gating CI anyway, though, so I wonder what of the things I said made > > you shelve that idea. > > I just don't like pursuing things that might increase your maintenance > burden or make your day worse. I know you don't want to be involved, but > this kind of necessarily involves you at least indirectly, so ... It > genuinely felt a little rude to press onward without getting a bit more > information first. > > I figured I'd ask Kevin what his feelings are to see if that > un-muddies the waters. So my hope is that it would in fact decrease the maintenance burden because we would catch bugs in the tests in time, and dealing with false positives would cost us less time than dealing with such bugs. But then, this is something that is mostly a point for mypy, not for pylint. > > (Another concern I had was linter updates breaking CI, but you promised > > to keep the linter in a steady configuration so this wouldn’t happen. So > > all in all, I can’t remember I brought any argument that would ac > > buttually speak against your idea.) > > > > Right. I can't promise stability for iotest 297 itself, because that test is > designed to run with "whatever the person running it happens to have > installed", which I can't control. But we can't control that right now > anyway, so that's not a regression. > > I *can* control the CI environment, though. There are two python linting > jobs that run in CI now: > > - check-python-pipenv is a CI job that runs against Python 3.6 and a very > specific pinned list of dependencies (and their dependencies). These > packages do not change unless we/I change them explicitly. It should offer > stability to the linting environment in CI. Right now, this job is a > "must-pass" CI job. > > (If you have the right packages, you can run the same exact test locally > with 'make venv-check' in the python/ directory.) So this would be the one additional thing that I should regularly run before sending a pull request? > - check-python-tox is a CI job that runs against Python 3.6 through 3.10 > inclusive. The Python versions it tests against are manually configured. The > versions of the linters (and their dependencies) it uses are always "the > latest ones that fulfill the dependency criteria". This CI job, however, is > allowed to fail with a warning. What does "warning" mean here? Do all linter failures just result in CI warnings, or is this the difference between errors and warnings that linters are making? So we would ignore only warnings, but errors would still make the CI job fail? > My belief was that it would be useful to find out about new linter or python > incompatibilities without holding up the build. They're likely things that > will eventually show up in people's manual invocations of 297 as they > upgrade their distro, their packages, etc. But it is definitely not a gating > test. Just a heads up thing. > > (If you have the right packages, you can run the same exact test locally > with 'make check-tox' in the python/ directory.) Makes sense to me as a heads up thing. So I should run this one locally every once in a while, or just check CI results of a pull request after it went in? Kevin
