Only print make venv message when needed.

[ezio-melotti]

When make venv is called

• if the venv doesn't exist, it creates it and announce its creation
• if it does, it says it exists and suggests make clean-venv

This was added in #856, but because of this change, all other targets that have the venv target as a dependency now print the message saying that the venv exist.

If venv is removed from the . PHONY list it doesn't get reevaluated and doesn't print the message, but this also applies when it's invoked directly, making the message that suggests make clean-venv useless since it's never printed.

I wrestled with Makefile for a while, and this PR is the best I came up with. It's intelligible, but has some repetition. I tried to get rid of the repetition by creating a Makefile function:

define create-venv =
        $(PYTHON) -m venv $(VENVDIR)
        $(VENVDIR)/bin/python3 -m pip install --upgrade pip
        $(VENVDIR)/bin/python3 -m pip install -r requirements.txt
endef

but I couldn't find a way to call it from the other target, since they use a bash if/else that can't be used to call makefile functions.

Another option is to create a target instead of the function, and call it with make target:

create-venv:
	$(PYTHON) -m venv $(VENVDIR)
	$(VENVDIR)/bin/python3 -m pip install --upgrade pip
	$(VENVDIR)/bin/python3 -m pip install -r requirements.txt
	@echo "The venv has been created in the $(VENVDIR) directory"

venv:
	@if [ -d $(VENVDIR) ] ; then \
		echo "venv already exists."; \
		echo "To recreate it, remove it first with \`make clean-venv'."; \
	else \
		make create-venv; \
	fi

ensure-venv:
	@if [ ! -d $(VENVDIR) ] ; then \
		make create-venv; \
	fi

this works, but it launches another instance of make and prints some extra messages like make[1]: Entering/Leaving directory '/home/user/devguide' -- not very elegant.

There are other solutions that I explored, including conditional functions or conditional syntax. There are also other ways of detecting dirs and acting upon their presence/absence, but the intelligibility of all these solutions goes downhill pretty quickly.

So the options are:

1. Merge the current PR, with some duplication
2. Add the create-venv target with nested make calls
3. Find a solution that removes the duplication without looking like bad ASCII-art
4. Remove the if/else from the venv target, document make clean-venv, and hope people figure it out by reading the help

[hugovk]

The message can be useful as a prompt when the venv directory already exists, but a new tool has been added to requirements.txt and it's not yet in my venv:

$ make check
venv already exists.
To recreate it, remove it first with `make clean-venv'.
# Ignore the tools and venv dirs and check that the default role is not used.
./venv/bin/sphinx-lint -i tools -i ./venv --enable default-role
No problems found.
my-new-tool
make: my-new-tool: No such file or directory
make: *** [check] Error 1

[ezio-melotti]

Always printing that message is confusing since it says that the "venv already exists." (stating a fact) and suggests instructions to recreate it (just a tip), and this can easily be misinterpreted as a problem that must be addressed by running the suggested command. The fact that it's printed for all the commands, with no apparent connection to the venv, makes it even more confusing.

Adding new tools is a corner case that doesn't happen often. In that case the author can probably figure out that the venv needs to be updated/recreated, and make venv will explain how to do it.

[hugovk]

How about keeping ensure-venv as you have it here:

ensure-venv:
	@if [ ! -d $(VENVDIR) ] ; then \
		$(PYTHON) -m venv $(VENVDIR); \
		$(VENVDIR)/bin/python3 -m pip install --upgrade pip; \
		$(VENVDIR)/bin/python3 -m pip install -r requirements.txt; \
		echo "The venv has been created in the $(VENVDIR) directory"; \
	fi

And have venv call ensure-venve like this?

venv:
	@if [ -d $(VENVDIR) ] ; then \
		echo "venv already exists."; \
		echo "To recreate it, remove it first with \`make clean-venv'."; \
	else \
		$(MAKE) ensure-venv; \
	fi

[ezio-melotti]

Is using $(MAKE) different than using make directly?

I tried to call make directly but I decided against it since it starts another process and prints some spurious output too (see my first message)

[hugovk]

Yes:

Recursive make commands should always use the variable MAKE, not the explicit command name ‘make’

https://www.gnu.org/software/make/manual/make.html#MAKE-Variable

We use it at https://github.com/python-pillow/Pillow/blob/main/Makefile and it's widely used in https://github.com/python/cpython/blob/main/Makefile.pre.in and https://github.com/python/cpython/blob/main/Doc/Makefile.

Actually, I don't get any Entering/Leaving directory output when using either make or $(MAKE) (with GNU Make 3.81 on macOS).

[ezio-melotti]

Weird, with the code from the first message (with venv, ensure-venv, and create-venv) I get the Entering/Leaving directory, but with your suggestion (only venv and ensure-venv) I don't (using GNU Make 4.3 and either make or $(MAKE)).

Not sure why it works like this, but it seems to work. I'll run a few more tests locally and then update the PR.

[ezio-melotti]

The Entering/Leaving directory is printed whenever make is called recursively. With my initial approach both venv and ensure-venv were calling make create-venv, and since all commands were going through ensure-venv there was a recursive call and the extra output. With your approach the recursive call only happens when make call is called and the venv doesn't exist. In that case the extra output doesn't matter much, so I updated the PR to follow your suggestion.

[hugovk]

Thank you!

We could do something similar in https://github.com/python/cpython/blob/main/Doc/Makefile