botan icon indicating copy to clipboard operation
botan copied to clipboard

Use Breathe for API Reference documentation

Open reneme opened this issue 2 years ago • 11 comments

Currently, the documentation in docs/api_ref duplicates quite a bit of the API details already documented in Doxygen. This adds a maintenance burden and leads to outdated documentation.

Instead, I propose to use Breathe for our Sphinx documentation. This allows pulling API documentation from Doxygen into Sphinx documentation.

Example

Say, I want to describe the mandatory callbacks to be implemented by an application using TLS.

First I would use a doxygen member group to define the mandatory members in the code.

namespace Botan::TLS {
class Callbacks {
public:
  /**
   * @name Mandatory
   * Those callbacks must be overridden by all applications wishing to use TLS.
   */
  ///@{
  void tls_emit_data();
  void tls_record_received();
  // ...
  /// @}
};
}

Then, instead of recreating all the information about the callback methods, one would simply add the following in the reStructuredText documentation for Sphinx:

.. doxygenclass:: Botan::TLS::Callbacks
    :members:
    :membergroups: Mandatory

And the result would look like that. Further tuning is possible, of course:

image

reneme avatar Mar 15 '23 09:03 reneme

Sounds good to me

randombit avatar Mar 15 '23 10:03 randombit

It does seem breathe is available in most distros

https://repology.org/project/python:breathe/versions and https://repology.org/project/breathe/versions

But the question arises what can/should we do if breathe is not available. Can we proceed with building the docs at that point, in some degraded fashion? Or is there no chance of proceeding?

randombit avatar Mar 15 '23 11:03 randombit

We can detect that breathe is not installed in the system (via except ImportError in Sphinx's conf.py) and simply not register it as an extension in Sphinx. This will result in warnings during build:

doc/api_ref/tls.rst:39: WARNING: Unknown directive type "doxygenclass".

.. doxygenclass:: Botan::TLS::Callbacks
    :members:
    :membergroups: Mandatory

and simply leave out the Doxygen information.

Probably, we could even mock breathe's directives in this case and render some "API documentation not available"-warning into the documentation in lieu of the actual breathe output.

Is building the documentation on some restricted platform an actual use case though?

reneme avatar Mar 16 '23 09:03 reneme

By the way, breathe works well with PDF output as well:

image

reneme avatar Mar 16 '23 09:03 reneme

Is building the documentation on some restricted platform an actual use case though?

Not really, but it would be nice if the user got basically working documentation even if they don't/can't have this extension installed for whatever reason. It sounds like things are fine in that regard.

randombit avatar Mar 16 '23 13:03 randombit

Status Update

I think there's enough progress in converting the API reference to Breathe for today. I split the changes into small PRs (#3470, #3472, #3473, #3474, #3475, #3476, #3477, #3478) to ease the review a bit.

A note on method: I went through the documentation in rST, moved extra detail over to the Doxygen comments where appropriatie and then replaced .. cpp:function:: with .. doxygenfunction:: (and friends). In places where the actual API documentation was not front-and-center for the topic, I wrapped it into .. container:: toggle to collapse it by default. I feel that makes the documentation much less cluttered for the occasional reader.

At some places I did find and fix minor documentation errors. Though, generally, these should happen in a separate pass.

Chapters that still need attention

  • Hardware Wrappers

    • [ ] pkcs11.rst
    • [ ] tpm.rst
  • Transport Layer Security (#3484)

    • [x] credentials_manager.rst
    • [x] tls.rst
      • [ ] Update the ASIO stream's documentation
    • [x] psk_db.rst
  • Public Key Infrastructure

    • [ ] pubkey.rst
    • [ ] x509.rst
  • Core Functionality

    • [ ] rng.rst
    • [ ] secmem.rst
  • Misc

    • [ ] fpe.rst
    • [ ] keywrap.rst
    • [ ] otp.rst
    • [ ] roughtime.rst
    • [ ] srp.rst
    • [ ] tss.rst
    • [ ] zfec.rst
  • Integrations

    • [ ] ffi.rst
    • [ ] python.rst
  • Legacy

    • [ ] filters.rst

reneme avatar Apr 05 '23 14:04 reneme

Lessons Learned

Lets collect some lessons learned for our use case. In my opinion the main objective is not having to duplicate API documentation between header files (Doxygen) and Sphinx (rST).

Below is an opinionated brain dump of my insights from the PRs created last week:

  • functionally Breathe does what it says on the box
    • usage of rST directives is convenient and fairly easy to understand
    • easy integration into our build pipeline (./configure.py, build_docs.py) and CI
    • PDF generation is good enough
  • more customization options would be nice, e.g.:
    • the order of the methods in the documentation should be equal to the order in the :members: list (perhaps Doxygen member groups could be a workaround)
    • perhaps have a way to add "additional information" to the documentation of specific members for the rST-based documentation only
  • performance: it's awfully slow
    • after integrating most of the open adaption PRs, my machine needs 10 minutes (!) to re-generate the documentation (with 600% CPU utilization).
    • CI will likely take a lot longer 😞 (Doxygen generated ~2.500 XML (~80 MB) files that needs to be processed)
    • this seems to be a known issue for a while:
      • https://github.com/breathe-doc/breathe/issues/439
      • https://github.com/breathe-doc/breathe/issues/315
    • with a WIP patch: https://github.com/breathe-doc/breathe/pull/891

Like that Breathe this is just not feasible, I guess. 😢

reneme avatar Apr 12 '23 12:04 reneme

Regardless of using Breathe or not: The current state of the documentation needs some TLC. The currently open (Breathe-related) PRs contain a number of cleanups and fixes to the documentation. If we decide to abandon Breathe, I could pull those out.

Re:togglebutton: I'm sure we'd find a way to make this work without the dependency.

Re:Breathe: I must say, I'm quite sad with this situation. I actually do enjoy the way Breathe bridges the gap between rST and Doxygen. Despite the lacking functionality and speed its a great tool, IMO. What's your general feeling of Breathe's output/functionality @randombit? Do you think its worth investing more time into this?

reneme avatar Apr 12 '23 12:04 reneme

A note on performance: disabling source code listing in the XML output (XML_PROGRAMLISTING = NO in botan.doxy.in) cuts the size of the generated XML roughly in half. As Breathe's performance issues seem to stem from inefficient parsing, this also drastically speeds up the documentation generation in general. Proper measurements pending.

reneme avatar May 12 '23 07:05 reneme

With XML_PROGRAMLISTING = NO the performance of Breathe is still not great, but I'd argue its "good enough". In this little test balloon (merge of all Breathe adaptions) the docs CI job ran for about 2 minutes (in total with setup) and slightly more than 1 minute for the build step alone.

I find that bearable for the convenience it brings. Also, I removed the sphinx-togglebutton dependency (by inlining the extension in our repo).

@randombit If we decide to take this further, I'd be glad if we could start reviewing/merging the open PRs to reduce clutter. Otherwise, I'd like to rescue the API documentation fixes in them and then close them.

Unfortunately, there doesn't seem to be a notable alternative to Breathe for our usecase. Except manual sync with the documentation as before or replacing the API docs in Sphinx with links to Doxygen.

reneme avatar May 23 '23 06:05 reneme

For the record: @lieser pointed out that people work on a speed improvement for Breathe. https://github.com/breathe-doc/breathe/discussions/962

reneme avatar Dec 20 '23 12:12 reneme

We decided to drop the idea of integrating Breathe. Closing.

reneme avatar Jul 23 '24 15:07 reneme