Re: [PATCH 1/1] python: add check-python target

[John Snow]

On 7/14/20 12:30 AM, Cleber Rosa wrote:
> On Mon, Jul 13, 2020 at 09:30:26PM -0400, John Snow wrote:
>> Move pylintrc and flake8 up to the root of the python folder where
>> they're the most useful. Add a requirements.cqa.txt file to house
>> the requirements necessary to build a venv sufficient for running
>> code quality analysis on the python folder. Add a makefile that
>> will build the venv and run the tests.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  Makefile                    |  1 +
>>  python/{qemu => }/.flake8   |  0
>>  python/Makefile.include     | 33 +++++++++++++++++++++++++++++++++
>>  python/{qemu => }/pylintrc  |  1 +
>>  python/requirements.cqa.txt |  3 +++
>>  5 files changed, 38 insertions(+)
>>  rename python/{qemu => }/.flake8 (100%)
>>  create mode 100644 python/Makefile.include
>>  rename python/{qemu => }/pylintrc (99%)
>>  create mode 100644 python/requirements.cqa.txt
>>
>> diff --git a/Makefile b/Makefile
>> index b1b8a5a6d0..41808be392 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -478,6 +478,7 @@ dummy := $(call unnest-vars,, \
>>                  trace-obj-y)
>>  
>>  include $(SRC_PATH)/tests/Makefile.include
>> +include $(SRC_PATH)/python/Makefile.include
>>  
>>  all: $(DOCS) $(if $(BUILD_DOCS),sphinxdocs) $(TOOLS) $(HELPERS-y) 
>> recurse-all modules $(vhost-user-json-y)
>>  
>> diff --git a/python/qemu/.flake8 b/python/.flake8
>> similarity index 100%
>> rename from python/qemu/.flake8
>> rename to python/.flake8
>> diff --git a/python/Makefile.include b/python/Makefile.include
>> new file mode 100644
>> index 0000000000..917808e2f1
>> --- /dev/null
>> +++ b/python/Makefile.include
>> @@ -0,0 +1,33 @@
>> +# -*- Mode: makefile -*-
>> +
>> +PYLIB_VENV_DIR=$(BUILD_DIR)/venv/cqa
>> +PYLIB_VENV_REQ=$(SRC_PATH)/python/requirements.cqa.txt
>> +
>> +$(PYLIB_VENV_DIR): $(PYLIB_VENV_REQ)
>> +    $(call quiet-command, \
>> +        $(PYTHON) -m venv $@, \
>> +        VENV, $@)
>> +    $(call quiet-command, \
>> +        $(PYLIB_VENV_DIR)/bin/python3 -m pip -q install -r 
>> $(PYLIB_VENV_REQ), \
>> +        PIP, $(PYLIB_VENV_REQ))
>> +    $(call quiet-command, touch $@)
>> +
> 
> Maybe we should try to create a generic rule that takes a directory
> name and a requirements file and creates the venv accordingly, instead
> of duplicating the other similar rules under tests/Makefile.include?
> 

Maybe, but I have to admit that my Makefile prowess is lacking and would
be at the mercy of Somebody Else(tm) to do that.

Can I get away with saying "Patches welcome" here?

>> +pylib-venv: $(PYLIB_VENV_DIR)
>> +
>> +check-python: pylib-venv
>> +    $(call quiet-command, cd $(SRC_PATH)/python && \
>> +        $(PYLIB_VENV_DIR)/bin/python3 -m flake8 qemu, \
>> +        FLAKE8, \
>> +        $(SRC_PATH)/python/qemu \
> 
> I can see how this venv would be very useful to run the same checks on
> other Python code (for instance, the acceptance tests themselves), so
> we'd also need another "check-python"-like rule, or include those on
> the same call.
> 
> Ideas? :)
> 

Not good ones at the moment... this is still fuzzy in my head.

There's no reason to conflate development packages (mypy, flake8, and
pylint) with runtime packages (avocado-framework, pycdlib).

I consider the requirements.cqa.txt file I added the "development
requirements". I pinned them to specific versions for the purposes of CI
repeatability. (A future patch that may add a setup.py here would re-add
these packages without pinned versions.)

Since the acceptance test venv had a different use case (running the
code) versus this one (analyzing the code) I kept them separate.

That said; maybe it's not a problem to use the same actual venv, but use
different pip setup steps as-needed. We would need to be careful not to
pin conflicting versions between the two different directories!

We'd also then want a test (somewhere) that did nothing but installed
both sets of requirements and made sure it worked.


Lastly, I want to point out that with future plans to package the python
library as an independently installable entity I want to avoid putting
anything in that directory that references python code it "doesn't
manage". avocado-framework, for example, has no business being
referenced in python/ yet.

Ideally this folder would also have its own Makefile that ran the code
quality analysis checks by itself without the Qemu infrastructure
involved (e.g. you can just type 'make check' inside of ./qemu/python),
but then there's code duplication between Makefile and Makefile.include.

That got messy looking and stupid, so I opted for the top-level include
instead for now so that it could be invoked from the build directory,
but I'm not sure I made the right call.

> Thanks!
> - Cleber.
> 

Oh, and the final step that's needed is to add a gitlab CI job to run
check-python as a test step, but that should be easy after Dan's changes.

--js