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

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Petyr,

On 3/27/25 15:57, Petr Vorel wrote:
> Hi Andrea,
>
>> 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.
> I don't have openSUSE statistics, thus using Debian popcon:
>
> * bash: 99.98%
> https://qa.debian.org/popcon.php?package=bash
>
> * zsh (compatible with bash):  7.32%
> https://qa.debian.org/popcon.php?package=zsh
>
> * fish (you're using): 1.95%
> https://qa.debian.org/popcon.php?package=fish
>
> * csh + tcsh: 0.67% + 1.91%
> https://qa.debian.org/popcon.php?package=tcsh
> https://qa.debian.org/popcon.php?package=csh
>
> I'm not really convinced anybody uses {t}csh on shell development, but ok,
> giving up and add also this support :).
Thanks for the stats. Let's keep bash only then. Default will be good.
>>>> 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.
> I'm ok to have phony target 'setup' which points to '.venv' target (less code
> than to have only setup target bug check for directory presence). I suppose you
> talk only about the make target, not about the virtualenv directory name.
>
> About the directory name: I felt '.venv' is kind of the default directory which
> is used by people for virtualenv. Looking at:
> https://codesearch.debian.net/search?q=%5C.venv&perpkg=1&page=1
> People mostly use: .venv, then venv, .venv*, venv*, .env, env, .eggs
>
> That's why it make sense to me create the directory as '.venv'. Also it's good
> that it's a hidden directory (starts with '.'). Hope you agree.
>
> Kind regards,
> Petr
>
>> Andrea
Yes I meant "make setup" target, not the virtualenv directory name. That 
can be anything and ".venv" is fine.


Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp