| Message ID | 20201027223815.159802-1-jsnow@redhat.com |
|---|---|
| Headers | show |
| Series | python: add linters to gitlab CI | expand |
On 10/27/20 11:38 PM, John Snow wrote: > Following patches make obvious that we ought to ignore certain > directories to avoid wildly erroneous flake8 output. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > python/setup.cfg | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/python/setup.cfg b/python/setup.cfg > index cb696291ba38..d0ad683b5148 100644 > --- a/python/setup.cfg > +++ b/python/setup.cfg > @@ -30,6 +30,8 @@ devel = > > [flake8] > extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's > +exclude = __pycache__, > + .venv, Can we make flake8 aware the files are in a git repository instead? Anyway, Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > [mypy] > strict = True >
On 10/28/20 2:19 AM, Thomas Huth wrote: > On 27/10/2020 23.38, John Snow wrote: >> Try using pytest to manage our various tests; even though right now >> they're only invoking binaries and not really running any python-native >> code. >> >> Create tests/, and add test_lint.py which calls out to mypy, flake8, >> pylint and isort to enforce the standards in this directory. >> >> Add pytest to the setup.cfg development dependencies; add a pytest >> configuration section as well; run it in verbose mode. >> >> Finally, add pytest to the Pipfile environment and lock the new >> dependencies. (Note, this also updates an unrelated dependency; but the >> only way to avoid this is to pin that dependency at a lower version -- >> which there is no reason to do at present.) >> >> Provided you have the right development dependencies (mypy, flake8, >> isort, pylint, and now pytest) You should be able to run "pytest" from >> the python folder to run all of these linters with the correct >> arguments. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> python/Pipfile.lock | 71 ++++++++++++++++++++++++++++++++++++--- >> python/setup.cfg | 5 +++ >> python/tests/test_lint.py | 28 +++++++++++++++ >> 3 files changed, 99 insertions(+), 5 deletions(-) >> create mode 100644 python/tests/test_lint.py >> >> diff --git a/python/Pipfile.lock b/python/Pipfile.lock >> index 05077475d750..105ffbc09a8e 100644 >> --- a/python/Pipfile.lock >> +++ b/python/Pipfile.lock >> @@ -30,6 +30,14 @@ >> "markers": "python_version >= '3.5'", >> "version": "==2.4.2" >> }, >> + "attrs": { >> + "hashes": [ >> + "sha256:26b54ddbbb9ee1d34d5d3668dd37d6cf74990ab23c828c2888dccdceee395594", >> + "sha256:fce7fc47dfc976152e82d53ff92fa0407700c21acd20886a13777a0d20e655dc" >> + ], >> + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", > > Can't you simply use "python_version >= '3.6'" instead? > > Thomas > This file is generated; I don't really actually know what these markers mean or to whom. I can't edit it because it's checksummed. --js
On 10/28/20 4:50 AM, Philippe Mathieu-Daudé wrote: > On 10/27/20 11:38 PM, John Snow wrote: >> Following patches make obvious that we ought to ignore certain >> directories to avoid wildly erroneous flake8 output. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> python/setup.cfg | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/python/setup.cfg b/python/setup.cfg >> index cb696291ba38..d0ad683b5148 100644 >> --- a/python/setup.cfg >> +++ b/python/setup.cfg >> @@ -30,6 +30,8 @@ devel = >> >> [flake8] >> extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's >> +exclude = __pycache__, >> + .venv, > > Can we make flake8 aware the files are in a git repository instead? > Long story short, no. Python tooling copies source out of git for many reasons -- during installation, packaging, etc -- and it loses git metadata. This is why I have a VERSION file in this directory, too. I have no access to the git tags from within the python packaging ecosystem. --js > Anyway, > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> >> [mypy] >> strict = True >> >
On 10/28/20 2:23 PM, John Snow wrote: > On 10/28/20 2:19 AM, Thomas Huth wrote: >> On 27/10/2020 23.38, John Snow wrote: >>> Try using pytest to manage our various tests; even though right now >>> they're only invoking binaries and not really running any python-native >>> code. >>> >>> Create tests/, and add test_lint.py which calls out to mypy, flake8, >>> pylint and isort to enforce the standards in this directory. >>> >>> Add pytest to the setup.cfg development dependencies; add a pytest >>> configuration section as well; run it in verbose mode. >>> >>> Finally, add pytest to the Pipfile environment and lock the new >>> dependencies. (Note, this also updates an unrelated dependency; but the >>> only way to avoid this is to pin that dependency at a lower version -- >>> which there is no reason to do at present.) >>> >>> Provided you have the right development dependencies (mypy, flake8, >>> isort, pylint, and now pytest) You should be able to run "pytest" from >>> the python folder to run all of these linters with the correct >>> arguments. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> python/Pipfile.lock | 71 ++++++++++++++++++++++++++++++++++++--- >>> python/setup.cfg | 5 +++ >>> python/tests/test_lint.py | 28 +++++++++++++++ >>> 3 files changed, 99 insertions(+), 5 deletions(-) >>> create mode 100644 python/tests/test_lint.py >>> >>> diff --git a/python/Pipfile.lock b/python/Pipfile.lock >>> index 05077475d750..105ffbc09a8e 100644 >>> --- a/python/Pipfile.lock >>> +++ b/python/Pipfile.lock >>> @@ -30,6 +30,14 @@ >>> "markers": "python_version >= '3.5'", >>> "version": "==2.4.2" >>> }, >>> + "attrs": { >>> + "hashes": [ >>> + >>> "sha256:26b54ddbbb9ee1d34d5d3668dd37d6cf74990ab23c828c2888dccdceee395594", >>> >>> + >>> "sha256:fce7fc47dfc976152e82d53ff92fa0407700c21acd20886a13777a0d20e655dc" >>> >>> + ], >>> + "markers": "python_version >= '2.7' and python_version >>> not in '3.0, 3.1, 3.2, 3.3'", >> >> Can't you simply use "python_version >= '3.6'" instead? >> >> Thomas >> > > This file is generated; I don't really actually know what these markers > mean or to whom. I can't edit it because it's checksummed. We should remember to add a line such "The Pipfile.lock content is generated" in the commit message each time it is modified after a change in setup.cfg :) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 28/10/2020 14.23, John Snow wrote: > On 10/28/20 2:19 AM, Thomas Huth wrote: >> On 27/10/2020 23.38, John Snow wrote: >>> Try using pytest to manage our various tests; even though right now >>> they're only invoking binaries and not really running any python-native >>> code. >>> >>> Create tests/, and add test_lint.py which calls out to mypy, flake8, >>> pylint and isort to enforce the standards in this directory. >>> >>> Add pytest to the setup.cfg development dependencies; add a pytest >>> configuration section as well; run it in verbose mode. >>> >>> Finally, add pytest to the Pipfile environment and lock the new >>> dependencies. (Note, this also updates an unrelated dependency; but the >>> only way to avoid this is to pin that dependency at a lower version -- >>> which there is no reason to do at present.) >>> >>> Provided you have the right development dependencies (mypy, flake8, >>> isort, pylint, and now pytest) You should be able to run "pytest" from >>> the python folder to run all of these linters with the correct >>> arguments. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> python/Pipfile.lock | 71 ++++++++++++++++++++++++++++++++++++--- >>> python/setup.cfg | 5 +++ >>> python/tests/test_lint.py | 28 +++++++++++++++ >>> 3 files changed, 99 insertions(+), 5 deletions(-) >>> create mode 100644 python/tests/test_lint.py >>> >>> diff --git a/python/Pipfile.lock b/python/Pipfile.lock >>> index 05077475d750..105ffbc09a8e 100644 >>> --- a/python/Pipfile.lock >>> +++ b/python/Pipfile.lock >>> @@ -30,6 +30,14 @@ >>> "markers": "python_version >= '3.5'", >>> "version": "==2.4.2" >>> }, >>> + "attrs": { >>> + "hashes": [ >>> + >>> "sha256:26b54ddbbb9ee1d34d5d3668dd37d6cf74990ab23c828c2888dccdceee395594", >>> + >>> "sha256:fce7fc47dfc976152e82d53ff92fa0407700c21acd20886a13777a0d20e655dc" >>> + ], >>> + "markers": "python_version >= '2.7' and python_version not >>> in '3.0, 3.1, 3.2, 3.3'", >> >> Can't you simply use "python_version >= '3.6'" instead? >> >> Thomas >> > > This file is generated; I don't really actually know what these markers mean > or to whom. I can't edit it because it's checksummed. If the file is only generated, why do we need that in the repository? ... that only calls for trouble if other people try to apply changes here... Thomas
On 10/28/20 10:54 AM, Thomas Huth wrote: > On 28/10/2020 14.23, John Snow wrote: >> On 10/28/20 2:19 AM, Thomas Huth wrote: >>> On 27/10/2020 23.38, John Snow wrote: >>>> Try using pytest to manage our various tests; even though right now >>>> they're only invoking binaries and not really running any python-native >>>> code. >>>> >>>> Create tests/, and add test_lint.py which calls out to mypy, flake8, >>>> pylint and isort to enforce the standards in this directory. >>>> >>>> Add pytest to the setup.cfg development dependencies; add a pytest >>>> configuration section as well; run it in verbose mode. >>>> >>>> Finally, add pytest to the Pipfile environment and lock the new >>>> dependencies. (Note, this also updates an unrelated dependency; but the >>>> only way to avoid this is to pin that dependency at a lower version -- >>>> which there is no reason to do at present.) >>>> >>>> Provided you have the right development dependencies (mypy, flake8, >>>> isort, pylint, and now pytest) You should be able to run "pytest" from >>>> the python folder to run all of these linters with the correct >>>> arguments. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> python/Pipfile.lock | 71 ++++++++++++++++++++++++++++++++++++--- >>>> python/setup.cfg | 5 +++ >>>> python/tests/test_lint.py | 28 +++++++++++++++ >>>> 3 files changed, 99 insertions(+), 5 deletions(-) >>>> create mode 100644 python/tests/test_lint.py >>>> >>>> diff --git a/python/Pipfile.lock b/python/Pipfile.lock >>>> index 05077475d750..105ffbc09a8e 100644 >>>> --- a/python/Pipfile.lock >>>> +++ b/python/Pipfile.lock >>>> @@ -30,6 +30,14 @@ >>>> "markers": "python_version >= '3.5'", >>>> "version": "==2.4.2" >>>> }, >>>> + "attrs": { >>>> + "hashes": [ >>>> + >>>> "sha256:26b54ddbbb9ee1d34d5d3668dd37d6cf74990ab23c828c2888dccdceee395594", >>>> + >>>> "sha256:fce7fc47dfc976152e82d53ff92fa0407700c21acd20886a13777a0d20e655dc" >>>> + ], >>>> + "markers": "python_version >= '2.7' and python_version not >>>> in '3.0, 3.1, 3.2, 3.3'", >>> >>> Can't you simply use "python_version >= '3.6'" instead? >>> >>> Thomas >>> >> >> This file is generated; I don't really actually know what these markers mean >> or to whom. I can't edit it because it's checksummed. > > If the file is only generated, why do we need that in the repository? ... > that only calls for trouble if other people try to apply changes here... Because it is generated with respect to a given point in time; this specifies the exact loadout of packages that will be used to run the linter. If you remove it, every time you run "pipenv lock" again, it will use newer and newer packages each time ... which defeats the purpose of having a lockfile to begin wtih. The intention is that this lockfile only gets updated as an intentional action; using newer dependencies and so on for the test environment is a conscious action. You are free to use the latest and greatest packages yourself if you choose; just skip the venv step -- but then you're on your own for making sure that environment works. *this* environment receives my full-throated support. The tests will and must pass on *this* environment. --js
based-on: <20201020193555.1493936-1-jsnow@redhat.com> [PATCH v3 00/15] python: create installable package This series serves as an addendum to the python package series; it adds easy invocations for running the linters with the correct options for the purposes of preventing regressions in our python libraries and tools. It adds (to ./python): - make venv: Use pipenv to recreate a venv as laid out precisely by Pipfile.lock; this requires *precisely* Python 3.6. - make check: Run linters using pytest framework using user's current environment (Which may well be their own venv, their OS environment, whatever.) It only requires *a* python; at the moment, 3.6, 3.7, and 3.8 are supported. There are some known problems with 3.9 as of yet. - make venv-check: Run the linters using the venv laid out by Pipfile.lock specifically. Then, it adds a very simple gitlab test to run this on top of the Fedora32 build. I chose Fedora as the host for this test only because Fedora's package ecosystem makes it very easy to install multiple versions of Python, which allows us to test against our minimum required version explicitly to detect against any accidental breakages that slip in from developers coding against 3.7, 3.8, 3.9, etc. This is the simplest thing I could come up with that fulfilled a few design goals I had in mind: 1. It can be run locally with an arbitrary environment 2. It can be run locally with a precise environment, matching how it will be executed in the CI environment 3. It can be invoked from the python directory in a standalone manner. 4. It runs on our CI infrastructure, preventing regressions in the python code. John Snow (5): python: add pytest and tests python: add excluded dirs to flake8 config python: add Makefile for some common tasks python: add .gitignore gitlab: add python linters to CI .gitlab-ci.yml | 10 ++++ python/.gitignore | 9 ++++ python/Makefile | 35 +++++++++++++ python/Pipfile.lock | 71 ++++++++++++++++++++++++-- python/setup.cfg | 7 +++ python/tests/test_lint.py | 28 ++++++++++ tests/docker/dockerfiles/fedora.docker | 2 + 7 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 python/.gitignore create mode 100644 python/Makefile create mode 100644 python/tests/test_lint.py -- 2.26.2