[LTP] [PATCH v3 2/4] doc/Makefile: Allow to create and use .venv

[Andrea Cervesato]

Hi Petr,

On 3/25/25 10:48, Petr Vorel wrote:
> Hi Andrea,
>
> first, thanks for your review.
>
>> Hi,
>> On 3/25/25 00:40, Petr Vorel wrote:
>>> This is an optional target (not run by default).
>>> If .venv exists, it's used in other targets.
>>> This helps to use virtualenv for development, but avoid using it by
>>> default (readthedoc uses container with virtualenv, creating it would be
>>> waste of time).
>>> Add 'distclean' target which removes also .venv/ directory.
>>> Signed-off-by: Petr Vorel <pvorel@suse.cz>
>>> ---
>>> Changes since v2:
>>> * Add distclean in "doc/Makefile: Allow to create and use .venv"
>>>    doc/Makefile | 19 +++++++++++++++++--
>>>    1 file changed, 17 insertions(+), 2 deletions(-)
>>> diff --git a/doc/Makefile b/doc/Makefile
>>> index 3c5682ad00..3b8265d88e 100644
>>> --- a/doc/Makefile
>>> +++ b/doc/Makefile
>>> @@ -5,15 +5,30 @@ top_srcdir		?= ..
>>>    include $(top_srcdir)/include/mk/env_pre.mk
>>> +PYTHON := python3
>>> +VENV_DIR := .venv
>>> +VENV_CMD := . $(VENV_DIR)/bin/activate
>> This will cut off all shells not supporting "activate" script, such as fish
>> or csh.
> I would guess csh is not much used nowadays in Linux. I would dare to say bash
> is much more popular than modern fish (I know more people using zsh), but sure,
> how about:
>
> VENV_CMD := . $(VENV_DIR)/bin/activate || . venv/bin/activate.fish
>
> Or even add also activate.csh if you think people are using it.
> I would ignore the rest of activate.* scripts
Yeah, somehow we need to handle those shells.
>> Quite possible this Makefile would work only on CI, since developers often
>> customize their own shells, unless you override VENV_CMD. And, in that case,
>> virtualenv creation is 1 command away and it makes this Makefile feature
>> superfluous.
> I would say in with the above it will work for most of the users (I have heavily
> customised shell and venv always worked) and yes, it allows to overwrite.
>
> make VENV_CMD=". .venv/bin/activate.csh"
>
>>> +RUN_VENV := if [ -d $(VENV_DIR) ]; then $(VENV_CMD); fi
>>> +
>>> +# install sphinx only if needed
>>> +INSTALL_SPHINX := $(shell $(PYTHON) -c "import sphinx" 2>/dev/null && echo ":" || echo "pip install sphinx")
>> This can be added to requirements.txt, there's no need to handle it in
>> Makefile.
> I added this as it speeds up the installation. Sure, I can remove it.
>
>>> +
>>> +$(VENV_DIR):
>> "setup" stage is more clear than using virtualenv directory name.
> Using directory name gives us detection if it's needed for free. You complained
> about Makefile getting complicated, using setup would complicate it more. Do you
> want that?
"make setup" is not complicated. Fixing command based on the folder name 
is: none cares how the virtualenv folder is named and none really wants 
to remember how it's called.
If the goal is to keep things simple, exposing Makefile implementations 
to the users sounds weird and counterintuitive.

Andrea