Re: [PATCH v6 7/9] docs: Change Makefile and sphinx configuration for doxygen

From: Luca Fancellu <luca.fancellu@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	wei.chen@arm.com, Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v6 7/9] docs: Change Makefile and sphinx configuration for doxygen
Date: Thu, 1 Jul 2021 14:36:59 +0100	[thread overview]
Message-ID: <1FC1E8DF-8AED-4ABD-BE9A-DBBD9D66EDBB@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2106231506040.24906@sstabellini-ThinkPad-T480s>

> On 24 Jun 2021, at 00:33, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 10 May 2021, Luca Fancellu wrote:
>> Modify docs/Makefile to call doxygen and generate sphinx
>> html documentation given the doxygen XML output.
>> 
>> Modify docs/conf.py sphinx configuration file to setup
>> the breathe extension that works as bridge between
>> sphinx and doxygen.
>> 
>> Add some files to the .gitignore to ignore some
>> generated files for doxygen.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> .gitignore    |  6 ++++++
>> docs/Makefile | 42 +++++++++++++++++++++++++++++++++++++++---
>> docs/conf.py  | 48 +++++++++++++++++++++++++++++++++++++++++++++---
>> 3 files changed, 90 insertions(+), 6 deletions(-)
>> 
>> diff --git a/.gitignore b/.gitignore
>> index 1c2fa1530b..d271e0ce6a 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -58,6 +58,12 @@ docs/man7/
>> docs/man8/
>> docs/pdf/
>> docs/txt/
>> +docs/doxygen-output
>> +docs/sphinx
>> +docs/xen.doxyfile
>> +docs/xen.doxyfile.tmp
>> +docs/xen-doxygen/doxygen_include.h
>> +docs/xen-doxygen/doxygen_include.h.tmp
>> extras/mini-os*
>> install/*
>> stubdom/*-minios-config.mk
>> diff --git a/docs/Makefile b/docs/Makefile
>> index 8de1efb6f5..2f784c36ce 100644
>> --- a/docs/Makefile
>> +++ b/docs/Makefile
>> @@ -17,6 +17,18 @@ TXTSRC-y := $(sort $(shell find misc -name '*.txt' -print))
>> 
>> PANDOCSRC-y := $(sort $(shell find designs/ features/ misc/ process/ specs/ \( -name '*.pandoc' -o -name '*.md' \) -print))
>> 
>> +# Directory in which the doxygen documentation is created
>> +# This must be kept in sync with breathe_projects value in conf.py
>> +DOXYGEN_OUTPUT = doxygen-output
>> +
>> +# Doxygen input headers from xen-doxygen/doxy_input.list file
>> +DOXY_LIST_SOURCES != cat "xen-doxygen/doxy_input.list"
>> +DOXY_LIST_SOURCES := $(realpath $(addprefix $(XEN_ROOT)/,$(DOXY_LIST_SOURCES)))

Hi Stefano,

> 
> I cannot find exactly who is populating doxy_input.list. I can see it is
> empty in patch #6. Does it get populated during the build?

doxy_input.list is the only file that should be modified by the developer when he/she wants to add documentation
for a new file to be parsed by Doxygen, in my patch about documenting grant_tables.h you can see I add
there the path “xen/include/public/grant_table.h"

> 
> 
>> +DOXY_DEPS := xen.doxyfile \
>> +			 xen-doxygen/mainpage.md \
>> +			 xen-doxygen/doxygen_include.h
>> +
>> # Documentation targets
>> $(foreach i,$(MAN_SECTIONS), \
>>   $(eval DOC_MAN$(i) := $(patsubst man/%.$(i),man$(i)/%.$(i), \
>> @@ -46,8 +58,28 @@ all: build
>> build: html txt pdf man-pages figs
>> 
>> .PHONY: sphinx-html
>> -sphinx-html:
>> -	sphinx-build -b html . sphinx/html
>> +sphinx-html: $(DOXY_DEPS) $(DOXY_LIST_SOURCES)
>> +ifneq ($(SPHINXBUILD),no)
> 
> This check on SPHINXBUILD is new, it wasn't there before. Why do we need
> it now? We are not really changing anything in regards to Sphinx, just
> adding Doxygen support. Or was it a mistake that it was missing even
> before this patch?

Yes this is new, I saw that we didn’t look if sphinx was installed in the system, so now we did

> 
> 
>> +	$(DOXYGEN) xen.doxyfile
>> +	XEN_ROOT=$(realpath $(XEN_ROOT)) $(SPHINXBUILD) -b html . sphinx/html
>> +else
>> +	@echo "Sphinx is not installed; skipping sphinx-html documentation."
>> +endif
>> +
>> +xen.doxyfile: xen.doxyfile.in xen-doxygen/doxy_input.list
>> +	@echo "Generating $@"
>> +	@sed -e "s,@XEN_BASE@,$(realpath $(XEN_ROOT)),g" $< \
>> +		| sed -e "s,@DOXY_OUT@,$(DOXYGEN_OUTPUT),g" > $@.tmp
>> +	@$(foreach inc,\
>> +		$(DOXY_LIST_SOURCES),\
>> +		echo "INPUT += \"$(inc)\"" >> $@.tmp; \
>> +	)
>> +	mv $@.tmp $@
>> +
>> +xen-doxygen/doxygen_include.h: xen-doxygen/doxygen_include.h.in
>> +	@echo "Generating $@"
>> +	@sed -e "s,@XEN_BASE@,$(realpath $(XEN_ROOT)),g" $< > $@.tmp
>> +	@mv $@.tmp $@
> 
> Is the absolute path required? If not, we can probably get rid of this
> generation step and simply have the relative path in
> xen-doxygen/doxygen_include.h. I think this could apply to
> xen.doxyfile.in above.

Unfortunately yes, the doxygen_include.h is a file that is included in every documented header before 
starting the doxygen parser, since we don’t have all the headers in one path, it is impossible to have here
a relative path that is good for every header in Xen.

> 
> 
>> .PHONY: html
>> html: $(DOC_HTML) html/index.html
>> @@ -71,7 +103,11 @@ clean: clean-man-pages
>> 	$(MAKE) -C figs clean
>> 	rm -rf .word_count *.aux *.dvi *.bbl *.blg *.glo *.idx *~
>> 	rm -rf *.ilg *.log *.ind *.toc *.bak *.tmp core
>> -	rm -rf html txt pdf sphinx/html
>> +	rm -rf html txt pdf sphinx $(DOXYGEN_OUTPUT)
>> +	rm -f xen.doxyfile
>> +	rm -f xen.doxyfile.tmp
>> +	rm -f xen-doxygen/doxygen_include.h
>> +	rm -f xen-doxygen/doxygen_include.h.tmp
>> 
>> .PHONY: distclean
>> distclean: clean
>> diff --git a/docs/conf.py b/docs/conf.py
>> index 50e41501db..a48de42331 100644
>> --- a/docs/conf.py
>> +++ b/docs/conf.py
>> @@ -13,13 +13,17 @@
>> # add these directories to sys.path here. If the directory is relative to the
>> # documentation root, use os.path.abspath to make it absolute, like shown here.
>> #
>> -# import os
>> -# import sys
>> +import os
>> +import sys
>> # sys.path.insert(0, os.path.abspath('.'))
>> 
>> 
>> # -- Project information -----------------------------------------------------
>> 
>> +if "XEN_ROOT" not in os.environ:
>> +    sys.exit("$XEN_ROOT environment variable undefined.")
>> +XEN_ROOT = os.path.abspath(os.environ["XEN_ROOT"])
>> +
>> project = u'Xen'
>> copyright = u'2019, The Xen development community'
>> author = u'The Xen development community'
>> @@ -35,6 +39,7 @@ try:
>>             xen_subver = line.split(u"=")[1].strip()
>>         elif line.startswith(u"export XEN_EXTRAVERSION"):
>>             xen_extra = line.split(u"=")[1].split(u"$", 1)[0].strip()
>> +
> 
> spurious change?

I think I’ve intentionally added a new line to separate the code from the except: below,
but if it is a problem I can remove it

> 
> 
>> except:
>>     pass
>> finally:
>> @@ -44,6 +49,15 @@ finally:
>>     else:
>>         version = release = u"unknown version"
>> 
>> +try:
>> +    xen_doxygen_output = None
>> +
>> +    for line in open(u"Makefile"):
>> +        if line.startswith(u"DOXYGEN_OUTPUT"):
>> +                xen_doxygen_output = line.split(u"=")[1].strip()
>> +except:
>> +    sys.exit("DOXYGEN_OUTPUT variable undefined.")
> 
> This is a bit strange: isn't there a better way to get the
> DOXYGEN_OUTPUT variable than reading the Makefile?
> 
> At that point I think it would be better to define DOXYGEN_OUTPUT a
> second time in conf.py. But maybe it could be passed as an evironmental
> variable?

Yes we could pass it as an environment variable as we do with XEN_ROOT,
I will fix it in a next release.

> 
> 
>> # -- General configuration ---------------------------------------------------
>> 
>> # If your documentation needs a minimal Sphinx version, state it here.
>> @@ -53,7 +67,8 @@ needs_sphinx = '1.4'
>> # Add any Sphinx extension module names here, as strings. They can be
>> # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
>> # ones.
>> -extensions = []
>> +# breathe -> extension that integrates doxygen xml output with sphinx
>> +extensions = ['breathe']
>> 
>> # Add any paths that contain templates here, relative to this directory.
>> templates_path = ['_templates']
>> @@ -175,6 +190,33 @@ texinfo_documents = [
>>      'Miscellaneous'),
>> ]
>> 
>> +# -- Options for Breathe extension -------------------------------------------
>> +
>> +breathe_projects = {
>> +    "Xen": "{}/docs/{}/xml".format(XEN_ROOT, xen_doxygen_output)
>> +}
>> +breathe_default_project = "Xen"
>> +
>> +breathe_domain_by_extension = {
>> +    "h": "c",
>> +    "c": "c",
>> +}
>> +breathe_separate_member_pages = True
>> +breathe_show_enumvalue_initializer = True
>> +breathe_show_define_initializer = True
>> +
>> +# Qualifiers to a function are causing Sphihx/Breathe to warn about
>> +# Error when parsing function declaration and more.  This is a list
>> +# of strings that the parser additionally should accept as
>> +# attributes.
>> +cpp_id_attributes = [
>> +    '__syscall', '__deprecated', '__may_alias',
>> +    '__used', '__unused', '__weak',
>> +    '__DEPRECATED_MACRO', 'FUNC_NORETURN',
>> +    '__subsystem',
> 
> Should we also have any of following:
> 
> __packed
> __init
> __attribute__
> __aligned__
> 
> in the list? In any case, we don't have to add them right now, we could
> add them later as we expand Doxygen coverage if they become needed.

Sure it is possible, I can add them now since I have to push a fix for this patch
If you want.

Cheers,

Luca

> 
> 
>> +]
>> +c_id_attributes = cpp_id_attributes
>> +
>> 
>> # -- Options for Epub output -------------------------------------------------