-
Notifications
You must be signed in to change notification settings - Fork 547
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
ghost
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.
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
HEADand 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--localis 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.
kevinAlbs
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.
Apologies for the delayed review. Migrating perl scripts to bash is much appreciated.
vector-of-bool
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.
LGTM
kevinAlbs
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.
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-currentanddoxygen-latesttargets 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_directorydoxygen-install-headerstarget that is important). Thedocs_source_directorydoxygen-install-headerstarget takes advantage of the symmetric directory layout implemented by #1026 and raw-copies the<lib>/includeand<lib>/libdirectories 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/doxygendirectory 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
includeandlibdirectories without any filters. If there are any stray.hppheader files in either directory, they will be blindly copied as well. This is not an issue for thedoxygen-latesttarget which uses a temporary directory, but may cause some inconvenience for thedoxygen-currenttarget.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_EXECUTABLEvariable (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.pleffectively 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.plwas only being used to implement the behavior ofgenerate-latest-apidocs.pland 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.
sedis 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.