-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Remove Breathe from doc infra #73671
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
Remove Breathe from doc infra #73671
Conversation
bc9a80c to
f40a81c
Compare
These look like perfect candidates to be submitted separately: https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs |
Yep, I will open PRs for some of these changes. However, since some of the tweaks made to our use of C domain roles and Breathe directives would have not been necessary without this PR, I will probably keep them here. |
I don't think it makes a lot of difference. As long as it does not hurt Breathe, any preparation that makes PRs smaller is a win; especially for very impactful changes like replacing one build tool with another. In an ideal world, the final PR for such a replacement is only a few lines long because it merely "flips the switch". This is not always possible of course but it should be the aspiration. Continuous Integration FTW. |
2de1413 to
ded3df1
Compare
80fd7fb to
bbf19f4
Compare
a05fbc0 to
70d44b1
Compare
Simplify how the code sample refers to C objects to be less dependant on Breathe. Signed-off-by: Benjamin Cabé <[email protected]>
Do not pass options to doxygengroup to rationalize usage and be less dependent on Breathe. Signed-off-by: Benjamin Cabé <[email protected]>
Deactivate breathe extension from docs. Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add an initial version of doxybridge, an extension that allows to use Sphinx C domain to automatically reference Doxygen pages. It also introduces minimal support for `.. doxygengroup` directive, which effectively links to the group's Doxygen page. Co-authored-by: Benjamin Cabé <[email protected]> Signed-off-by: Gerard Marull-Paretas <[email protected]> Signed-off-by: Benjamin Cabé <[email protected]>
gmarull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
|
Very much into making the Doxygen references links to the API documentation rather than embedding it in the main documentation. Thanks for this! |
|
What I do not like about this: it's useful to find similar functions for a feature, this is very prevalent with bluetooth but can be seen with something basic, on the current documentation you can search for |
|
How do you reference back from the doxygen documentation of a subsystem to the where the subsystem is documented? for example from here https://builds.zephyrproject.io/zephyr/pr/73671/docs/doxygen/html/group__input__interface.html to here: https://builds.zephyrproject.io/zephyr/pr/73671/docs/services/input/index.html ?? |
I can look at auto-generating "back links" from Doxygen group pages to the corresponding Sphinx pages containing a |
|
I won't review the code changes sorry but I just measured this PR.
This also saves 10s on the incremental, "zero-change" rebuild time (I personally think the performance of the incremental build matters more). Here are the build times on my very old, "more cores than the build can use", Arch Linux build server. Slow disks but 350G RAM and filesystem cache is hot. The 4x CPU usage improvement is the most impressive datapoint. If you want to save the planet, stop "breathing"? /s Building the base of this PR also prints a lot of "Invalid C declaration" warnings. This PR does not. Nit: I doubt anyone will git bisect the documentation build but from a pedantic perspective it would have been nicer to have some "atomic" commit in the series that flips from breathe to the new solution instead of a "gap" (do NOT fix this and lose approvals unless someone else requests some other change). |
|
Should we remove Breathe from the example-application as well? |
Yes, will take care of this |
|
Actually, it would be nice to package existing local extension into a pip-installable package, so that projects like example-application can re-use it. |
I like the idea -- probably need to think about it some more as this would mean that these extension would essentially become "API", or at least that we would need to be more careful when changing their behavior (which might actually be a good thing, don't get me wrong) |
|
New kid on the block: https://hawkmoth.readthedocs.io/en/stable/index.html Just in case anyone is interested. Only heard of it now and I don't know anything about it sorry.
|
|
Sphinx itself was the problem when I had profiled this before, which lead me to fixes in sphinx's C domain improving things. But ultimately... its the C domain and how things are done that is problematic not the doxygen parsing part from what I recall, so I don't know if this really helps unless it completely replaces it. |
|
I understand the breathe performance issue has been solved. But in an ideal world, a solution based on Hawkmooth (or something like it) could:
So that's why I mentioned hawkmoth. But maybe it can't deliver that or not yet and I just made some noise; apologies. |
|
Breathe was a problem in itself, but Sphinx is also veeeery slow rendering content, that explains the dramatic speed increase in the docs build since all C docs were removed. Doxygen has proven to be unbeatable in terms of speed. |
|
Hawkmoth maintainer here 👋 I think Hawkmoth is a great alternative to Doxygen+Breathe if you're primarily interested in API documentation and only need a subset of Doxygen features. Hawkmoth is certainly not a drop-in replacement for Doxygen, and that was never really a goal. The Hawkmoth examples as well as Mesa developer documentation give an idea what Hawkmoth can offer. If you're happy with Doxygen, and you've got its configuration and complexity under control, or need a lot of its features, you're probably better off just sticking with it. But it was also not an insurmountable task to switch Mesa from Doxygen+Breathe to Hawkmoth, as they were only using Doxygen for API documentation. The cool part is simplifying the documentation build to just |
Exactly this. |
Doxygen is very fast but it also does a lot... except .rst integration, which was the purpose of breathe IIRC. It all depends what is desired and that has never been clear to me. Requirements should always be informed by what is practical but "doxygen is fast, so let's use that whatever it does" sounds a bit extreme and backwards :-) If the requirement were to perform only 5% of what doxygen does but better integrated with .rst, then using hawkmoth could take about the same time or be even faster by virtue of doing less. If keeping .rst and doxygen separate is desired then forget it (and hit the "unsubscribe" button).
It would be interesting to build the mesa docs (and others) with and without hawkmoth (leaving blanks instead), measure the time difference and "advertise" these sample overheads on the project page. If that's not possible because it makes the build "fail fast", then... it should be made possible for basic performance evaluation :-) |
📄 Live doc example: https://builds.zephyrproject.io/zephyr/pr/73671/docs/services/input/index.html
Following up on some great work from @gmarull a while back (PR #39702), this PR effectively drops Breathe to instead directly link to our Doxygen website.
For people not familiar with Breathe, it's what's allowed us to effectively "embed" Doxygen documentation directly in our main docs.zephyrproject.org documentation. The
.. doxygengroup::would allow to list the entire contents of a Doxygen group and embed it in a given ReStructuredText page, and mentions to a C object (ex. struct, function, etc.) made using the C domain "roles" (ex.:c:struct:`foo`,:c:func:`bar`) would automatically link to the RST page in which the Doxygen group was embedded. This makes for a nice "one stop shop" experience but doesn't scale well as the project grows.This PR builds on @gmarull's "doxybridge" extension so that it handles all of the C domain roles used in our docs. Also adds support for a basic
.. doxygengroup::(initially coming from Breathe) directive as it's convenient, for now at least, to have the "API reference" sections of many of our pages still pointing to something useful, as well as list relevant code samples.The extension now uses Doxygen "official" xml parser (where Gerard was initially using ElementTree).
It also parses in parallel, as this is quite a resource extensive, and only parses again when needed on subsequent incremental builds.
A clean "turbo" build of the docs on my M2 takes ~3 minutes, and I am hopeful that OOM errors are also a thing of the past.
Still to be tested/done: