Re: [PATCH v8 5/5] dts: add API doc generation

[Thomas Monjalon <thomas@monjalon.net>]

01/08/2024 15:03, Juraj Linkeš:
> On 30. 7. 2024 15:51, Thomas Monjalon wrote:
> > 12/07/2024 10:57, Juraj Linkeš:
> >> The tool used to generate DTS API docs is Sphinx, which is already in
> >> use in DPDK. The same configuration is used to preserve style with one
> >> DTS-specific configuration (so that the DPDK docs are unchanged) that
> >> modifies how the sidebar displays the content.
> > 
> > What is changed in the sidebar?
> > 
> 
> These are the two changes:
> html_theme_options = {
>      'collapse_navigation': False,
>      'navigation_depth': -1,
> }
> 
> The first allows you to explore the structure without needing to enter 
> any specific section - it puts the + at each section so everything is 
> expandable.
> The second just means that each section can be fully expanded (there's 
> no limit).

OK interesting, you may add a comment # unlimited depth


> >> +# A local reference must be relative to the main index.html page
> >> +# The path below can't be taken from the DTS meson file as that would
> >> +# require recursive subdir traversal (doc, dts, then doc again)
> > 
> > This comment is really obscure.
> 
> I guess it is. I just wanted to explain that there's not way to do this 
> without spelling out the path this way. At least I didn't find a way.
> Should I remove the comment or reword it?

May be removed I think.

> >> +cdata.set('DTS_API_MAIN_PAGE', join_paths('..', 'dts', 'html', 'index.html'))
> > 
> > Oh I think I get it:
> > 	- DTS_API_MAIN_PAGE is the Meson variable
> > 	- dts_api_main_page is the Doxygen variable
> > 
> 
> Yes, this is a way to make it work. Maybe there's something else (I'm 
> not that familiar with Doxygen), but from what I can tell, there wasn't 
> a command line option that would set a variable (passing the path form 
> Meson to Doxygen) and nothing else I found worked.
> 
> Is this solution ok? If we want to explore something else, is there 
> someone with more experience with Doxygen who could help?

Yes it's OK like that.


> >> +dts_root = environ.get('DTS_ROOT')
> > 
> > Why does it need to be passed as an environment variable?
> > Isn't it a fixed absolute path?
> 
> The path to DTS needs to be passed in some way (and added to sys.path) 
> so that Sphinx knows where the sources are in order to import them.
> 
> Do you want us to not pass the path, but just hardcode it here? I didn't 
> really think about that, maybe that could work.

I think hardcode is better here.


> >> +To build DTS API docs, install the dependencies with Poetry, then enter its shell:
> > 
> > I don't plan to use Poetry on my machine.
> > Can we simply describe the dependencies even if the versions are not specified?
> 
> The reason we don't list the dependencies anywhere is that doing it with 
> Poetry is much easier (and a bit safer, as Poetry is going to install 
> tested versions).
> 
> But I can add references to the two relevant sections of 
> dts/pyproject.toml which contain the dependencies with a note that they 
> can be installed with pip (and I guess that would be another 
> dependency), but at that point it's that not much different than using 
> Poetry.

I want to use my system package manager.
I am from this old school thinking we should have a single package manager in a system.

> >> +.. code-block:: console
> >> +
> >> +   poetry install --no-root --with docs
> >> +   poetry shell
> >> +
> >> +The documentation is built using the standard DPDK build system.
> >> +After executing the meson command and entering Poetry's shell, build the documentation with:
> >> +
> >> +.. code-block:: console
> >> +
> >> +   ninja -C build dts-doc
> > 
> > Don't we rely on the Meson option "enable_docs"?
> 
> I had a discussion about this with Bruce, but I can't find it anywhere, 
> so here's what I remember:
> 1. We didn't want to tie the dts api doc build to dpdk doc build because 
> of the dependencies.

Sure
But we could just skip if dependencies are not met?

> 2. There's a way to build docs without the enable_docs option (running 
> ninja with the target), which is what we added for dts. This doesn't tie 
> the dts api doc build to the dpdk doc build.

Yes

> 3. We had an "enable_dts_docs" Meson option in the past (to keep it 
> separate from dpdk doc build), but decided to drop it. My memory is hazy 
> on this, but I think it was, again, because of the additional steps 
> needed to bring up the dependency (poetry shell) - at that point, 
> supporting just the ninja build way is sufficient. Bruce may shed more 
> light on this.


> >> +   Make sure to fix any Sphinx warnings when adding or updating docstrings,
> >> +   and also run the ``devtools/dts-check-format.sh`` script and address any issues it finds.
> > 
> > It looks like something to write in the contributing guide.
> > 
> 
> I could add it there, where is the right place? In patches.rst, section 
> "Checking the Patches"?

Yes