Re: Running iotest linters from check-python-* CI jobs

[John Snow]

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-jsnow@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.
>
> 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.

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.)

> 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.

> (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.)

- 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.

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.)

> Max

--js