Skip to content
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

Convert package-level and component-level documentation into form Doxygen can understand #242

Merged
merged 9 commits into from
Apr 17, 2024

Conversation

pniedzielski
Copy link
Collaborator

@pniedzielski pniedzielski commented Apr 8, 2024

Issue number of the reported bug or feature request: #118

Describe your changes
The changes in this PR convert our package-level and component-level documentation into directory and file documentation, respectively, so that Doxygen is able to understand them. Please see the messages on each commit for further details.

Note that I have not included the built documentation files, which live in docs/docs/apidocs/cpp_apidocs/. We may want to keep these on a separate branch (gh-pages?), and the current output of Doxygen is still not optimal (see Future changes below for details), so I will not modify these in this PR.

Testing performed
Testing the documentation build by running doxygen manually from the root of the repository. Using Doxygen version 1.9.8, the only warnings left are because we use the project's README.md as our API docs' starting point. This should be changed.

Future changes
A few more changes need to be made to the documentation so we can automatically generate it on release:

  1. Catch conversion errors from BDE-style documentation to Doxygen documentation. As noted in the linked issue, there are some places which confused the automated conversion tool, leaving us with old-style BDE formatting that Doxygen cannot understand. These should be changed.
  2. Add a real \mainpage, which links to each of the packages for easier discoverability. This will also fix the remaining Doxygen warnings.
  3. Teach CMake to find and run Doxygen. This will let us call the equivalent of make doc to generate documentation, which we can do in our CI. This will let us catch documentation errors in CI and ultimately publish the documentation automatically on releases.
  4. (As a nice-to-have,) Convert BDE class member grouping to a form Doxygen understands. Doxygen provides the ability to divide class members into arbitrary groups with names. If this change is not too intrusive, we can consider doing this to make our API docs easier to understand.

@pniedzielski pniedzielski added documentation Improvements or additions to documentation enhancement New feature or request A-Docs Area: Documentation labels Apr 8, 2024
@pniedzielski pniedzielski requested a review from a team as a code owner April 8, 2024 22:50
@pniedzielski pniedzielski requested review from hallfox and removed request for kaikulimu and dorjesinpo April 9, 2024 15:26
@hallfox
Copy link
Collaborator

hallfox commented Apr 16, 2024

Looks pretty good! I'm happy with the rendered output, even though we've lost the component level breakdown in packages. A few things I'm noticing:

  1. Package level documentation appears to be rendering as README links in the related pages section without any of the contents in the README. Is this expected? (Or am I seeing something weird?)
  2. Several "impl" classes are visible in the documentation that weren't visible in the previous one. My guess is there was some rule in bdedox that was excluding them by the nature of that name, or some option we had enabled that was removed that I can't see in this PR. Is it likely we'll be able to hide these again? Or should we find other ways to mark them as internal only? Seems like doxygen's \internal command might help us here.

@pniedzielski
Copy link
Collaborator Author

I'm happy with the rendered output, even though we've lost the component level breakdown in packages.

Yeah, IIRC component levelization was not part of the OSS release (see ed9fa8e for the only other commit to these READMEs), but it would be nice to add it back in. I believe the currently published levelization is incomplete, but I would need to check again.

  1. Package level documentation appears to be rendering as README links in the related pages section without any of the contents in the README. Is this expected? (Or am I seeing something weird?)

This is "expected", but I haven't found a good way around it. See doxygen/doxygen#9437, it's a known issue. Doesn't help us much though.

  1. Several "impl" classes are visible in the documentation that weren't visible in the previous one. My guess is there was some rule in bdedox that was excluding them by the nature of that name, or some option we had enabled that was removed that I can't see in this PR. Is it likely we'll be able to hide these again? Or should we find other ways to mark them as internal only? Seems like doxygen's \internal command might help us here.

\internal is a good solution here. Since in BDE-style docs, the publically-usable symbols are listed in component-level documentation, it may be that bdedox only generates documentation for those symbols listed in the component-level documentation. Either way, we removed that list in this PR intentionally, so \internal is a good idea.

