[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Running iotest linters from check-python-* CI jobs
From: |
John Snow |
Subject: |
Re: Running iotest linters from check-python-* CI jobs |
Date: |
Tue, 22 Jun 2021 12:24:45 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 |
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