Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Breathe for API Reference documentation #3377

Closed
reneme opened this issue Mar 15, 2023 · 12 comments
Closed

Use Breathe for API Reference documentation #3377

reneme opened this issue Mar 15, 2023 · 12 comments

Comments

@reneme
Copy link
Collaborator

reneme commented Mar 15, 2023

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

@randombit
Copy link
Owner

Sounds good to me

@randombit
Copy link
Owner

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?

@reneme
Copy link
Collaborator Author

reneme commented Mar 16, 2023

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
Copy link
Collaborator Author

reneme commented Mar 16, 2023

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

image

@randombit
Copy link
Owner

randombit commented Mar 16, 2023

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.

@reneme
Copy link
Collaborator Author

reneme commented Apr 5, 2023

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 (Doc: Breathe for TLS #3484)

    • credentials_manager.rst
    • tls.rst
      • Update the ASIO stream's documentation
    • 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
Copy link
Collaborator Author

reneme commented Apr 12, 2023

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

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

@reneme
Copy link
Collaborator Author

reneme commented Apr 12, 2023

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
Copy link
Collaborator Author

reneme commented May 12, 2023

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
Copy link
Collaborator Author

reneme commented May 23, 2023

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
Copy link
Collaborator Author

reneme commented Dec 20, 2023

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

@reneme
Copy link
Collaborator Author

reneme commented Jul 23, 2024

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

@reneme reneme closed this as completed Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants