Skip to content

Lint: call tools inside venv using make's PYTHON - #2408

Merged
JelleZijlstra merged 3 commits into
python:mainfrom
hugovk:make-pre-commit-as-module
Mar 12, 2022
Merged

JelleZijlstra merged 3 commits into
python:mainfrom
hugovk:make-pre-commit-as-module

Conversation

@hugovk

@hugovk hugovk commented Mar 10, 2022 •

Copy link
Copy Markdown
Member

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:

make lint
make PYTHON=python3.7 lint
make PYTHON=/Users/huvankem/.pyenv/shims/python3.8 lint

And the tools are installed inside a venv using the given Python version.

This is based on the CPython docs makefile and devguide makefile, so should follow a familiar pattern.

@hugovk hugovk added the lint Linter-related work and linting fixes on PEPs label Mar 10, 2022
@hugovk
hugovk requested a review from AA-Turner as a code owner March 10, 2022 11:42
@hugovk hugovk changed the title Lint: call pre-commit as a module using make's PYTHON Lint: call tools inside venv using make's PYTHON Mar 10, 2022
@hugovk
hugovk requested a review from CAM-Gerlach March 10, 2022 12:10
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Co-authored-by: Brett Cannon <brett@python.org>
Comment thread Makefile
Comment on lines +29 to +37
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

@AA-Turner AA-Turner Mar 10, 2022 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, make is 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"; \
	fi

The 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.txt

I'm sure I've seen similar in other https://github.com/python Makefiles, so using venvs is a fairy common pattern.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Makefile
VENVDIR=.venv
JOBS=8
RENDER_COMMAND=$(PYTHON) build.py -j $(JOBS)
RENDER_COMMAND=$(VENVDIR)/bin/python3 build.py -j $(JOBS)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one installs make on windows, this would fail, right? As the executables directory is Scripts.

A

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a make expert, but this LGTM aside from one not directly related concern. Thanks, @hugovk

@AA-Turner AA-Turner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a domain expert, so leaving this to someone else to merge!

A

@JelleZijlstra
JelleZijlstra self-requested a review March 12, 2022 02:28
@JelleZijlstra
JelleZijlstra merged commit 00a5a0b into python:main Mar 12, 2022
@hugovk
hugovk deleted the make-pre-commit-as-module branch March 12, 2022 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA signed lint Linter-related work and linting fixes on PEPs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make targets don't use venv

6 participants