[PATCH 0/5] python: add linters to gitlab CI

[PATCH 0/5] python: add linters to gitlab CI

[John Snow]

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

[PATCH 1/5] python: add pytest and tests

[John Snow]

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'",
+            "version": "==20.2.0"
+        },
         "flake8": {
             "hashes": [
                 "sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
@@ -46,6 +54,13 @@
             "markers": "python_version < '3.8'",
             "version": "==2.0.0"
         },
+        "iniconfig": {
+            "hashes": [
+                "sha256:011e24c64b7f47f6ebd835bb12a743f2fbe9a26d4cecaa7f53bc4f35ee9da8b3",
+                "sha256:bc3af051d7d14b2ee5ef9969666def0cd1a000e121eaea580d4a313df4b37f32"
+            ],
+            "version": "==1.1.1"
+        },
         "isort": {
             "hashes": [
                 "sha256:dcab1d98b469a12a1a624ead220584391648790275560e1a43e54c5dceae65e7",
@@ -115,6 +130,30 @@
             ],
             "version": "==0.4.3"
         },
+        "packaging": {
+            "hashes": [
+                "sha256:4357f74f47b9c12db93624a82154e9b120fa8293699949152b22065d556079f8",
+                "sha256:998416ba6962ae7fbd6596850b80e17859a5753ba17c32284f67bfff33784181"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==20.4"
+        },
+        "pluggy": {
+            "hashes": [
+                "sha256:15b2acde666561e1298d71b523007ed7364de07029219b604cf808bfa1c765b0",
+                "sha256:966c145cd83c96502c3c3868f50408687b38434af77734af1e9ca461a4081d2d"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==0.13.1"
+        },
+        "py": {
+            "hashes": [
+                "sha256:366389d1db726cd2fcfc79732e75410e5fe4d31db13692115529d34069a043c2",
+                "sha256:9ca6883ce56b4e8da7e79ac18787889fa5206c79dcc67fb065376cd2fe03f342"
+            ],
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==1.9.0"
+        },
         "pycodestyle": {
             "hashes": [
                 "sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
@@ -139,6 +178,22 @@
             "markers": "python_version >= '3.5'",
             "version": "==2.6.0"
         },
+        "pyparsing": {
+            "hashes": [
+                "sha256:c203ec8783bf771a155b207279b9bccb8dea02d8f0c9e5f8ead507bc3246ecc1",
+                "sha256:ef9d7589ef3c200abe66653d3f1ab1033c3c419ae9b9bdb1240a85b024efc88b"
+            ],
+            "markers": "python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2, 3.3'",
+            "version": "==2.4.7"
+        },
+        "pytest": {
+            "hashes": [
+                "sha256:7a8190790c17d79a11f847fba0b004ee9a8122582ebff4729a082c109e81a4c9",
+                "sha256:8f593023c1a0f916110285b6efd7f99db07d59546e3d8c36fc60e2ab05d3be92"
+            ],
+            "markers": "python_version >= '3.5'",
+            "version": "==6.1.1"
+        },
         "qemu": {
             "editable": true,
             "path": "."
@@ -148,7 +203,7 @@
                 "sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259",
                 "sha256:8b74bedcbbbaca38ff6d7491d76f2b06b3592611af620f8426e82dddb04a5ced"
             ],
-            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'",
+            "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
             "version": "==1.15.0"
         },
         "toml": {
@@ -162,9 +217,11 @@
             "hashes": [
                 "sha256:0666aa36131496aed8f7be0410ff974562ab7eeac11ef351def9ea6fa28f6355",
                 "sha256:0c2c07682d61a629b68433afb159376e24e5b2fd4641d35424e462169c0a7919",
+                "sha256:0d8110d78a5736e16e26213114a38ca35cb15b6515d535413b090bd50951556d",
                 "sha256:249862707802d40f7f29f6e1aad8d84b5aa9e44552d2cc17384b209f091276aa",
                 "sha256:24995c843eb0ad11a4527b026b4dde3da70e1f2d8806c99b7b4a7cf491612652",
                 "sha256:269151951236b0f9a6f04015a9004084a5ab0d5f19b57de779f908621e7d8b75",
+                "sha256:3742b32cf1c6ef124d57f95be609c473d7ec4c14d0090e5a5e05a15269fb4d0c",
                 "sha256:4083861b0aa07990b619bd7ddc365eb7fa4b817e99cf5f8d9cf21a42780f6e01",
                 "sha256:498b0f36cc7054c1fead3d7fc59d2150f4d5c6c56ba7fb150c013fbc683a8d2d",
                 "sha256:4e3e5da80ccbebfff202a67bf900d081906c358ccc3d5e3c8aea42fdfdfd51c1",
@@ -173,16 +230,20 @@
                 "sha256:73d785a950fc82dd2a25897d525d003f6378d1cb23ab305578394694202a58c3",
                 "sha256:8c8aaad94455178e3187ab22c8b01a3837f8ee50e09cf31f1ba129eb293ec30b",
                 "sha256:8ce678dbaf790dbdb3eba24056d5364fb45944f33553dd5869b7580cdbb83614",
+                "sha256:92c325624e304ebf0e025d1224b77dd4e6393f18aab8d829b5b7e04afe9b7a2c",
                 "sha256:aaee9905aee35ba5905cfb3c62f3e83b3bec7b39413f0a7f19be4e547ea01ebb",
+                "sha256:b52ccf7cfe4ce2a1064b18594381bccf4179c2ecf7f513134ec2f993dd4ab395",
                 "sha256:bcd3b13b56ea479b3650b82cabd6b5343a625b0ced5429e4ccad28a8973f301b",
                 "sha256:c9e348e02e4d2b4a8b2eedb48210430658df6951fa484e59de33ff773fbd4b41",
                 "sha256:d205b1b46085271b4e15f670058ce182bd1199e56b317bf2ec004b6a44f911f6",
                 "sha256:d43943ef777f9a1c42bf4e552ba23ac77a6351de620aa9acf64ad54933ad4d34",
                 "sha256:d5d33e9e7af3b34a40dc05f498939f0ebf187f07c385fd58d591c533ad8562fe",
+                "sha256:d648b8e3bf2fe648745c8ffcee3db3ff903d0817a01a12dd6a6ea7a8f4889072",
+                "sha256:fac11badff8313e23717f3dada86a15389d0708275bddf766cca67a84ead3e91",
                 "sha256:fc0fea399acb12edbf8a628ba8d2312f583bdbdb3335635db062fa98cf71fca4",
                 "sha256:fe460b922ec15dd205595c9b5b99e2f056fd98ae8f9f56b888e7a17dc2b757e7"
             ],
-            "markers": "implementation_name == 'cpython' and python_version < '3.8'",
+            "markers": "python_version < '3.8' and implementation_name == 'cpython'",
             "version": "==1.4.1"
         },
         "typing-extensions": {
@@ -201,11 +262,11 @@
         },
         "zipp": {
             "hashes": [
-                "sha256:16522f69653f0d67be90e8baa4a46d66389145b734345d68a257da53df670903",
-                "sha256:c1532a8030c32fd52ff6a288d855fe7adef5823ba1d26a29a68fd6314aa72baa"
+                "sha256:102c24ef8f171fd729d46599845e95c7ab894a4cf45f5de11a44cc7444fb1108",
+                "sha256:ed5eee1974372595f9e416cc7bbeeb12335201d8081ca8a0743c954d4446e5cb"
             ],
             "markers": "python_version >= '3.6'",
-            "version": "==3.3.1"
+            "version": "==3.4.0"
         }
     }
 }
diff --git a/python/setup.cfg b/python/setup.cfg
index 503384f0c920..cb696291ba38 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -25,6 +25,7 @@ devel =
     isort >= 5.0.5
     mypy >= 0.770
     pylint >= 2.6.0
+    pytest >= 3.0.0
 
 
 [flake8]
@@ -72,3 +73,7 @@ include_trailing_comma=True
 line_length=72
 lines_after_imports=2
 multi_line_output=3
+
+[tool:pytest]
+addopts = -v
+console_output_style = count
diff --git a/python/tests/test_lint.py b/python/tests/test_lint.py
new file mode 100644
index 000000000000..38fefa01c667
--- /dev/null
+++ b/python/tests/test_lint.py
@@ -0,0 +1,28 @@
+import subprocess
+
+
+def _external_test(command_line: str) -> None:
+    args = command_line.split(' ')
+    try:
+        subprocess.run(args, check=True)
+    except subprocess.CalledProcessError as err:
+        # Re-contextualize to avoid pytest showing error context inside
+        # the subprocess module itself
+        raise Exception("'{:s}' returned {:d}".format(
+            ' '.join(args), err.returncode)) from None
+
+
+def test_isort() -> None:
+    _external_test('isort -c qemu')
+
+
+def test_flake8() -> None:
+    _external_test('flake8')
+
+
+def test_pylint() -> None:
+    _external_test('pylint qemu')
+
+
+def test_mypy() -> None:
+    _external_test('mypy -p qemu')
-- 
2.26.2

[PATCH 2/5] python: add excluded dirs to flake8 config

[John Snow]

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,
 
 [mypy]
 strict = True
-- 
2.26.2

[PATCH 3/5] python: add Makefile for some common tasks

[John Snow]

Add "make venv" to create the pipenv-managed virtual environment that
contains our explicitly pinned dependencies.

Add "make check" to run the python linters [in the host execution
environment].

Add "make venv-check" which combines the above two: create/update the
venv, then run the linters in that explicitly managed environment.

make clean: delete miscellaneous build output possibly created by
pipenv, pip, or other python packaging utilities

make distclean: delete the above, and the .venv, too.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Makefile | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 python/Makefile

diff --git a/python/Makefile b/python/Makefile
new file mode 100644
index 000000000000..51915eaec3bc
--- /dev/null
+++ b/python/Makefile
@@ -0,0 +1,35 @@
+.PHONY: help venv venv-check check clean distclean
+
+help:
+	@echo "python packaging help:"
+	@echo ""
+	@echo "make venv:       Create pipenv's virtual environment."
+	@echo "    NOTE: Requires Python 3.6 and pipenv."
+	@echo "          Will download packages from PyPI."
+	@echo "    HINT: On Fedora: 'sudo dnf install python36 pipenv'"
+	@echo ""
+	@echo "make venv-check: run linters using pipenv's virtual environment."
+	@echo ""
+	@echo "make check:      run linters using the current environment."
+	@echo "    Hint: Install deps with: 'pip install \".[devel]\"'"
+	@echo ""
+	@echo "make clean:      remove build output."
+	@echo ""
+	@echo "make distclean:  remove venv files and everything from 'clean'."
+
+
+venv: .venv
+.venv: Pipfile.lock
+	@PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
+
+venv-check: venv
+	@pipenv run make check
+
+check:
+	@pytest
+
+clean:
+	rm -rf build/ dist/
+
+distclean: clean
+	rm -rf qemu.egg.info/ .venv
-- 
2.26.2

[PATCH 4/5] python: add .gitignore

[John Snow]

Ignore build and package output (build, dist, qemu.egg-info);
effectively these are "in-tree" builds of a kind.

Ignore miscellaneous cached python confetti (__pycache__, *.pyc,
.mypy_cache).

Ignore .idea (pycharm) and .venv (pipenv et al).

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/.gitignore | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 python/.gitignore

diff --git a/python/.gitignore b/python/.gitignore
new file mode 100644
index 000000000000..78c522768bc1
--- /dev/null
+++ b/python/.gitignore
@@ -0,0 +1,9 @@
+*.pyc
+.idea/
+.mypy_cache/
+.pytest_cache/
+.venv/
+__pycache__/
+build/
+dist/
+qemu.egg-info/
-- 
2.26.2

[PATCH 5/5] gitlab: add python linters to CI

[John Snow]

Add python3.6 to the fedora container image: we need it to run the
linters against that explicit version to make sure we don't break our
minimum version promise. Add pipenv, too.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 .gitlab-ci.yml                         | 10 ++++++++++
 tests/docker/dockerfiles/fedora.docker |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 5d6773efd291..1a32e8b9ba83 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -413,6 +413,16 @@ check-patch:
     GIT_DEPTH: 1000
   allow_failure: true
 
+
+check-python:
+  stage: build
+  image: $CI_REGISTRY_IMAGE/qemu/fedora:latest
+  script:
+    - cd python
+    - make venv-check
+  variables:
+    GIT_DEPTH: 1000
+
 check-dco:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 0b5053f2d090..e065fcda8ff2 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -81,6 +81,7 @@ ENV PACKAGES \
     numactl-devel \
     perl \
     perl-Test-Harness \
+    pipenv \
     pixman-devel \
     python3 \
     python3-PyYAML \
@@ -90,6 +91,7 @@ ENV PACKAGES \
     python3-pip \
     python3-sphinx \
     python3-virtualenv \
+    python3.6 \
     rdma-core-devel \
     SDL2-devel \
     snappy-devel \
-- 
2.26.2

Re: [PATCH 1/5] python: add pytest and tests

[Thomas Huth]

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

Re: [PATCH 4/5] python: add .gitignore

[Thomas Huth]

On 27/10/2020 23.38, John Snow wrote:
> Ignore build and package output (build, dist, qemu.egg-info);
> effectively these are "in-tree" builds of a kind.

Since we recently moved away from in-tree builds, could these also be made
out-of-tree only?

 Thomas

Re: [PATCH 2/5] python: add excluded dirs to flake8 config

[Philippe Mathieu-Daudé]

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
> 

Re: [PATCH 4/5] python: add .gitignore

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 27/10/2020 23.38, John Snow wrote:
>> Ignore build and package output (build, dist, qemu.egg-info);
>> effectively these are "in-tree" builds of a kind.
>
> Since we recently moved away from in-tree builds, could these also be made
> out-of-tree only?

The build should only write to the per-build spaces: the build tree,
per-build scratch in /tmp, ...  Writing to shared space such as the
source tree can break parallel independent builds.  I consider that a
bug.

Re: [PATCH 4/5] python: add .gitignore

[Daniel P. Berrangé]

On Wed, Oct 28, 2020 at 10:16:33AM +0100, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
> > On 27/10/2020 23.38, John Snow wrote:
> >> Ignore build and package output (build, dist, qemu.egg-info);
> >> effectively these are "in-tree" builds of a kind.
> >
> > Since we recently moved away from in-tree builds, could these also be made
> > out-of-tree only?
> 
> The build should only write to the per-build spaces: the build tree,
> per-build scratch in /tmp, ...  Writing to shared space such as the
> source tree can break parallel independent builds.  I consider that a
> bug.

Or worse it will simply fail when contributors have the source tree
as a read-only filesystem.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/5] python: add pytest and tests