I don't have a good solution to the first issue (I was thinking of it lumped together with our \mainpage being a broken version of the project README as a "later" problem...), but I'll do the latter in this PR.

@hallfox
Copy link
Collaborator

hallfox commented Apr 16, 2024

I don't have a good solution to the first issue (I was thinking of it lumped together with our \mainpage being a broken version of the project README as a "later" problem...), but I'll do the latter in this PR.

I'm okay with this for now.

I think we should keep hiding the internal components.

Running `doxygen` on our documentation right now search all header files
and source files in the public packages within the `bmq` package group.
We already exclude the test files for each component (`*.t.cpp`) from
our search, but this still means that for each component, we generate
pages for both its `*.cpp` file and its `*.h` file.

For the purpose of generating documentation, though, we only need to
search the header file, which contains both component-level
documentation (currently in the BDE format, which Doxygen cannot
process) and type/function-level documentation (which was partially
converted into a format Doxygen can process).  The `.cpp` source file
for each component contains no documentation, and so the file
documentation that is generated only contains only the implementation
source code, which a user can easily look up in our GitHub repository.

This patch makes the simple change of excluding the `.cpp` source files
for each component from Doxygen’s search.  This prevents Doxygen from
generating HTML pages for each implementation source file, which contain
no new or interesting information above the source file itself.  This
will also allow us to generate component-level documentation as
documentation of the component’s header file itself, once we convert the
component-level documentation into a format that Doxygen can handle.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
All BlazingMQ components live within the namespace `BloombergLP`, under
which package namespaces `bmqa`, `bmqp`, etc. live.  In most places in
our current documentation, though, this qualifier is elided: the class
`BloombergLP::bmqa::AbstractSession` is referred to only as
`bmqa::AbstractSession`.  While this is usually not an issue, if we want
Doxygen to generate links to the documentation for these symbols, we
must prefix them with `BloombergLP::`.  This gives us two options:

  1. Any time we reference a symbol in our documentation, we must make
     sure to that we reference its fully-qualified name, including the
     `BloombergLP::` namespace prefix.  These changes would take the
     form

         … 'bmqa::AbstractSession' …
     →
         … @ref BloombergLP::bmqa::AbstractSession ….

     This will make our documentation more noisy and harder to read.

  2. We add our own Doxygen command alias that takes a symbol name and
     generates the above.  We will have to make sure to use this alias
     whenever we modify our Doxygen documentation, or symbol references
     will not work correctly.  Then, changes would take the form

         … 'bmqa::AbstractSession' …
     →
         … @Bbref{bmqa::AbstractSession} …,

     which is only a little noisier than what we have now.

This patch takes the second approach, and introduces a Doxygen command
alias named `bbref`, which performs the substition described above.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Now that component-level documentation has been conflated with header
file-level documentation, the component-level documentation is buried
within the `Files` section in the left-hand navigation pane of our
generated Doxygen documentation.  Several levels of hierarchy within
this output are noise: namely, we do not need the `src/groups/bmq`
directories that are incidental to the logical hierarchy of our
components.

This patch teaches Doxygen to strip out those directories from the names
of files it searches, yielding a documentation structure that is closer
to the BDE-style package and component hierarchy.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
During the open-sourcing process, we converted all class and function
documentation over from the BDE documentation style to Doxygen +
Markdown using an automated tool.  This tool did not affect
package-group-level documentation, package-level documentation, or
component-level documentation, though, which is still in the previous
BDE style, and which is thus not picked up when we run Doxygen.  This is
a big problem, since much of our SDK’s documentation, as well as
examples, are given in component-level documentation rather than on the
concrete classes, functions, or macros.  This also means we cannot run
Doxygen on our source code to produce updated versions of the
documentation when we release, meaning our documentation is still stuck
at where it was with our first open-source release.

