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

[Petr Vorel <pvorel@suse.cz>]

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 :).

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

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