[John Snow]

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

Re: [PATCH 4/5] python: add .gitignore

[John Snow]

On 10/28/20 4:13 AM, Thomas Huth wrote:
> On 27/10/2020 23.38, John Snow wrote:
>> Ignore build and package output (build, dist, qemu.egg-info);
>> effectively these are "in-tree" builds of a kind.
> 
> Since we recently moved away from in-tree builds, could these also be made
> out-of-tree only?
> 
>   Thomas
> 

I'm not sure to be really honest with you.

For "developer installs", I think the answer is *no*, it has to be 
in-tree. Basically you are installing this directory as a living 
package, as the live copy. It adds some metadata to the folder to do 
that. No way around it.

I'll investigate, but I have doubts.

--js

Re: [PATCH 4/5] python: add .gitignore

[John Snow]

On 10/28/20 5:16 AM, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 27/10/2020 23.38, John Snow wrote:
>>> Ignore build and package output (build, dist, qemu.egg-info);
>>> effectively these are "in-tree" builds of a kind.
>>
>> Since we recently moved away from in-tree builds, could these also be made
>> out-of-tree only?
> 
> The build should only write to the per-build spaces: the build tree,
> per-build scratch in /tmp, ...  Writing to shared space such as the
> source tree can break parallel independent builds.  I consider that a
> bug.
> 

It's not really a "build" in that traditional sense, but if you were to 
execute "make venv-check" in parallel, I'm not confident it would work 
right. Don't do that, I guess.

This has nothing to do with QEMU's build step. We don't need to "build" 
or "install" this package to use it during QEMU builds or (most) 
testing. We *do* need to install it to a virtual environment to test it 
with an explicit set of linter packages, though.

See also: why do we ignore *.pyc and __pycache__ files in the whole 
tree? These are in effect build artifacts too. I'm not sure I would know 
how to avoid those being created. Maybe it's possible? but... I don't 
think this is a problem that we have to solve, actually.

OK, all that whining aside, I will give it a legitimate try. I just 
wanted to prepare you for disappointment. I might be able to move build/ 
and dist/, but I have doubts that anything can reasonably be done about 
qemu.egg-info, __pycache__, .mypy_cache, or the like.

--js

Re: [PATCH 4/5] python: add .gitignore

[Daniel P. Berrangé]

On Wed, Oct 28, 2020 at 09:39:07AM -0400, John Snow wrote:
> On 10/28/20 5:16 AM, Markus Armbruster wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> > 
> > > On 27/10/2020 23.38, John Snow wrote:
> > > > Ignore build and package output (build, dist, qemu.egg-info);
> > > > effectively these are "in-tree" builds of a kind.
> > > 
> > > Since we recently moved away from in-tree builds, could these also be made
> > > out-of-tree only?
> > 
> > The build should only write to the per-build spaces: the build tree,
> > per-build scratch in /tmp, ...  Writing to shared space such as the
> > source tree can break parallel independent builds.  I consider that a
> > bug.
> > 
> 
> It's not really a "build" in that traditional sense, but if you were to
> execute "make venv-check" in parallel, I'm not confident it would work
> right. Don't do that, I guess.
> 
> This has nothing to do with QEMU's build step. We don't need to "build" or
> "install" this package to use it during QEMU builds or (most) testing. We
> *do* need to install it to a virtual environment to test it with an explicit
> set of linter packages, though.
> 
> See also: why do we ignore *.pyc and __pycache__ files in the whole tree?
> These are in effect build artifacts too. I'm not sure I would know how to
> avoid those being created. Maybe it's possible? but... I don't think this is
> a problem that we have to solve, actually.

