[libcamera-devel] [PATCH 3/2] libcamera: deprecated: Add new deprecated header

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, May 23, 2023 at 12:19:48PM +0100, Kieran Bingham via libcamera-devel wrote:
> Provide a new header that will be included when applications
> use <libcamera/libcamera.h> to bring in additional context and warnings
> to generate when the libcamera API is modified where possible.
> 
> Introduce a deprecated implemenation of StreamRoles to report

s/implemenation/implementation/

> that StreamRoles is no longer a libcamera definition.
> 
> This allows the compiler to report the following when the header is
> available:
> 
> ../../src/apps/qcam/main_window.cpp: In member function ‘int MainWindow::startCapture()’:
> ../../src/apps/qcam/main_window.cpp:366:21: error: ‘using StreamRoles = class std::vector<libcamera::StreamRole>’ is deprecated: Use a span, array or vector directly [-Werror=deprecated-declarations]
>   366 |         StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
>       |                     ^~~~~
> In file included from include/libcamera/libcamera.h:16,
>                  from ../../src/apps/qcam/main_window.cpp:14:
> ../../include/libcamera/deprecated.h:21:7: note: declared here
>    21 | using StreamRoles [[deprecated("Use a span, array or vector directly")]]
>       |       ^~~~~~~~~~~
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> This is a proposal to support reporting deprecated functionality to
> applications at compile time.

This is an interesting indeed, I quite like it. I have a few comments
though (which I'm sure doesn't surprise you :-)).

First, I don't think we will be able to easily move all deprecated
features to a separate header. It's easy in this case, but it won't work
for a class member for instance. Maybe a macro that simplifies usage of
the [[deprecated]] attribute would be an alternative, especially if that
macro could take the version number in which the feature will be dropped
to enforce proper scheduling of feature deprecation.

Then, I wonder if application developers may complain, as a deprecation
warning would break builds that use -Werror. This could be considered as
breaking applications. Of course we could argue that deprecation
warnings should be treated as warnings by our users, not as errors, but
we can't enforce or control that. I'm not thinking here about the short
term, as breaking application build due to a deprecation warning isn't
worse than breaking it due to removal of the feature, but once we'll
guarantee ABI stability I expect us to accumulate deprecated features
over the lifetime of a major release. Maybe making the deprecation
helper macro I mentioned above conditional upon a type of build (debug
vs. release) or a configuration parameter could solve this problem. We
could also check how this is handled by other projects.

Finally, do we want to introduce deprecation warnings already ? I expect
lots of ABI changes, and this may slow us down.

>  include/libcamera/deprecated.h | 24 ++++++++++++++++++++++++
>  include/libcamera/meson.build  |  1 +
>  2 files changed, 25 insertions(+)
>  create mode 100644 include/libcamera/deprecated.h
> 
> diff --git a/include/libcamera/deprecated.h b/include/libcamera/deprecated.h
> new file mode 100644
> index 000000000000..ba48d5a142dc
> --- /dev/null
> +++ b/include/libcamera/deprecated.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Ideas on Board Oy.
> + *
> + * deprecated.h - Deprecated API reporting
> + *
> + * Deprecated features of the API will be exposed here for at least one release
> + * iteration to ease reporting of API adjustments for applications.
> + */
> +
> +#pragma once
> +
> +namespace libcamera {
> +
> +/*
> + * Deprectated following v0.0.5

s/Deprectated/Deprecated/

> + *
> + * The use of StreamRoles indicates applications to use dynamic allocations
> + * of the StreamRole when this is not always required.
> + */
> +using StreamRoles [[deprecated("Use a span, array or vector directly")]]
> +	= std::vector<StreamRole>;
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 408b7acf152c..2f470d2ac61e 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -7,6 +7,7 @@ libcamera_public_headers = files([
>      'camera_manager.h',
>      'color_space.h',
>      'controls.h',
> +    'deprecated.h',
>      'fence.h',
>      'framebuffer.h',
>      'framebuffer_allocator.h',

-- 
Regards,

Laurent Pinchart