MDEV-39718: Produce Markdown plugin API documentation - #5112
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CMake utility to generate plugin API documentation using Doxygen and Moxygen, along with various documentation formatting improvements in header files to ensure proper rendering. The review identifies a critical bug in the CMake macro where the first source file is inadvertently removed from the list, causing incomplete documentation. Additionally, the feedback suggests using functions instead of macros for better encapsulation, adopting more idiomatic boolean options in CMake, and defaulting the documentation generation to off to avoid build-time dependency issues in environments lacking the required tools.
gkodinov
added
the
MariaDB Foundation
e20f707 to
436a2ca
Compare
There was a problem hiding this comment.
Hi, @gkodinov ,
I don't think you need to integrate this as part of cmake, it looks over-engineered for the purpose,
when you can bundle every tool you need in a Docker container and run it as part of GitHub Actions.
Not to say, every change for this cmake file is a server build,
whether, having a containerized build environment means you can control
the behavior of "what needs to be done" somewhere else.
I will re-iterate what I told you on the e-mail so @vaintroub can read also.
Now that I read this patch, you don't even need cmake in container with what I proposed, simplifying the Dockerfile by a lot.
-------- from e-mail ------------
_From what you describe, you absolutely do not need Buildbot or a build host.
You only need GitHub Actions and a container with the required tools.
First of all, for local testing, you can build a container image that includes the tools you need.
For example, at the end of this email, you will find a simple Dockerfile with the tools you've requested (and some build-tools required by a server CMake invocation).
The generation logic (CMake, calling Doxygen, Moxygen) can be included in a script
as part of this containerized build environment (in my example, it is generate.sh). You can also copy the Doxygen configuration file, Doxyfile.in, into this container.
Then, you can publish this container to the GitHub Container Registry.
and later use it as part of a GitHub Action that responds to push events on the branches you are interested in. GitHub can run containers on its public agents.
Any further logic, such as opening a Pull Request or performing a regression test can be accommodated as part of this GitHub Action._
FROM node:20-bookworm-slim
ARG DEBIAN_FRONTEND=noninteractive
ARG MOXYGEN_PACKAGE=moxygen@0.8.0
RUN apt-get update \
&& apt-get install -y --no-install-recommends \
bison \
build-essential \
ca-certificates \
cmake \
doxygen \
flex \
gettext-base \
git \
graphviz \
libaio-dev \
libfmt-dev \
liblz4-dev \
liblzma-dev \
libncurses-dev \
libnuma-dev \
libpcre2-dev \
libreadline-dev \
libsnappy-dev \
libssl-dev \
libsystemd-dev \
ninja-build \
perl \
pkg-config \
zlib1g-dev \
&& rm -rf /var/lib/apt/lists/*
RUN npm install --global "${MOXYGEN_PACKAGE}" \
&& npm cache clean --force
WORKDIR /work
COPY Doxyfile.in /opt/plugins-api-docs/Doxyfile.in
COPY generate.sh /usr/local/bin/generate-plugins-api-docs
RUN chmod +x /usr/local/bin/generate-plugins-api-docs
ENTRYPOINT ["generate-plugins-api-docs"]
There was a problem hiding this comment.
The idea behind pushing as much of it as possible into cmake is that developers should be able to efficiently drive this via normal commits to the repo. Besides, developing the API docs is easier if you just hit "make" when you need a preview.
There was a problem hiding this comment.
@RazvanLiviuVarzaru ghcr.io docker recipes are scoped to either a login or to an org. I would really appreciate if you could add the above docker image to https://github.com/orgs/MariaDB/packages. Just remove the things after (and including) WORKDIR.
And please do not use @0.8.0 in the recipe: just install whatever's latest.
I can do the rest from the relevant github action script.
There was a problem hiding this comment.
I also wonder if the dependencies can be added to bb-worker? would that be too much?
There was a problem hiding this comment.
@gkodinov Hi,
The sample Dockerfile it was just an example for you to get an idea of how it could look like.
If you are going with the solution I proposed, you can build it to your like and push it yourself to GHCR.
There was a problem hiding this comment.
"I also wonder if the dependencies can be added to bb-worker? would that be too much?"
I am not onboard with having Buildbot producing documentation.
Buildbot is concerned with building and testing the server. The helper tools, i.e. producing documentation for the knowledge base can be hosted elsewhere. Generating documentation for the plugin API naturally happens at a lower cadence and frequency than the server’s need to be built and tested (which is for every commit). That is why I suggested using GitHub Actions.
There is no need for Buildbot to invest resources so that, for every commit, a builder generates documentation that people will most likely look at very rarely, if at all, except perhaps for a small number of people. Consequently, I do not plan to add the tools to a bb-worker.
There was a problem hiding this comment.
"The idea behind pushing as much of it as possible into cmake is that developers should be able to efficiently drive this via normal commits to the repo. Besides, developing the API docs is easier if you just hit "make" when you need a preview."
It somehow feels like implementing a solution is taking priority over validating the output of such a solution with the people who will utilize it.
I would have expected you to generate some documentation with the tool first and discuss it internally with the developers, so that you could agree on a format and process. Did that happened?
What is a developer supposed to do if they run CMake to generate documentation?
Will they open a pull request to update the knowledge base? Is that their responsibility?
If there is no feedback loop where the documentation produced as a result of the developer’s code contributions is visible, then I do not see the point in having them generate it via cmake.
I tried using Doxygen and Moxygen in the form you proposed, and I found the documentation rather cumbersome to read. Is it just me?
There was a problem hiding this comment.
It's under review in this PR. But yes, I have had discussions prior to the PR.
There was a problem hiding this comment.
No, it's not just you. We need to start producing the docs before we improve on them.
There was a problem hiding this comment.
What is a developer supposed to do if they run CMake to generate documentation?
Tend to errors produced. And make sure whatever they're writing is usable docs-wise.
Will they open a pull request to update the knowledge base? Is that their responsibility?
Ideally it should be done as part of the pull request that adds or edits the API
If there is no feedback loop where the documentation produced as a result of the developer’s code contributions is visible, then I do not see the point in having them generate it via cmake.
This is the whole point in having a cmake target: you build as you normally would and the build would fail if you've added wrong doxygen comments or forgot to document some parameter or something.
vuvova
left a comment
vuvova
left a comment
There was a problem hiding this comment.
code-wise see comments in-line. this is under assumption that we'll eventually agree that this should be in the server repo.
As far as I understand, the main question, whether it has to be in the server repo at all, is still open.
| SET(DOXYGEN_EXTRACT_ALL YES) | ||
| SET(DOXYGEN_QUIET YES) | ||
| SET(DOXYGEN_XML_PROGRAMLISTING YES) | ||
| doxygen_add_docs(${DOCS_PROJECT_NAME} ${SOURCES} ALL USE_STAMP_FILE COMMENT "Generating ${ARG_NAME} XML docs with doxygen") |
There was a problem hiding this comment.
remove ALL, this custom target must be EXCLUDE FROM ALL because it's neither installed nor packaged
There was a problem hiding this comment.
Note that the whole thing is off by default: it's governed by WITH_GENERATED_DOCS already. So, if you enable it, you must know what you're doing.
I'd like to keep it into ALL (this and the other target) mostly because of the workflow I've mentioned above: edit your files and press "build".
Yes, it's not packaged because it's being delivered to a different repo instead. So, having it on some of the general off-the-mill builds will trigger an error as it should IMHO.
I can of course remove it, but please consider the workflow above before confirming that I should.
There was a problem hiding this comment.
press "build" where? I thought about, like edit your files and run make plugin_api_docs or something. Normal build doesn't build docs, building docs doesn't run a full build.
There was a problem hiding this comment.
with EXCLUDE FROM ALL likely there will be no need to have WITH_GENERATED_DOCS, the target could be generated automatically if preconditions are met. Or even always.
There was a problem hiding this comment.
press "build" where? I thought about, like edit your files and run
make plugin_api_docsor something. Normal build doesn't build docs, building docs doesn't run a full build.
In the IDE. E.g. I use vs.code. Cmd-Shift-B "builds". Docs included, if so configured. I of course can go in and pick a cmake target from the side panel to build. But it's not as straightforward as just pressing a familiar key combo. And, at least on my laptop, building the whole thing is equally fast to finding the right target, then click the little button next to it.
with EXCLUDE FROM ALL likely there will be no need to have WITH_GENERATED_DOCS, the target could be generated automatically if preconditions are met. Or even always.
True. But I am after the above workflow.
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/docs/${ARG_NAME}/md | ||
| BYPRODUCTS ${CMAKE_BINARY_DIR}/docs/${ARG_NAME}/md/${ARG_NAME}.md | ||
| COMMENT "Generating ${ARG_NAME} markdown docs with moxygen" | ||
| ) |
There was a problem hiding this comment.
see my reply on the other similar question please.
My argument about having it into the server repo is that I can do as I did today: edit the header file(s), press "Build" and get a result (or a build error if it fails). This will help me personally update the docs in a more natural fashion. I'd guess others too. |
Okay, this is a good argument, I agree. |
|
Another option would be to commit a script, like, |
Serg, what are you trying to achieve here? The github action is a script in the repository, formatted in a special way. I have no qualms running cmake from it. I like the simplicity of the cmake/doxygen integration. The Doxyfile is a stuff nightmares are made from: too many options that nobody really knows or cares about. |
It would avoid running cmake detecting all libraries that a server build needs, failing if, e.g. ncurses-devel is not installed, and so no. All the overhead that doc generation does not need. And github action or you will call this script, which will be in the server repo. Making doc generation dependent on server build prerequisites looks a bit artificial |
The alternative to this is:
This would need the build directory to be passed. Unless you want to generate the docs (potentially many files) into the source directory itself. And, we'd loose the "make only if changed" part this way: unless we hard-code all the headers into the cmake target in addtion to the doxygen file. And the benefits of being able to throw away the build dir and get a clean slate. Overall, the more I think about it, the more it looks like a square peg into a round hole. But, at this point, after soo many iterations and changes of heart, I'm willing to do anything to get this over with. |
|
FYI, this is what the alternative solution looks like. Not pretty (IMHO). 0001-MDEV-39718-Produce-Markdown-plugin-API-documentation.patch |
|
There will be a second comment after I check the patch, but just to reply to your first one:
I think it just simpler. |
|
I've read the attached patch. I don't get it, what's less pretty than it would've been in pure cmake approach? you still need to edit |
The two things I do not like about the alternative patch are:
How about the following compromise:
I believe this will get me what I need (building the docs only when needed when I press Cmd-Ctrl-B in vs. studio) while (mostly) keeping it out of cmake and allowing the github action to not need to run cmake to produce the docs. Does this sound like a good compromise? |
|
@gkodinov , you can write cross-platform scripts with cmake, if you wish. it can be used as general cross-plattform language, a bit like perl, or python or so . you execute them scripts with cmake -P yourscript.cmake. And still this script does not have to run during the build. |
For the sake of simplicity, I'll keep the .sh file for now. But it's an interesting idea. |
a04d716 to
cc0d1dc
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds infrastructure to generate Markdown-formatted Plugin API documentation (Doxygen XML + moxygen Markdown) and adjusts various Plugin API headers’ Doxygen markup to improve/enable clean documentation generation.
Changes:
- Added a Doxygen configuration, mainpage, and a helper script to generate Markdown docs for the Plugin API.
- Added a CMake option/target to run the documentation generation as a build utility.
- Normalized/fixed Doxygen comments across several
include/mysql/**headers to improve generated output.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| include/mysql/service_thd_rnd.h | Doxygen param annotations corrected/expanded for plugin API docs generation. |
| include/mysql/service_thd_autoinc.h | Normalized Doxygen [in]/[out] parameter tags. |
| include/mysql/service_progress_report.h | Moved thd_progress_init prototype out of a comment block so Doxygen parsing is cleaner. |
| include/mysql/service_my_snprintf.h | Adjusted Doxygen formatting/escaping to improve Markdown generation output. |
| include/mysql/service_logger.h | Doxygen tag cleanup for logger service documentation. |
| include/mysql/service_debug_sync.h | Wrapped example code in @code/@endcode for proper Doxygen rendering. |
| include/mysql/psi/psi.h | Cleaned up Doxygen grouping/typedef documentation to support generated docs. |
| include/mysql/psi/psi_memory.h | Removed/adjusted Doxygen group metadata for generated docs. |
| include/mysql/psi/psi_base.h | Removed/adjusted Doxygen group metadata for generated docs. |
| include/mysql/psi/mysql_thread.h | Fixed Doxygen param name mismatch in macro documentation. |
| include/mysql/psi/mysql_socket.h | Adjusted Doxygen parsing around conditional signatures; minor doc cleanups. |
| include/mysql/psi/mysql_mdl.h | Updated Doxygen group name for metadata instrumentation docs. |
| include/mysql/index.dox | Added Plugin API documentation mainpage content. |
| include/mysql/generate_plugin_api_docs.sh | Added a helper script to run Doxygen + moxygen to produce Markdown docs. |
| include/mysql/Doxyfile.generated_docs_plugin_api | Added a dedicated Doxygen config for Plugin API docs generation. |
| include/m_ctype.h | Doxygen escaping and parameter naming fixes for clearer generated output. |
| include/CMakeLists.txt | Added an opt-in CMake target/option to generate Plugin API docs during builds. |
Comments suppressed due to low confidence (1)
include/mysql/psi/mysql_socket.h:156
- This Doxygen comment describes setting a socket descriptor/address, but it documents
mysql_socket_set_thread_owner, which sets ownership. The brief should match the function to avoid misleading generated docs.
/**
Set socket descriptor and address.
@param socket instrumented socket
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vuvova
left a comment
vuvova
left a comment
There was a problem hiding this comment.
you've put your files in include/mysql/. Please make sure they're not installed with other headers
| #LOOKUP_CACHE_SIZE = 0 | ||
| NUM_PROC_THREADS = 0 | ||
| #TIMESTAMP = NO | ||
| EXTRACT_ALL = YES |
There was a problem hiding this comment.
what does it do? i didn't quite understand from the documentation
There was a problem hiding this comment.
It will generate (bare) doxygen docs even for the elements that do not have doxygen comment decorations. It's useful as it has the link target (so that other docs or structures can reference it) and the data type (so you don't have to resort to the source file).
| #INPUT_ENCODING = UTF-8 | ||
| #INPUT_FILE_ENCODING = | ||
| FILE_PATTERNS = *.h *.dox | ||
| RECURSIVE = YES |
There was a problem hiding this comment.
I thought we wanted NO here to avoid scanning psi/ files?
There was a problem hiding this comment.
It's OK, I've fixed the PSI files too. They are a part of the API IMHO.
| #GENERATE_TESTLIST = YES | ||
| #GENERATE_BUGLIST = YES | ||
| #GENERATE_DEPRECATEDLIST= YES | ||
| #GENERATE_REQUIREMENTS = YES |
There was a problem hiding this comment.
you have disabled GENERATE_HTML and GENERATE_LATEX but did not disable any other GENERATE options — are they all disabled anyway (I checked a couple, they only work if some other condition is true, which isn't the case here, but I did not check them all, so I ask) ?
There was a problem hiding this comment.
Yes, the rest are off by default.
|
|
||
| # run doxygen to generate the plugin API documentation XML | ||
| # Need to pass output directory so that XML docs get generated in it. | ||
| # Need to EXCLUDE the output directory so that doxygen does not try to parse the generated XML and markdown files |
There was a problem hiding this comment.
do you need to EXCLUDE it? not a bit deal, admittedly, exclude away, but you've set FILE_PATTERNS to *.c, *.dox, it shouldn't try to real xml or md files. Also, md/ dir is emply, you've just deleted and recreated it.
There was a problem hiding this comment.
A stability thing. I'd like to keep it if you do not mind.
| WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/mysql" | ||
| COMMENT "Generating plugin API documentation") | ||
| ADD_CUSTOM_TARGET(plugin_api_docs ALL | ||
| DEPENDS ${DOCS_STAMP}) |
There was a problem hiding this comment.
why do you need a stamp, you can use any of generated files as a stamp. but ok, whatever.
just make sure none of this gets installed.
There was a problem hiding this comment.
Fair enough. Removed the stamp and used api.md instead.
On the install comments: can you be more specific please? I do not install any target or bunary by default.
The headers installation uses FILES_MATCHING PATTERN "*.h". This doesn't include any of the .sh and the Doxyfile files.
Generated the plugin API headers using a shell script. Fixed some doxygen comment mistakes in the headers. Added a cmake conveninence target to generate the docs into $BUILD_DIR/docs Added a main page for the API docs. Included all of the existing group .md files into the CMake target Leveraged moxygen 2.1.11's fixes to produce the full API docs in a single go Removed the list of output .md files from the CMake target and switched to a stamp file to avoid unnecessary rebuilds of the docs when the list of .md files changes. Addressed various review comments.
Implemented a cmake utility macro to generate markdown documentation.
Generated the plugin API headers.
Fixed some doxygen comment mistakes in these.