Lint: call tools inside venv using make's PYTHON #2408
Conversation
Co-authored-by: Brett Cannon <brett@python.org>
| venv: | ||
| @if [ -d $(VENVDIR) ] ; then \ | ||
| echo "venv already exists."; \ | ||
| echo "To recreate it, remove it first with \`make clean-venv'."; \ | ||
| else \ | ||
| $(PYTHON) -m venv $(VENVDIR); \ | ||
| $(VENVDIR)/bin/python3 -m pip install -r requirements.txt; \ | ||
| echo "The venv has been created in the $(VENVDIR) directory"; \ | ||
| fi |
There was a problem hiding this comment.
This sort of goes against the point of make as I understand it -- if the name exists on the filesystem then make won't run the target, whereas we need a lot of logic here to do the same thing and support customisation of the venv directory.
I really don't want to overcomplicate this -- whilst I have a desire to make usability better for PEP authors, no-one complained up until now, and so we should balance usability to PEP authors for whom the current Makefile doesn't work and maintainability to ourselves.
A
There was a problem hiding this comment.
Yep, make is
10000
primarily for building/compiling files but this sort of thing is pretty common too.
This one is based on the CPython docs Makefile:
venv:
@if [ -d $(VENVDIR) ] ; then \
echo "venv already exists."; \
echo "To recreate it, remove it first with \`make clean-venv'."; \
else \
$(PYTHON) -m venv $(VENVDIR); \
$(VENVDIR)/bin/python3 -m pip install -U pip setuptools; \
$(VENVDIR)/bin/python3 -m pip install -r requirements.txt; \
echo "The venv has been created in the $(VENVDIR) directory"; \
fiThe devguide's Makefile is similar:
venv:
$(PYTHON) -m venv venv
./venv/bin/python3 -m pip install --upgrade pip
./venv/bin/python3 -m pip install -r requirements.txtI'm sure I've seen similar in other https://github.com/python Makefiles, so using venvs is a fairy common pattern.
There was a problem hiding this comment.
I'm not a make expert, but as a general principle, I don't think we should let purity override practicality here, particularly when it comes to usability for PEP authors and contributors. We certainly want to reduce friction there as much as practical, and given all the changes lately and the remaining ones we want to do, making things easier rather than harder for users helps all that go more smoothly for everyone.
| JOBS=8 | ||
| RENDER_COMMAND=$(PYTHON) build.py -j $(JOBS) | ||
| RENDER_COMMAND=$(VENVDIR)/bin/python3 build.py -j $(JOBS) |
There was a problem hiding this comment.
If one installs make on windows, this would fail, right? As the executables directory is Scripts.
A
There was a problem hiding this comment.
I don't use make on Windows, I've often seen a separate make.bat though so I think that is used instead of Makefile.
There was a problem hiding this comment.
@AA-Turner correct, this is Unix-only. The PYTHON variable could be updated to use VENVDIR, but that would also allow for overriding the path to the Python executable.
There was a problem hiding this comment.
I do have make on my Windows dev machines and use it when necessary sometimes, but at least on this repo I don't really do so, and instead run the various commands I need directly (usually with custom invocations). I wouldn't expect very many if any non-WSL Windows users to do so either.
There was a problem hiding this comment.
I'm not a make expert, but this LGTM aside from one not directly related concern. Thanks, @hugovk
For #2402.
Fixes #2403.
The pre-commit tool can be called as a module as
pre_commit: pre-commit/pre-commit#1627 (comment)This allows us to choose which Python version to use:
And the tools are installed inside a venv using the given Python version.
This is based on the CPython docs
makefileand devguidemakefile, so should follow a familiar pattern.