You can disable pyc files with

  export PYTHONDONTWRITEBYTECODE=dontmesswithmysourcedir

https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/5] python: add excluded dirs to flake8 config

[John Snow]

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

Re: [PATCH 1/5] python: add pytest and tests

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 1/5] python: add pytest and tests

[Thomas Huth]

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

Re: [PATCH 4/5] python: add .gitignore

[John Snow]

On 10/28/20 4:13 AM, Thomas Huth wrote:
> On 27/10/2020 23.38, John Snow wrote:
>> Ignore build and package output (build, dist, qemu.egg-info);
>> effectively these are "in-tree" builds of a kind.
> 
> Since we recently moved away from in-tree builds, could these also be made
> out-of-tree only?
> 
>   Thomas
> 

# Summary

This wound up being controversial, so let's go over exactly what 
artifacts I have ignored here, and why I have done so. This email gets 
long, but please stick with me for at least this first Summary section.

One thing I would like to immediately clear up is that these artifacts 
are not related to a "QEMU build" in any way, shape or form; i.e., 
building QEMU does not cause these artifacts to exist.

For clarity: neither "make" nor "make check" from the root nor the build 
root will cause any changes to the ./python source tree. These artifacts 
come from explicit actions taken during manual testing/development in 
the ./python directory.

I use a Makefile in the ./python directory to canonize the expected 
invocations for a few common operations I anticipate developers wanting 
to take; these actions occur outside of "the QEMU build".


Those actions, and the expected artifacts they create, are:

- Running 'make check' inside ./python; AKA
   running 'pytest' (no arguments.)
   (.mypy_cache, .pytest_cache, __pycache__, possibly *.pyc)

- Running 'make venv' inside ./python; AKA
   running PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
   (.venv, qemu.egg-info/, build/)

- Running 'make venv-check' inside ./python; AKA
   running 'make venv; pipenv run make check'
   (All of the above from both categories)

- Running various PyPI distribution commands; not in the Makefile
   (e.g. python3 setup.py sdist bdist_wheel)
   (build/, dist/)

- Using the Pycharm IDE to edit, run, or test python files:
   (.idea/)


While these do not happen during a (QEMU) make/make check, they DO 
happen as a result of my gitlab job, which simply does the dumbest 
possible thing and navigates to the source ./python directory and runs 
"make venv-check".

This was presumed "safe", because the gitlab output is not committed 
back into the container, does not affect the subsequent build, and has 
its output discarded after the job completes.

Read on below for more detailed information on what each file is, what 
creates it, what it's useful for, and why you might come to have these 
files in your source tree during development.



# make check

running "make check" here simply invokes "pytest". This causes 
.pytest_cache to exist where you ran it. The tests located in tests/ 
will run mypy (.mypy_cache), flake8, pylint and isort.

The act of running or importing files here may or may not cause 
__pycache__ and *.pyc files to exist based on the caller's environment.

All of the files created by running the linter are normal confetti you 
might expect to incur from running such programs; i.e. if you run mypy 
or pytest separately, of your own volition, you will encounter these 
files being made for you.

It is normal and expected for developers to want to run the linters 
during development to ensure adherence to the coding standards, so it is 
normal and expected to see these files being created in the source tree 
-- not during a build, but during development.

We ignore __pycache__ and *.pyc in the root tree too, this is just 
extending the concept to more python tooling that may litter the tree.


# make venv

This is an alias for "pipenv sync --dev", which uses the pipenv tool to 
actualize a very specific virtual environment based on the contents in 
Pipfile.lock.

It normally creates a virtual environment somewhere like:
  /home/jsnow/.local/share/virtualenvs/python-Z09Et8eW

but you can configure it to look in $CWD/.venv instead. There are no 
other options I am aware of! Your $CWD must include the Pipfile.lock file.

Let's assume that we will want to create a venv in two distinct contexts:

(1) For the purposes of iterative development; as an object we want to 
keep long-term (conjured explicitly by the developer), and

(2) For the purposes of a continuous integration check; a build check, 
or some other automatic invocation not explicitly conjured by the developer.

For the first, it's okay to store in ~/.local/share/ etc, and it's okay 
to store it in the source tree too -- in both cases, you'll have one 
venv per source tree. No problem. (And reminder, because this python 
virtual environment is tied to the python *source* and has no 
relationship whatsoever to a configuration of a QEMU build, a 1:1 
relationship from venv:source is OK. We do not need (or want) a 1:1 
relationship with configurations.)

For the second, it would be best if we didn't taint the user's 
environment. In this case, constraining it to where the the venv was 
requested is the best choice available. There are no hooks in this patch 
series that will invoke the creation of this venv in the source tree 
when a user types "make" or "make check" in their root or their build root.

So: ignoring .venv is for the sake of the python developer, but does not 
imply this file will exist for those not developing python.

Lastly, The qemu.egg-info file is an artifact of installing the source 
package itself into the virtual environment in "editable" mode. This is 
an expected artifact of development, but won't occur during a build.

A trick I explicitly support is `cd ~/src/qemu/python; pip install -e .` 
-- this installs a python package to your current environment (wherever 
it is; It can be one of your explicit creation) that simply installs 
symlinks to your git source tree -- this is a feature -- so that while 
updating the python source, your installed package always reflects the 
latest version without having to "re-install" the package to test it.

Therefore, it's reasonable and expected to have a 'qemu.egg-info/' 
folder in your source tree: it means you've installed this package 
somewhere. This doesn't happen by accident!


# make venv-check

This is a hook that runs both things: it creates a venv, then runs the 
tests inside that venv. It will create all of the confetti from both 
above processes.

If you made a build directory and configured QEMU, the configuration 
script will have copied the python directory into the build directory. 
You can run "make venv-check" from in there to avoid disturbing your 
source tree.

However, since running configure isn't necessary to check the python 
code, you can just run it straight from the source tree.


# .idea

This is the pycharm IDE folder for managing project settings. If you use 
pycharm to edit the Python code (and I encourage you to try if you wish 
to contribute Python code), you will see this directory created.


# build

This directory will be created as a result of engaging with pipenv.

It will show up under any of the following:

pipenv sync
pipenv lock
pipenv install

I believe this happens as a side-effect of installing the qemu package 
in editable mode, however:

`pip install -e .` does not create such a directory. I'm not sure which 
step is creating it and what it's used for, to be crassly honest. It 
appears safe to delete afterwards.

Actually, you can even create a read-only build directory owned by root, 
and pipenv will ... work the same, and never write into that directory. 
What's going on? Someone else's bug, I guess.

(It also shows up if you were to type "python3 setup.py bdist_wheel", 
which is a packaging step you would only type if you were preparing to 
build and package this code to upload it to PyPI.)


# dist

Only shows up if you type 'python3 setup.py sdist'. No reason to do this 
unless you're building a source distribution of the Python package alone 
to upload to somewhere else. Intentional developer action.


So ... long story short: I think these files are legitimate; I am sorry 
to have alarmed people by calling it an "in-tree build", they're not 
related to the QEMU build.

--js

Re: [PATCH 1/5] python: add pytest and tests

[John Snow]

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