[PATCH 0/1] python: add make check-python target

[PATCH 0/1] python: add make check-python target

[John Snow]

Quick try at getting the code quality analysis tools into a makefile
target.

Based-on: 20200710052220.3306-1-jsnow@redhat.com
Based-on: 20200710050649.32434-1-jsnow@redhat.com

There are other python patches pulled since that cause new errors to
show up, so this definitely can't be enforced just yet.

John Snow (1):
  python: add check-python target

 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

-- 
2.21.3

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

[John Snow]

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 $@)
+
+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 \
+	)
+	$(call quiet-command, cd $(SRC_PATH)/python && \
+	    $(PYLIB_VENV_DIR)/bin/python3 -m pylint qemu, \
+	    PYLINT, \
+	    $(SRC_PATH)/python/qemu \
+	)
+	$(call quiet-command, cd $(SRC_PATH)/python && \
+	    $(PYLIB_VENV_DIR)/bin/python3 -m mypy \
+	        --strict --no-error-summary qemu, \
+	    MYPY, \
+	    "--strict $(SRC_PATH)/python/qemu" \
+	)
diff --git a/python/qemu/pylintrc b/python/pylintrc
similarity index 99%
rename from python/qemu/pylintrc
rename to python/pylintrc
index 5d6ae7367d..65b4545a6b 100644
--- a/python/qemu/pylintrc
+++ b/python/pylintrc
@@ -16,6 +16,7 @@ disable=too-many-arguments,
         too-many-public-methods,
 
 [REPORTS]
+score=no
 
 [REFACTORING]
 
diff --git a/python/requirements.cqa.txt b/python/requirements.cqa.txt
new file mode 100644
index 0000000000..dbf45984bc
--- /dev/null
+++ b/python/requirements.cqa.txt
@@ -0,0 +1,3 @@
+pylint==2.5.3
+mypy==0.782
+flake8==3.8.3
-- 
2.21.3

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

[Cleber Rosa]

[-- Attachment #1: Type: text/plain, Size: 2802 bytes --]

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?

> +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? :)

Thanks!
- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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