-
Notifications
You must be signed in to change notification settings - Fork 544
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
Conversation
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.
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.
etc/generate-latest-apidocs.sh
Outdated
mkdir -p "${apidocpath:?}" | ||
|
||
# Use a clean copy of the repository. | ||
git clone -q -c advice.detachedHead=false -b "cxx-3199" . "${tmpdir}" |
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.
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.
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.
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.
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.
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"; |
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.
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.
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.
Good catch. Release instructions have been updated accordingly.
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.
Apologies for the delayed review. Migrating perl scripts to bash is much appreciated.
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.
LGTM
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.
LGTM with a fix to the packaging tasks.
etc/generate-latest-apidocs.sh
Outdated
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 |
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.
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.
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.
@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.
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.
@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).
Summary
Addresses CXX-3199.
Refactors the
doxygen-current
anddoxygen-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 thedocs_source_directory
doxygen-install-headers
target that is important). Thedocs_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
andlib
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 thedoxygen-latest
target which uses a temporary directory, but may cause some inconvenience for thedoxygen-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 ofDoxygen::doxygen
, but this variable is still used for the internal call tofind_program()
).The following command thus generates the current API doc pages for local preview:
Latest API Docs
The
generate-*
Perl scripts are completely removed in favor of a Bash script toremovereduce our toolchain dependency on Perl (still used fordeploy-to-ghpages.pl
). Following #1169, thegenerate-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. Thegenerate-apidocs-from-tag.pl
was only being used to implement the behavior ofgenerate-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.