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

[Petr Vorel <pvorel@suse.cz>]

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

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

FYI I expect people will just call 'make doc' in toplevel directory (see
following commits). These people will not even know about name of the .venv
target.

And people doing packaging and still want to build the documentation (IMHO
nobody currently does: SUSE, Buildroot and Yocto), it's still possible via
'make -C doc'.

Kind regards,
Petr

> > +	$(PYTHON) -m virtualenv $(VENV_DIR)
> > +	$(VENV_CMD) && pip install -r requirements.txt && $(INSTALL_SPHINX)
> > +
> >   ${abs_top_builddir}/metadata/ltp.json:
> >   	$(MAKE) -C ${abs_top_builddir}/metadata
> >   all: ${abs_top_builddir}/metadata/ltp.json
> > -	sphinx-build -b html . html
> > +	$(RUN_VENV); sphinx-build -b html . html
> >   spelling:
> > -	sphinx-build -b spelling -d build/doctree . build/spelling
> > +	$(RUN_VENV); sphinx-build -b spelling -d build/doctree . build/spelling
> >   clean:
> >   	rm -rf html/ build/ _static/syscalls.rst _static/tests.rst syscalls.tbl \
> >   		${abs_top_builddir}/metadata/ltp.json
> > +
> > +distclean: clean
> > +	rm -rf $(VENV_DIR)
> Andrea

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