* [PATCH 0/5] python: add linters to gitlab CI @ 2020-10-27 22:38 John Snow 2020-10-27 22:38 ` [PATCH 1/5] python: add pytest and tests John Snow ` (4 more replies) 0 siblings, 5 replies; 20+ messages in thread From: John Snow @ 2020-10-27 22:38 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Alex Bennée, Cleber Rosa, 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/5] python: add pytest and tests 2020-10-27 22:38 [PATCH 0/5] python: add linters to gitlab CI John Snow @ 2020-10-27 22:38 ` John Snow 2020-10-28 6:19 ` Thomas Huth 2020-10-27 22:38 ` [PATCH 2/5] python: add excluded dirs to flake8 config John Snow ` (3 subsequent siblings) 4 siblings, 1 reply; 20+ messages in thread From: John Snow @ 2020-10-27 22:38 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Alex Bennée, Cleber Rosa, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] python: add pytest and tests 2020-10-27 22:38 ` [PATCH 1/5] python: add pytest and tests John Snow @ 2020-10-28 6:19 ` Thomas Huth 2020-10-28 13:23 ` John Snow 0 siblings, 1 reply; 20+ messages in thread From: Thomas Huth @ 2020-10-28 6:19 UTC (permalink / raw) To: John Snow, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Alex Bennée, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] python: add pytest and tests 2020-10-28 6:19 ` Thomas Huth @ 2020-10-28 13:23 ` John Snow 2020-10-28 14:09 ` Philippe Mathieu-Daudé 2020-10-28 14:54 ` Thomas Huth 0 siblings, 2 replies; 20+ messages in thread From: John Snow @ 2020-10-28 13:23 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Alex Bennée, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] python: add pytest and tests 2020-10-28 13:23 ` John Snow @ 2020-10-28 14:09 ` Philippe Mathieu-Daudé 2020-10-28 14:54 ` Thomas Huth 1 sibling, 0 replies; 20+ messages in thread From: Philippe Mathieu-Daudé @ 2020-10-28 14:09 UTC (permalink / raw) To: John Snow, Thomas Huth, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Alex Bennée 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> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] python: add pytest and tests 2020-10-28 13:23 ` John Snow 2020-10-28 14:09 ` Philippe Mathieu-Daudé @ 2020-10-28 14:54 ` Thomas Huth 2020-10-28 18:38 ` John Snow 1 sibling, 1 reply; 20+ messages in thread From: Thomas Huth @ 2020-10-28 14:54 UTC (permalink / raw) To: John Snow, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Alex Bennée 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] python: add pytest and tests 2020-10-28 14:54 ` Thomas Huth @ 2020-10-28 18:38 ` John Snow 0 siblings, 0 replies; 20+ messages in thread From: John Snow @ 2020-10-28 18:38 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Alex Bennée 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] python: add excluded dirs to flake8 config 2020-10-27 22:38 [PATCH 0/5] python: add linters to gitlab CI John Snow 2020-10-27 22:38 ` [PATCH 1/5] python: add pytest and tests John Snow @ 2020-10-27 22:38 ` John Snow 2020-10-28 8:50 ` Philippe Mathieu-Daudé 2020-10-27 22:38 ` [PATCH 3/5] python: add Makefile for some common tasks John Snow ` (2 subsequent siblings) 4 siblings, 1 reply; 20+ messages in thread From: John Snow @ 2020-10-27 22:38 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Alex Bennée, Cleber Rosa, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] python: add excluded dirs to flake8 config 2020-10-27 22:38 ` [PATCH 2/5] python: add excluded dirs to flake8 config John Snow @ 2020-10-28 8:50 ` Philippe Mathieu-Daudé 2020-10-28 13:42 ` John Snow 0 siblings, 1 reply; 20+ messages in thread From: Philippe Mathieu-Daudé @ 2020-10-28 8:50 UTC (permalink / raw) To: John Snow, qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Alex Bennée 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 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] python: add excluded dirs to flake8 config 2020-10-28 8:50 ` Philippe Mathieu-Daudé @ 2020-10-28 13:42 ` John Snow 0 siblings, 0 replies; 20+ messages in thread From: John Snow @ 2020-10-28 13:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Alex Bennée 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 >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] python: add Makefile for some common tasks 2020-10-27 22:38 [PATCH 0/5] python: add linters to gitlab CI John Snow 2020-10-27 22:38 ` [PATCH 1/5] python: add pytest and tests John Snow 2020-10-27 22:38 ` [PATCH 2/5] python: add excluded dirs to flake8 config John Snow @ 2020-10-27 22:38 ` John Snow 2020-10-27 22:38 ` [PATCH 4/5] python: add .gitignore John Snow 2020-10-27 22:38 ` [PATCH 5/5] gitlab: add python linters to CI John Snow 4 siblings, 0 replies; 20+ messages in thread From: John Snow @ 2020-10-27 22:38 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Alex Bennée, Cleber Rosa, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/5] python: add .gitignore 2020-10-27 22:38 [PATCH 0/5] python: add linters to gitlab CI John Snow ` (2 preceding siblings ...) 2020-10-27 22:38 ` [PATCH 3/5] python: add Makefile for some common tasks John Snow @ 2020-10-27 22:38 ` John Snow 2020-10-28 8:13 ` Thomas Huth 2020-10-27 22:38 ` [PATCH 5/5] gitlab: add python linters to CI John Snow 4 siblings, 1 reply; 20+ messages in thread From: John Snow @ 2020-10-27 22:38 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Alex Bennée, Cleber Rosa, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-27 22:38 ` [PATCH 4/5] python: add .gitignore John Snow @ 2020-10-28 8:13 ` Thomas Huth 2020-10-28 9:16 ` Markus Armbruster ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Thomas Huth @ 2020-10-28 8:13 UTC (permalink / raw) To: John Snow, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Alex Bennée, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-28 8:13 ` Thomas Huth @ 2020-10-28 9:16 ` Markus Armbruster 2020-10-28 9:22 ` Daniel P. Berrangé 2020-10-28 13:39 ` John Snow 2020-10-28 13:26 ` John Snow 2020-10-28 18:33 ` John Snow 2 siblings, 2 replies; 20+ messages in thread From: Markus Armbruster @ 2020-10-28 9:16 UTC (permalink / raw) To: Thomas Huth Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Alex Bennée, qemu-devel, Wainer dos Santos Moschetta, Philippe Mathieu-Daudé, Cleber Rosa, John Snow 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-28 9:16 ` Markus Armbruster @ 2020-10-28 9:22 ` Daniel P. Berrangé 2020-10-28 13:39 ` John Snow 1 sibling, 0 replies; 20+ messages in thread From: Daniel P. Berrangé @ 2020-10-28 9:22 UTC (permalink / raw) To: Markus Armbruster Cc: Fam Zheng, Peter Maydell, Thomas Huth, Eduardo Habkost, Philippe Mathieu-Daudé, qemu-devel, Wainer dos Santos Moschetta, John Snow, Cleber Rosa, Alex Bennée 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 :| ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-28 9:16 ` Markus Armbruster 2020-10-28 9:22 ` Daniel P. Berrangé @ 2020-10-28 13:39 ` John Snow 2020-10-28 13:41 ` Daniel P. Berrangé 1 sibling, 1 reply; 20+ messages in thread From: John Snow @ 2020-10-28 13:39 UTC (permalink / raw) To: Markus Armbruster, Thomas Huth Cc: Fam Zheng, Peter Maydell, Daniel P.Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, qemu-devel, Wainer dos Santos Moschetta, Cleber Rosa, Alex Bennée 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-28 13:39 ` John Snow @ 2020-10-28 13:41 ` Daniel P. Berrangé 0 siblings, 0 replies; 20+ messages in thread From: Daniel P. Berrangé @ 2020-10-28 13:41 UTC (permalink / raw) To: John Snow Cc: Fam Zheng, Peter Maydell, Thomas Huth, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, qemu-devel, Cleber Rosa, Alex Bennée 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 :| ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-28 8:13 ` Thomas Huth 2020-10-28 9:16 ` Markus Armbruster @ 2020-10-28 13:26 ` John Snow 2020-10-28 18:33 ` John Snow 2 siblings, 0 replies; 20+ messages in thread From: John Snow @ 2020-10-28 13:26 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Alex Bennée, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] python: add .gitignore 2020-10-28 8:13 ` Thomas Huth 2020-10-28 9:16 ` Markus Armbruster 2020-10-28 13:26 ` John Snow @ 2020-10-28 18:33 ` John Snow 2 siblings, 0 replies; 20+ messages in thread From: John Snow @ 2020-10-28 18:33 UTC (permalink / raw) To: Thomas Huth, qemu-devel Cc: Fam Zheng, Peter Maydell, Daniel P . Berrangé, Eduardo Habkost, Alex Bennée, Markus Armbruster, Wainer dos Santos Moschetta, Cleber Rosa, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] gitlab: add python linters to CI 2020-10-27 22:38 [PATCH 0/5] python: add linters to gitlab CI John Snow ` (3 preceding siblings ...) 2020-10-27 22:38 ` [PATCH 4/5] python: add .gitignore John Snow @ 2020-10-27 22:38 ` John Snow 4 siblings, 0 replies; 20+ messages in thread From: John Snow @ 2020-10-27 22:38 UTC (permalink / raw) To: qemu-devel Cc: Fam Zheng, Peter Maydell, Thomas Huth, Daniel P . Berrangé, Eduardo Habkost, Philippe Mathieu-Daudé, Markus Armbruster, Wainer dos Santos Moschetta, Alex Bennée, Cleber Rosa, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-10-28 18:40 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-27 22:38 [PATCH 0/5] python: add linters to gitlab CI John Snow 2020-10-27 22:38 ` [PATCH 1/5] python: add pytest and tests John Snow 2020-10-28 6:19 ` Thomas Huth 2020-10-28 13:23 ` John Snow 2020-10-28 14:09 ` Philippe Mathieu-Daudé 2020-10-28 14:54 ` Thomas Huth 2020-10-28 18:38 ` John Snow 2020-10-27 22:38 ` [PATCH 2/5] python: add excluded dirs to flake8 config John Snow 2020-10-28 8:50 ` Philippe Mathieu-Daudé 2020-10-28 13:42 ` John Snow 2020-10-27 22:38 ` [PATCH 3/5] python: add Makefile for some common tasks John Snow 2020-10-27 22:38 ` [PATCH 4/5] python: add .gitignore John Snow 2020-10-28 8:13 ` Thomas Huth 2020-10-28 9:16 ` Markus Armbruster 2020-10-28 9:22 ` Daniel P. Berrangé 2020-10-28 13:39 ` John Snow 2020-10-28 13:41 ` Daniel P. Berrangé 2020-10-28 13:26 ` John Snow 2020-10-28 18:33 ` John Snow 2020-10-27 22:38 ` [PATCH 5/5] gitlab: add python linters to CI John Snow
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.