This patch performs the first big step towards rectifying this
situation: it converts the old BDE-style component-level documentation
into Doxygen + Markdown documentation, applied to the header file
associated with each component.  After applying this patch, Doxygen is
able to pick up the component-level documentation and to produce
formatted pages for each component.

Documenting components in Doxygen
=================================

Doxygen has no native support for BDE-style organization, with package
groups, packages, and components, so we need to map these organizational
concepts onto Doxygen’s more impoverished and concrete ontology.  There
are two rough ways of going about this, described below.

The first strategy is the one that we employed before converting the
documentation on classes, functions, and macros from BDE-style to
Doxygen + Markdown format: use the `bdedox` tool from the
[bde-tools][bde-tools] repository, which to runs a filter program over
each package group, package, and component in the repository.  Using the
documentation embedded within a BDE-style repository, this filter
produces temporary Markdown files package groups, packages, and
components.  Those temporary Markdown files then serve as additional
input to Doxygen, rendering as separate Doxygen _pages_—free-form
documentation files that Doxygen imposes no structure or hierarchy on.
`bdedox`, however, expects all documentation to be in BDE-style,
including class- and function-level documentation; since we converted
both of these into Doxygen style, we cannot use the tool.  The only way
to follow this method of teaching Doxygen to treat our component-level
documentation as pages is to write our own filter program.  This is a
larger undertaking to support a legacy documentation format, which we
have already half-converted away from.

The second and alternative strategy is to map package groups, packages,
and components onto entities that Doxygen already natively understands.
We elected to use this strategy for this patch.  However, in the case of
components, there is no obvious C++-level entity that uniquely
corresponds to a component.  Instead, in this patch, we commit the sin
of conflating the header file associated with a component with the
component itself.  That is, we apply _component-level documentation_ to
the _component header file_.  After applying this patch, then, the
header file within a component (say, `bmqa_component`) will have a
Doxygen-style comment near its top that starts with `@file
bmqa_component.h`, and which then contains the documentation for
`bmqa_component`.

This approach, though easier, has its downsides.

  1. First, when referencing the documentation for another component in
     the library, we have to reference the header file name rather than
     the component name.  So, if some documentation comment wants to
     point to the documentation for component `bmqa_component`, it must
     reference it with `@ref bmqa_component.h`.  This might be
     confusing to users familiar with our BDE-style organization, but
     if we prefer, we can render it correctly with
     `\[bmqa_component\](@ref bmqa_component.h)`, at the cost of
     noisier documentation comments.

  2. Second, when generated, component-level documentation will be found
     under the `Files` documentation in the left-hand navigation column,
     buried within several subdirectories.  We can partially solve this
     by configuring Doxygen to strip the unnecessary subdirectories from
     file paths, but users may still not be inclined to look under
     `Files` for important documentation.  I cannot see a way to rename
     this portion of the documentation from `Files` to `Components`.

[bde-tools]: https://github.com/bloomberg/bde-tools

Notes on Documentation Markup
=============================

Doxygen understands Markdown and HTML formatting rather than BDE-style
documentation, so we have manually converted BDE-style markup to
Markdown and, in more advanced cases, HTML.  The resulting HTML is
less-readable than BDE-style markup, but produces easier-to-understand
Doxygen documentation.

Sections within the component-level documentation are rendered using
Markdown headings, with unique IDs, constructed by prefixing the
component name with a short identifier.  Unfortunately, even if they are
not referenced from other components, these IDs must be unique across
the entire project, meaning we cannot get by with shorter identifiers.
Doxygen will warn us when it encounters an identifier that is not
unique, though, which is a good way to catch these issues.  Within the
same documentation comment, we can link to a section just using a simple
Markdown link: `\[some link\](#bmqa_component_section)`.  If we want to
link to a section in a different component, we can use Doxygen’s
referencing capability: `\[some link\](@ref #bmqa_component_section)`.

