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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 24 Jun 2021, at 00:33, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxx>
>> ---
>> .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 
>> -------------------------------------------------