Skip to content

MDEV-39718: Produce Markdown plugin API documentation#5112

Open
gkodinov wants to merge 1 commit into
mainfrom
mdev-39718
Open

MDEV-39718: Produce Markdown plugin API documentation#5112
gkodinov wants to merge 1 commit into
mainfrom
mdev-39718

Conversation

@gkodinov
Copy link
Copy Markdown
Member

@gkodinov gkodinov commented May 22, 2026

Implemented a cmake utility macro to generate markdown documentation.
Generated the plugin API headers.
Fixed some doxygen comment mistakes in these.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmake/plugin_api_docs.cmake Outdated
Comment thread cmake/plugin_api_docs.cmake Outdated
Comment thread cmake/plugin_api_docs.cmake Outdated
Comment thread cmake/plugin_api_docs.cmake Outdated
@gkodinov gkodinov added the MariaDB Foundation Pull requests created by MariaDB Foundation label May 26, 2026
@gkodinov gkodinov force-pushed the mdev-39718 branch 2 times, most recently from e20f707 to 436a2ca Compare May 26, 2026 13:21
Comment thread cmake/plugin_api_docs.cmake Outdated
Comment thread cmake/plugin_api_docs.cmake Outdated
Copy link
Copy Markdown

@RazvanLiviuVarzaru RazvanLiviuVarzaru May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member Author

@gkodinov gkodinov May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wonder if the dependencies can be added to bb-worker? would that be too much?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown

@RazvanLiviuVarzaru RazvanLiviuVarzaru May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Copy Markdown

@RazvanLiviuVarzaru RazvanLiviuVarzaru May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's under review in this PR. But yes, I have had discussions prior to the PR.

@vuvova vuvova self-requested a review May 26, 2026 15:39
Implemented a cmake utility macro to generate markdown documentation.
Generated the plugin API headers.
Fixed some doxygen comment mistakes in these.
@gkodinov gkodinov marked this pull request as ready for review May 27, 2026 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Foundation Pull requests created by MariaDB Foundation

Development

Successfully merging this pull request may close these issues.

3 participants