The biggest benefit of Doxygen is that we do not need to list the
entities that are exposed by some component manually in the
documentation comment; instead, Doxygen will collect these itself and
produce the list for us.  As such, we have removed the `@CLASSES`
section from the component-level documentation.  At the moment, this is
not a perfect substitution, as some private entities witehin components
are not properly marked as private, and are thus included within the
generated entity lists.  We will fix this with a later patch.

We have mapped BDE `@PURPOSE` sections to Doxygen `@brief` documentation
and `@DESCRIPTION` sections to Doxygen long documentation (which isn’t
explicitly marked in a Doxygen documentation comment).  We have also
used the Doxygen `@see` command to implement BDE `@SEE_ALSO` sections.

Testing
=======

The changes in this patch are simple in principle, but were performed
manually.  Verifying that the changes in this patch are correct was done
by generating the documentation by running `doxygen` from the root
directory of this repository, ensuring that no new warnings or errors
are produced by it, and then manually reviewing the generated
documentation for its fidelity.  This last step is tedious and prone to
mistakes, so we relied heavily on the warnings from Doxygen to ensure
our documentation was correct.

Despite this, there are still a few warnings that are produced.  Doxygen’s
parser seems to be confused by several BDE macros, which it cannot find the
definitions of.  These warnings can be fixed by predefining those macros, but
this is left to a future patch.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Just as a previous patch documents components using Doxygen’s file
documentation support, this patch converts package-level documentation
into Doxygen’s directory documentation.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Our Doxygen is not configured to search through BDE’s header files for
macros to expand, so they are passed, unexpanded, on to Doxygen’s
parser.  In certain cases, this confuses the parser: for instance, the
lack of a semicolon after `BSLMF_NESTED_TRAIT_DECLARATION(..)‘ uses
results in an apparent syntax error, which causes Doxygen to build our
documentation with warnings.  Other BDE macros that are meant to enable
C++11 and later features to be used only when available, like
`BSLS_KEYWORD_FINAL`, are also unexpanded, and result in Doxygen failing
to properly document symbols to which they are attached.

This patch teaches Doxygen to ignore or properly expand the above two
macros, removing several warnings from our documentation builds.  There
may be more macros that need to be added, but these are the only ones
for which Doxygen issued warnings.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
Several Doxygen configuration elements that we do not use have been
deprecated or removed from later versions of Doxygen.  When building
with these versions, Doxygen would warn of their presence.

This patch runs `doxygen -u` on our `Doxyfile`, updating it to remove
these deprecated or removed configurations.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
In our existing published documentation, generated by `bdedox` from the
old component-level documentation comments, documentation for `Impl`
types is excluded.  After conversion, these types have Doxygen-style
documentation comments, and as such are picked up by Doxygen when we
build documentation now.  This patch marks all such types explicitly as
`@private`, so we do not generate their documentation by default.  This
does give us the option of generating internal builds of the
documentation which include these types, if we ever need such builds.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
In several places in our SDK, we need to forward declare a symbol from
one of the implementation packages (e.g., `bmqimp`).  These packages do
not form part of the public API of `libbmq`, and we do not want to
include them in the documentation.  Unfortunately, it looks like the
best way to do this is to manually and explicitly exclude the namespaces
for these symbols in the `Doxyfile`.  This patch excludes the namespaces
of components outside of `bmqa`, `bmqpi`, and `bmqt` that we need to
forward-declare, including private `libbmq` packages and BDE packages.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
@pniedzielski
Copy link
Collaborator Author

@hallfox Rebased onto main, but included two additional commits that address internal components being shown in the generated documentation.

Copy link
Collaborator

@hallfox hallfox left a comment

Choose a reason for hiding this comment

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

lgtm

@pniedzielski pniedzielski merged commit 9b033c2 into bloomberg:main Apr 17, 2024
10 checks passed
@pniedzielski pniedzielski deleted the doxygen/bmqa branch April 17, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Docs Area: Documentation documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants