Re: [PATCH v2 1/4] mk: Add 'make doc-pdf' target to convert guide docs to pdf

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

2015-01-30 18:13, John McNamara:
> Added make system support for building PDF versions of
> the guides. Requires Python Sphinx and TexLive Full.
> 
> Signed-off-by: John McNamara <john.mcnamara-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

>  ifndef V
>  RTE_SPHINX_VERBOSE := -q
> +RTE_PDFLATEX_VERBOSE := > /dev/null 2>&1

By redirecting everything in /dev/null, you won't see any error.
But pdflatex is an horrible software.
If I remember well, it's very difficult to filter only errors.
You could try "-interaction batchmode" for quiet mode and
"-interaction nonstopmode" for verbose mode.

> +DEFAULT_DPI ?= 300

It's better to prefix this global variable with RTE_ or DPDK_

> -clean: api-html-clean guides-html-clean
> +clean: api-html-clean guides-html-clean guides-latex-clean

Why not guides-pdf-clean?

> +pdf: pdf-rel_notes pdf-linux_gsg pdf-freebsd_gsg pdf-prog_guide \
> +	pdf-sample_app_ug pdf-testpmd_app_ug

In general, it's a bad idea to create virtual targets where it could
be a file. Example: the real file $(RTE_OUTPUT)/doc/.../rel_notes.pdf
would be a better target than pdf-rel_notes.
But maybe it's too difficult to have the full dependency chain to make
it able of recompiling only needed parts when needed.

> +pdf-%:
> +	@echo 'creating' $* 'pdf ...'
> +        # Generate the latex files.
> +	$(Q)$(RTE_SPHINX_BUILD) -b latex $(RTE_SPHINX_VERBOSE) \
> +		-c $(RTE_SDK)/doc/guides/$*  $(RTE_SDK)/doc/guides/$* \
> +		$(RTE_OUTPUT)/doc/latex/guides/$*
> +
> +        # Convert the svg files to png.
> +	$(eval svg_files=$(notdir $(wildcard $(RTE_SDK)/doc/guides/$*/img/*.svg)))
> +	$(eval svg_files=$(patsubst %.svg, %, $(svg_files)))
> +	@for svg in $(svg_files); do \

You could use $(svg_files:.svg=) instead of doing a patsubst above.

> +		inkscape -d $(DEFAULT_DPI) -D -b ffffff  \
> +			-f $(RTE_SDK)/doc/guides/$*/img/$$svg.svg \
> +			-e $(RTE_OUTPUT)/doc/latex/guides/$*/$$svg.png \
> +				$(RTE_PDFLATEX_VERBOSE); \
> +	done
> +
> +        # Change the svg image includes to png in the latex docs.
> +	$(Q)sed -i 's/\.svg/.png/g' $(RTE_OUTPUT)/doc/latex/guides/$*/*.tex

No, you could avoid that if every image references where x.* instead of x.svg or x.png.
The image extension should always be .* in rst to be format agnostic.
Then there is a (configurable) rule to choose the best format.

> +
> +        # Convert the latex to pdf.
> +	$(Q)$(MAKE) -C $(RTE_OUTPUT)/doc/latex/guides/$* \
> +		all-pdf $(RTE_PDFLATEX_VERBOSE)

Indentation is broken in this whole rule.

> --- a/mk/rte.sdkroot.mk
> +++ b/mk/rte.sdkroot.mk
> @@ -101,9 +101,10 @@ testall:
>  install uninstall:
>  	$(Q)$(MAKE) -f $(RTE_SDK)/mk/rte.sdkinstall.mk $@
>  
> -.PHONY: doc help
> +.PHONY: doc help pdf
>  doc: doc-all
>  help: doc-help
> +pdf: doc-pdf
>  doc-%:
>  	$(Q)$(MAKE) -f $(RTE_SDK)/mk/rte.sdkdoc.mk $*

Why having a pdf rule? It seems not consistent. doc-pdf is enough.

Thanks
-- 
Thomas