[libcamera-devel] [PATCH v2 4/5] libcamera: Documentation: Split public/private documentation
Dan Scally
dan.scally at ideasonboard.com
Wed Jan 10 09:49:26 CET 2024
Morning Kieran
On 09/01/2024 22:08, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2024-01-09 14:28:07)
>> Hi Dan
>> I really like the split!
>>
>> On Fri, Jan 05, 2024 at 04:41:03PM +0000, Daniel Scally via libcamera-devel wrote:
>>> The API reference pages generated by Doxygen are comprehensive, but
>>> therefore quite overwhelming for application developers who will
>>> likely never need to use the majority of the library's objects. To
>>> reduce the complexity of the documentation, split it into two runs of
>>> doxygen.
>>>
>>> In the first run of doxygen we pass a specific list of source files
>>> to parse, which is built from the File arrays in Meson's build files.
>>> This ensures that we only generate the documentation for code from
>>> those files.
>>>
>>> In the second run allow doxygen to generate documentation for all of
>>> the library's objects as it currently does. This set will now be
>>> output into build/Documentation/internal-api-html.
>>>
>>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>>> ---
>>> Changes in v2:
>>>
>>> - Formatting fixes (Jacopo)
>>> - Phraseology (Laurent)
>>> - Switched to passing specific files to parse to doxygen rather than
>>> relying on \internal to remove other docu-comments.
>>>
>>> Documentation/Doxyfile-internal.in | 21 +++++++
>>> Documentation/Doxyfile-public.in | 5 ++
>>> Documentation/Doxyfile.in | 26 ++-------
>>> Documentation/meson.build | 76 +++++++++++++++++++++++---
>>> include/libcamera/base/meson.build | 7 +++
>>> include/libcamera/internal/meson.build | 7 +++
>>> include/libcamera/meson.build | 10 ++++
>>> meson.build | 8 +++
>>> src/libcamera/base/class.cpp | 1 +
>>> src/libcamera/base/meson.build | 7 +++
>>> src/libcamera/camera.cpp | 7 +++
>>> src/libcamera/camera_manager.cpp | 1 +
>>> src/libcamera/framebuffer.cpp | 6 +-
>>> src/libcamera/meson.build | 7 +++
>>> src/libcamera/request.cpp | 1 +
>>> 15 files changed, 160 insertions(+), 30 deletions(-)
>>> create mode 100644 Documentation/Doxyfile-internal.in
>>> create mode 100644 Documentation/Doxyfile-public.in
>>>
>>> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
>>> new file mode 100644
>>> index 00000000..7b3cce49
>>> --- /dev/null
>>> +++ b/Documentation/Doxyfile-internal.in
>>> @@ -0,0 +1,21 @@
>>> +# SPDX-License-Identifier: CC-BY-SA-4.0
>>> +
>>> +INPUT = "@TOP_SRCDIR@/Documentation" \
>>> + "@TOP_SRCDIR@/include/libcamera" \
>>> + "@TOP_SRCDIR@/src/ipa/ipu3" \
>>> + "@TOP_SRCDIR@/src/ipa/libipa" \
>>> + "@TOP_SRCDIR@/src/libcamera" \
>>> + "@TOP_BUILDDIR@/include/libcamera" \
>>> + "@TOP_BUILDDIR@/src/libcamera"
>>> +
>>> +EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \
>> I'm still a bit puzzled on why we don't document Span<>, but this was
>> already here
> Presumably this is expected to be covered by the C++ definition of a
> Span. It's supposed to be a 'like for like' implementation. (or as close
> as possible I think).
>
> Perhaps somewhere we should document that fact. In the 'long future'
> (when we can support C++20 - I would expect we'd convert to use
> std::span in place of libcamera::Span.
>
>>> + @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>>> + @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>>> + @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
>>> + @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>>> + @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>>> + @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
>>> + @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>> + @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>> + @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>> + @TOP_BUILDDIR@/src/libcamera/proxy/
>>> diff --git a/Documentation/Doxyfile-public.in b/Documentation/Doxyfile-public.in
>>> new file mode 100644
>>> index 00000000..cdbc03a0
>>> --- /dev/null
>>> +++ b/Documentation/Doxyfile-public.in
>>> @@ -0,0 +1,5 @@
>>> +# SPDX-License-Identifier: CC-BY-SA-4.0
>>> +
>>> +INPUT = "@TOP_SRCDIR@/Documentation" \
>>> + "@INPUT@" \
>>> + "@TOP_BUILDDIR@/src/libcamera/version.cpp"
>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>>> index 48fea8bc..a271c7bc 100644
>>> --- a/Documentation/Doxyfile.in
>>> +++ b/Documentation/Doxyfile.in
>>> @@ -21,14 +21,6 @@ CASE_SENSE_NAMES = YES
>>>
>>> QUIET = YES
>>>
>>> -INPUT = "@TOP_SRCDIR@/Documentation" \
>>> - "@TOP_SRCDIR@/include/libcamera" \
>>> - "@TOP_SRCDIR@/src/ipa/ipu3" \
>>> - "@TOP_SRCDIR@/src/ipa/libipa" \
>>> - "@TOP_SRCDIR@/src/libcamera" \
>>> - "@TOP_BUILDDIR@/include/libcamera" \
>>> - "@TOP_BUILDDIR@/src/libcamera"
>>> -
>>> FILE_PATTERNS = *.c \
>>> *.cpp \
>>> *.h \
>>> @@ -36,17 +28,8 @@ FILE_PATTERNS = *.c \
>>>
>>> RECURSIVE = YES
>>>
>>> -EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \
>>> - @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>>> - @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>>> - @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
>>> - @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>>> - @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>>> - @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
>>> - @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>> - @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>> - @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>> - @TOP_BUILDDIR@/src/libcamera/proxy/
>>> + at INCLUDE_PATH = @TOP_BUILDDIR@/Documentation
>>> + at INCLUDE = @INCLUDE_FILE@
>>>
>>> EXCLUDE_PATTERNS = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>>> @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
>>> @@ -70,7 +53,10 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \
>>>
>>> EXCLUDE_SYMLINKS = YES
>>>
>>> -HTML_OUTPUT = api-html
>>> +HIDE_UNDOC_CLASSES = @HIDE_UNDOC_CLASSES@
>>> +HIDE_UNDOC_MEMBERS = @HIDE_UNDOC_MEMBERS@
>>> +HTML_OUTPUT = @HTML_OUTPUT@
>>> +INTERNAL_DOCS = @INTERNAL_DOCS@
>>>
>>> GENERATE_LATEX = NO
>>>
>>> diff --git a/Documentation/meson.build b/Documentation/meson.build
>>> index 7a58fec8..afaad751 100644
>>> --- a/Documentation/meson.build
>>> +++ b/Documentation/meson.build
>>> @@ -23,12 +23,7 @@ if doxygen.found() and dot.found()
>>>
>>> cdata.set('PREDEFINED', ' \\\n\t\t\t '.join(doxygen_predefined))
>>>
>>> - doxyfile = configure_file(input : 'Doxyfile.in',
>>> - output : 'Doxyfile',
>>> - configuration : cdata)
>>> -
>>> - doxygen_input = [
>>> - doxyfile,
>>> + global_doxygen_input = [
>>> libcamera_base_headers,
>>> libcamera_base_sources,
>>> libcamera_internal_headers,
>>> @@ -41,16 +36,79 @@ if doxygen.found() and dot.found()
>>> ]
>>>
>>> if is_variable('ipu3_ipa_sources')
>>> - doxygen_input += [ipu3_ipa_sources]
>>> + global_doxygen_input += [ipu3_ipa_sources]
>>> endif
>>>
>>> + # We need to generate two "include" files for the final Doxyfile which
>>> + # define a set of source files to use in the documentation parsing. We
>>> + # collected a list of the public sources in doxygen_public_sources, so we
>>> + # pass that to the doxyfiles so that Doxyfile-public refers only to those
>>> + # files. Although INPUT is sent to both, Doxyfile-internal.in doesn't refer
>>> + # to it and just hardcodes the directories to parse.
>> Thanks, this is quite clear now!
>>
>>> + cdata.set('INPUT', '" \\\n\t\t\t "'.join(doxygen_public_sources))
>>> + doxyfile_include_public = configure_file(input : 'Doxyfile-public.in',
>>> + output : 'Doxyfile-include-public',
>>> + configuration : cdata)
>>> + doxyfile_include_internal = configure_file(input : 'Doxyfile-internal.in',
>>> + output : 'Doxyfile-include-internal',
>>> + configuration : cdata)
>>> +
>>> + # We run doxygen twice - the first run excludes internal API objects as it
>>> + # is intended to document the public API only. A second run covers all of
>>> + # the library's objects for libcamera developers. To achieve this we need to
>>> + # flag as \internal some of the comments for objects which we wish to hide,
>>> + # and remove the auto generated documents via HIDE_UNDOC_CLASSES and
>>> + # HIDE_UNDOC_MEMBERS.
>>> +
>>> + cdata_public = configuration_data()
>>> + cdata_public.merge_from(cdata)
>>> + cdata_public.set('HIDE_UNDOC_CLASSES', 'YES')
>>> + cdata_public.set('HIDE_UNDOC_MEMBERS', 'YES')
>>> + cdata_public.set('HTML_OUTPUT', 'api-html')
>>> + cdata_public.set('INCLUDE_FILE', 'Doxyfile-include-public')
>>> + cdata_public.set('INTERNAL_DOCS', 'NO')
>>> +
>>> + doxyfile_public = configure_file(input : 'Doxyfile.in',
>>> + output : 'Doxyfile-public',
>>> + configuration : cdata_public)
>>> +
>>> + public_doxygen_input = global_doxygen_input
>>> + public_doxygen_input += doxyfile_public
>>> +
>>> custom_target('doxygen',
>>> - input : doxygen_input,
>>> + input : public_doxygen_input,
>>> output : 'api-html',
>>> - command : [doxygen, doxyfile],
>>> + command : [doxygen, doxyfile_public],
>>> install : true,
>>> install_dir : doc_install_dir,
>>> install_tag : 'doc')
>>> +
>>> + # This is the internal documentation, so _don't_ hide undocumented classes
>>> + # as we want everything to show up and warnings to be generated if any
>>> + # documentation is missing.
>>> +
>>> + cdata_internal = configuration_data()
>>> + cdata_internal.merge_from(cdata)
>>> + cdata_internal.set('HIDE_UNDOC_CLASSES', 'NO')
>>> + cdata_internal.set('HIDE_UNDOC_MEMBERS', 'NO')
>>> + cdata_internal.set('HTML_OUTPUT', 'internal-api-html')
>>> + cdata_internal.set('INCLUDE_FILE', 'Doxyfile-include-internal')
>>> + cdata_internal.set('INTERNAL_DOCS', 'YES')
>>> +
>>> + doxyfile_internal = configure_file(input : 'Doxyfile.in',
>>> + output : 'Doxyfile-internal',
>>> + configuration : cdata_internal)
>>> +
>>> + internal_doxygen_input = global_doxygen_input
>>> + internal_doxygen_input += doxyfile_internal
>>> +
>>> + custom_target('doxygen-internal',
>>> + input : internal_doxygen_input,
>>> + output : 'internal-api-html',
>>> + command : [doxygen, doxyfile_internal],
>>> + install : true,
>>> + install_dir : doc_install_dir,
>>> + install_tag : 'doc-internal')
>>> endif
>>>
>>> #
>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
>>> index f24f47de..82277f46 100644
>>> --- a/include/libcamera/base/meson.build
>>> +++ b/include/libcamera/base/meson.build
>>> @@ -38,3 +38,10 @@ libcamera_base_headers = [
>>>
>>> install_headers(libcamera_base_public_headers,
>>> subdir : libcamera_base_include_dir)
>>> +
>>> +foreach lbph : libcamera_base_public_headers
>>> + doxygen_public_sources += '/'.join(
>>> + meson.project_source_root(),
>>> + '@0@'.format(lbph)
>>> + )
>>> +endforeach
>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>>> index 4d59cb2a..4af36884 100644
>>> --- a/include/libcamera/internal/meson.build
>>> +++ b/include/libcamera/internal/meson.build
>>> @@ -58,4 +58,11 @@ libcamera_internal_headers = [
>>> libcamera_internal_headers_publically_undocumented
>>> ]
>>>
>>> +foreach lph : libcamera_internal_headers_publically_documented
> I think I saw a suggestion somewhere to drop _publically anyway, but
> otherwise I think the correct spelling is 'publicly'.
Derp - thanks!
>
>>> + doxygen_public_sources += '/'.join(
>>> + meson.project_source_root(),
>>> + '@0@'.format(lph)
>>> + )
>>> +endforeach
>>> +
>>> subdir('converter')
>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>>> index bab858a3..25e2f8a4 100644
>>> --- a/include/libcamera/meson.build
>>> +++ b/include/libcamera/meson.build
>>> @@ -26,6 +26,13 @@ subdir('ipa')
>>> install_headers(libcamera_public_headers,
>>> subdir : libcamera_include_dir)
>>>
>>> +foreach lph : libcamera_public_headers
>>> + doxygen_public_sources += '/'.join(
>>> + meson.project_source_root(),
>>> + '@0@'.format(lph)
>>> + )
> I'm surprised meson doesn't have better helpers for this.
> It might be worth a check through with meson to see if there's a cleaner
> way to handle this. But otherwise, if this is the only way to get the
> fully qualified paths if that's what's needed...
It does; File.full_path() [1], but the problem is that it wasn't added until Meson 1.4.0 and Laurent
was concerned that's too recent - quite reasonably, apparently, since my Ubuntu meson package only
gives me 0.61.2. This method is apparently available back to 0.54.0
[1] https://mesonbuild.com/Reference-manual_returned_file.html
>
>>> +endforeach
>>> +
>>> #
>>> # Generate headers from templates.
>>> #
>>> @@ -85,6 +92,7 @@ foreach mode, entry : controls_map
>>> '-r', ranges_file, '@INPUT@'],
>>> install : true,
>>> install_dir : libcamera_headers_install_dir)
>>> + doxygen_public_sources += control_headers.get(-1).full_path()
> What's going on here? (Why a .get(-1) ?)
We're within a foreach loop that adds a CustomTarget to the control_headers array, but doesn't
create a new variable for it, so to use it I'm just fetching the last entry from the array. A fuller
context for that change would be:
control_headers += custom_target(header + '_h',
input : input_files,
output : outfile,
command : [gen_controls, '-o', '@OUTPUT@',
'--mode', mode, '-t', template_file,
'-r', ranges_file, '@INPUT@'],
install : true,
install_dir : libcamera_headers_install_dir)
doxygen_public_sources += control_headers.get(-1).full_path()
Alternatively I could create a local variable for the custom target and use that, like so:
ct = custom_target(header + '_h',
input : input_files,
output : outfile,
command : [gen_controls, '-o', '@OUTPUT@',
'--mode', mode, '-t', template_file,
'-r', ranges_file, '@INPUT@'],
install : true,
install_dir : libcamera_headers_install_dir)
control_headers += ct
doxygen_public_sources += ct.full_path()
But it didn't seem worth it.
>
>>> endforeach
>>>
>>> libcamera_public_headers += control_headers
>>> @@ -101,6 +109,7 @@ formats_h = custom_target('formats_h',
>>> install : true,
>>> install_dir : libcamera_headers_install_dir)
>>> libcamera_public_headers += formats_h
>>> +doxygen_public_sources += formats_h.full_path()
>>>
>>> # libcamera.h
>>> libcamera_h = custom_target('gen-header',
>>> @@ -111,6 +120,7 @@ libcamera_h = custom_target('gen-header',
>>> install_dir : libcamera_headers_install_dir)
>>>
>>> libcamera_public_headers += libcamera_h
>>> +doxygen_public_sources += libcamera_h.full_path()
>>>
>>> # version.h
>>> version = libcamera_version.split('.')
>>> diff --git a/meson.build b/meson.build
>>> index e49de4c2..cca1883e 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -231,6 +231,14 @@ endif
>>> # Utilities are parsed first to provide support for other components.
>>> subdir('utils')
>>>
>>> +# To support auto-generation of documentation We need an array of the paths to
>> s/We/we
>>
>>> +# public headers and source files so that we can tell doxygen which files to
>>> +# look at later. Unfortunately the inclusion of custom targets in some of the
>>> +# existing arrays precludes using them directly and you cannot generate File
>>> +# objects from generated files, so we need to collect paths to relevant files
>>> +# within an array.
>>> +doxygen_public_sources = []
> Aha, that might have answered my question above.
>
>>> +
>>> subdir('include')
>>> subdir('src')
>>>
>>> diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp
>>> index 9c2d9f21..70fd5cd5 100644
>>> --- a/src/libcamera/base/class.cpp
>>> +++ b/src/libcamera/base/class.cpp
>>> @@ -184,6 +184,7 @@ Extensible::Extensible(std::unique_ptr<Extensible::Private> d)
>>> */
>>>
>>> /**
>>> + * \internal
>>> * \class Extensible::Private
>>> * \brief Base class for private data managed through a d-pointer
>>> */
>>> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
>>> index 523c5885..ae949a51 100644
>>> --- a/src/libcamera/base/meson.build
>>> +++ b/src/libcamera/base/meson.build
>>> @@ -30,6 +30,13 @@ libcamera_base_sources = [
>>> libcamera_base_internal_sources
>>> ]
>>>
>>> +foreach lbps : libcamera_base_public_sources
>>> + doxygen_public_sources += '/'.join(
>>> + meson.project_source_root(),
>>> + '@0@'.format(lbps)
>>> + )
>>> +endforeach
>>> +
>>> libdw = dependency('libdw', required : false)
>>> libunwind = dependency('libunwind', required : false)
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 0ad1a4b5..ed46f853 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -560,6 +560,13 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>>> */
>>>
>>> /**
>>> + * \internal
>>> + * \file libcamera\internal\camera.h
>>> + * \brief Internal Camera device handling
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> * \class Camera::Private
>>> * \brief Base class for camera private data
>>> *
>>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>>> index 355f3ada..61d45256 100644
>>> --- a/src/libcamera/camera_manager.cpp
>>> +++ b/src/libcamera/camera_manager.cpp
>>> @@ -23,6 +23,7 @@
>>> */
>>>
>>> /**
>>> + * \internal
>>> * \file libcamera/internal/camera_manager.h
>>> * \brief Internal camera manager support
>>> */
>> That's weird, I don't see CameraManager::Private::addCamera() and
>> CameraManager::Private::removeCamera() being documented. It was not
>> documented even before this commit though :/
>>
>>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
>>> index 5a7f3c0b..db450e11 100644
>>> --- a/src/libcamera/framebuffer.cpp
>>> +++ b/src/libcamera/framebuffer.cpp
>>> @@ -16,7 +16,10 @@
>>> /**
>>> * \file libcamera/framebuffer.h
>>> * \brief Frame buffer handling
>>> - *
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>> * \file libcamera/internal/framebuffer.h
>>> * \brief Internal frame buffer handling support
>>> */
>>> @@ -105,6 +108,7 @@ LOG_DEFINE_CATEGORY(Buffer)
>>> */
>>>
>>> /**
>>> + * \internal
>>> * \class FrameBuffer::Private
>>> * \brief Base class for FrameBuffer private data
>>> *
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index 676470c1..413e14db 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -59,6 +59,13 @@ libcamera_sources = [
>>> libcamera_internal_sources
>>> ]
>>>
>>> +foreach lps : libcamera_public_sources
>>> + doxygen_public_sources += '/'.join(
>>> + meson.project_source_root(),
>>> + '@0@'.format(lps)
>>> + )
>>> +endforeach
>>> +
>>> libcamera_sources += libcamera_public_headers
>>> libcamera_sources += libcamera_generated_ipa_headers
>>> libcamera_sources += libcamera_tracepoint_header
>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>>> index 949c556f..930d8c92 100644
>>> --- a/src/libcamera/request.cpp
>>> +++ b/src/libcamera/request.cpp
>>> @@ -33,6 +33,7 @@ namespace libcamera {
>>> LOG_DEFINE_CATEGORY(Request)
>>>
>>> /**
>>> + * \internal
>>> * \class Request::Private
>>> * \brief Request private data
>>> *
>> So we should remember to mark every classes that derives a private
>> implementation from Extensible to mark it as \internal. I think it's
>> fine, and I like the patch a lot!
>>
> Likewise, I suspect we can try to come up with a checkstyle reminder
> sometime.
Yeah I was going to take a look at that when time allowed
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Thanks!
>
>> With the few above points clarified
>> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>
>> Thanks
>> j
>>
>>> --
>>> 2.34.1
>>>
More information about the libcamera-devel
mailing list