Re: [PATCH v6 22/25] python: add Makefile for some common tasks

[Cleber Rosa]

On Tue, May 25, 2021 at 03:45:26PM -0400, John Snow wrote:
> On 5/25/21 3:24 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:38PM -0400, John Snow wrote:
> > > 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.
> > > 
> > > Add "make develop" which canonizes the runes needed to get both the
> > > linting pre-requisites (the "[devel]" part), and the editable
> > > live-install (the "-e" part) of these python libraries.
> > > 
> > > make clean: delete miscellaneous python packaging output possibly
> > > created by pipenv, pip, or other python packaging utilities
> > > 
> > > make distclean: delete the above, the .venv, and the editable "qemu"
> > > package forwarder (qemu.egg-info) if there is one.
> > > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   python/README.rst |  3 +++
> > >   python/Makefile   | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 45 insertions(+)
> > >   create mode 100644 python/Makefile
> > > 
> > > diff --git a/python/README.rst b/python/README.rst
> > > index e107bd12a69..3e09d20c23c 100644
> > > --- a/python/README.rst
> > > +++ b/python/README.rst
> > > @@ -35,6 +35,9 @@ Files in this directory
> > >   - ``qemu/`` Python package source directory.
> > >   - ``tests/`` Python package tests directory.
> > >   - ``avocado.cfg`` Configuration for the Avocado test-runner.
> > > +  Used by ``make check`` et al.
> > > +- ``Makefile`` provides some common testing/installation invocations.
> > > +  Try ``make help`` to see available targets.
> > >   - ``MANIFEST.in`` is read by python setuptools, it specifies additional 
> > > files
> > >     that should be included by a source distribution.
> > >   - ``PACKAGE.rst`` is used as the README file that is visible on 
> > > PyPI.org.
> > > diff --git a/python/Makefile b/python/Makefile
> > > new file mode 100644
> > > index 00000000000..184f59e5634
> > > --- /dev/null
> > > +++ b/python/Makefile
> > > @@ -0,0 +1,42 @@
> > > +.PHONY: help venv venv-check check clean distclean develop
> > > +
> > > +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 "    Hint: If you don't know which test to run, run this one!"
> > > + @echo ""
> > > + @echo "make develop:    Install deps for 'make check', and"
> > > + @echo "                 the qemu libs in editable/development mode."
> > > + @echo ""
> > > + @echo "make check:      run linters using the current environment."
> > > + @echo ""
> > 
> > Let's observe how this will be used (or misused).  I fear most people
> > will jump into `make check`, even though you have described `make
> > venv-check` as the primary choice.
> > 
> > We have a precedent with `make check-acceptance` that will create a
> > venv and use it by default, so we can consider that as a fallback
> > strategy based on user feedback.
> > 
> 
> Right, I see. Though, I did intentionally want to make it clear which of
> these invocations created an environment and which did not.
> 
> Unlike the acceptance tests, it might make sense to run these tests both
> inside and outside of that venv, so I opted to make the default "make"
> target "make help".
> 
> The Gitlab CI will run the right one, after all -- and I do still expect the
> regular 'make check' to pass, so I am not as sure that it's a crucial
> failure if someone runs the "wrong one".
> 
> > > + @echo "make clean:      remove build output."
> > > + @echo ""
> > > + @echo "make distclean:  remove venv files, qemu package forwarder, and"
> > > + @echo "                 everything from 'make clean'."
> > > +
> > > +venv: .venv
> > > +.venv: Pipfile.lock
> > > + @PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
> > > + @touch .venv
> > > +
> > > +venv-check: venv
> > > + @pipenv run make check
> > > +
> > > +develop:
> > > + pip3 install -e .[devel]
> > > +
> > > +check:
> > > + @avocado --config avocado.cfg run tests/
> > > +
> > > +clean:
> > > + rm -rf build/ dist/
> > > +
> > 
> > Usually `python3 setup.py clean --all` would be the better choice here,
> > but, it doesn't clean `dist/`, so I'm OK with this.
> > 
> 
> Hm, I should probably move the 'dist' down into 'distclean' anyway, and I
> will replace the clean invocation with the one you suggest.
>

OK then, makes sense to me.

- Cleber.

signature.asc
Description: PGP signature