Skip to content

CXX-3199 Generate API documentation with generated headers #1301

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

Merged
merged 22 commits into from
Dec 18, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Dec 11, 2024

Summary

Addresses CXX-3199.

Refactors the doxygen-current and doxygen-latest targets to generate API documentation pages using a full set of headers, including those which are generated by CMake during configuration (e.g. export.hpp).

Generated Headers

This PR effectively requires Doxygen be executed via the CMake targets doxygen-current (although it is really the docs_source_directory doxygen-install-headers target that is important). The docs_source_directory doxygen-install-headers target takes advantage of the symmetric directory layout implemented by #1026 and raw-copies the <lib>/include and <lib>/lib directories into the same directory. This effectively merges the public and internal directory structure together into a single "install" directory which is parsed by Doxygen.

Important

The build/doxygen directory is now reserved for use by Doxygen so that this behavior is reliable regardless of the custom binary directory being used by the user.

Warning

This blindly copies the contents of the include and lib directories without any filters. If there are any stray .hpp header files in either directory, they will be blindly copied as well. This is not an issue for the doxygen-latest target which uses a temporary directory, but may cause some inconvenience for the doxygen-current target.

This permits Doxygen documentation comments to be embedded directly in the generated headers' input files, obviating the need to define "Group" pages to document non-existent header files. The headers can now be referenced directly (e.g. @ref bsoncxx/v_noabi/bsoncxx/config/export.hpp) and are listed under the Files page just like any other header.

To continue supporting the use of a custom Doxygen binary (to enforce an exact Doxygen version and corresponding generated output), the CMake FindDoxygen module is used to permit customization via the DOXYGEN_EXECUTABLE variable (use as a result variable is deprecated in favor of Doxygen::doxygen, but this variable is still used for the internal call to find_program()).

The following command thus generates the current API doc pages for local preview:

# Custom binary if needed.
export DOXYGEN_BINARY=doxygen-1.12.0

# To avoid (re)downloading the C Driver.
export CMAKE_PREFIX_PATH=/path/to/c-driver/install

# Local repository.
cmake -S . -B <binary-dir>

# Generate API docs under build/docs/api/current.
cmake --build <binary-dir> --target doxygen-current

Latest API Docs

The generate-* Perl scripts are completely removed in favor of a Bash script to remove reduce our toolchain dependency on Perl (still used for deploy-to-ghpages.pl). Following #1169, the generate-latest-apidocs.pl effectively lost its purpose as the "generate the latest instead of generating all" script, since we no longer regenerate old doc pages. The generate-apidocs-from-tag.pl was only being used to implement the behavior of generate-latest-apidocs.pl and no longer used for any other purpose.

To avoid continuing to maintain Perl, both scripts are replaced with a straightforward Bash script that preserves only the minimum required behaviors from the old scripts. A temporary directory is used to clone a clean copy of the repository and checkout the relevant release tag whose source files are to be documented. sed is used to directly patch the Doxyfile with relevant version information and output path. CMake is called to generate the headers (via --target docs_source_directory) before running Doxygen manually to avoid confusing the current CMake project (which invokes --target doxygen-latest) and the CMake project in the temporary directory being used to generate the API documentation.

# For generate-latest-apidocs.sh.
export DOXYGEN_BINARY=doxygen-1.12.0

# To avoid (re)downloading the C Driver.
export CMAKE_PREFIX_PATH=/path/to/c-driver/install

# Local repository.
cmake -S . -B build

# Generate API docs under build/docs/api/<latest>.
cmake --build build --target doxygen-latest

@eramongodb eramongodb requested a review from kevinAlbs December 11, 2024 22:58
@eramongodb eramongodb self-assigned this Dec 11, 2024
@kevinAlbs kevinAlbs requested review from vector-of-bool, a user and kevinAlbs and removed request for kevinAlbs and a user December 12, 2024 19:52
@ghost ghost self-requested a review December 16, 2024 19:13
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good! It's nice to simplify the doc generation process. Two very minor comments, a doc update I just happened to come across and a hardcoded branch that I noticed when trying to build it locally.

mkdir -p "${apidocpath:?}"

# Use a clean copy of the repository.
git clone -q -c advice.detachedHead=false -b "cxx-3199" . "${tmpdir}"
Copy link

Choose a reason for hiding this comment

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

The hardcoded branch name will need to be removed. Checking whether we need a branch name at all, and I think the default behavior of git clone (forking the currently active branch) is always what we want. Adding "--depth 1" might be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Overlooked this reversion during a rebase. It is supposed to reference the latest release tag (e.g. v4.0.0). Fixed.

--depth 1

This is actually a local clone, for which --depth is ignored (as it provides no benefit):

warning: --depth is ignored in local clones; use file:// instead.

When the repository to clone from is on a local machine, this flag bypasses the normal "Git aware" transport mechanism and clones the repository by making a copy of HEAD and everything under objects and refs directories. The files under .git/objects/ directory are hardlinked to save space when possible. If the repository is specified as a local path (e.g., /path/to/repo), this is the default, and --local is essentially a no-op.

Copy link

Choose a reason for hiding this comment

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

Oh, good to know. Thanks!


# The required Doxygen version.
# The generated results are sensitive to the release version.
our $doxygen_version_required = "1.12.0";
Copy link

Choose a reason for hiding this comment

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

I noticed a dangling reference to this variable in the docs, the "IMPORTANT" note in releasing.md should refer to the new shell variable instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Release instructions have been updated accordingly.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review. Migrating perl scripts to bash is much appreciated.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

LGTM

@eramongodb eramongodb requested a review from kevinAlbs December 18, 2024 15:28
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM with a fix to the packaging tasks.

doxygen_version="$("${DOXYGEN_BINARY:?}" -v | perl -lne 'print $1 if m/^(\d+\.\d+\.\d+).*$/')"
if [[ "${doxygen_version:-}" != "${DOXYGEN_VERSION_REQUIRED:?}" ]]; then
echo "${DOXYGEN_BINARY} version ${doxygen_version:-"unknown"} does not equal ${DOXYGEN_VERSION_REQUIRED:?}" 1>&2
exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Requiring a version of Doxygen appears to cause failures in the packaging tasks:

doxygen version 1.9.8 does not equal 1.12.0

The Debian package appears to include docs built with the version of Doxygen available from apt. Consider reducing this to a warning for the doxygen-current target.

Copy link
Contributor Author

@eramongodb eramongodb Dec 18, 2024

Choose a reason for hiding this comment

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

@rcsanchez97 The override_dh_auto_build rule in the debian/unstable branch (along with other related scripts) may need to be updated to set a new DOXYGEN_ANY_VERSION=1 environment variable when building the doxygen-current target to avoid the Doxygen binary version compatibility check from failing the task. Alternatively they may be updated to avoid building API docs if that is permitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eramongodb Thanks for the heads up. The solution was fairly trivial, but testing it was a bit of a different story (owing to the Debian packaging being on a separate branch). I figured out a way to inject the proposed change into a patch build and confirmed that it works (here).

After pushing the tested change (on the debian/unstable branch), I re-ran the task on the waterfall and it worked (here).

@eramongodb eramongodb merged commit 99dc81f into mongodb:master Dec 18, 2024
1 of 2 checks passed
@eramongodb eramongodb deleted the cxx-3199 branch December 18, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants