[Buildroot] [PATCH] package/libcamera: add explicit dependency on libevent if libevent package to be built

[James Hilliard]

On Mon, Jul 11, 2022 at 7:11 AM Quentin Schulz
<quentin.schulz at theobroma-systems.com> wrote:
>
> Hi James,
>
> On 7/11/22 15:05, James Hilliard wrote:
> > On Mon, Jul 11, 2022 at 7:01 AM Quentin Schulz <foss+buildroot at 0leil.net> wrote:
> >>
> >> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
> >>
> >> The cam application requires libevent. Since there's no Kconfig option
> >> for it, cam building ability is checked by meson build system by default.
> >>
> >> If libevent is present in the sysroot, cam is built.
> >>
> >> The issue is that there's no explicit dependency on libevent in
> >> libcamera package. This means that it is possible for libevent AND
> >> libcamera to be built, but have libcamera be built before libevent.
> >> Meaning that even if all requirements seem to be fulfilled, cam still
> >> won't be enabled in some cases.
> >>
> >> This fixes the possible race by expliciting the dependency to libevent
> >> if the libevent package is enabled. Otherwise, explicitly disable cam
> >> building as it's already known that it isn't going to build.
> >>
> >> Cc: Quentin Schulz <foss+buildroot at 0leil.net>
> >> Signed-off-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>
> >> ---
> >>   package/libcamera/libcamera.mk | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/package/libcamera/libcamera.mk b/package/libcamera/libcamera.mk
> >> index 41d6a5abef..78243add18 100644
> >> --- a/package/libcamera/libcamera.mk
> >> +++ b/package/libcamera/libcamera.mk
> >> @@ -84,6 +84,12 @@ else
> >>   LIBCAMERA_CONF_OPTS += -Dqcam=disabled
> >>   endif
> >>
> >> +ifeq ($(BR2_PACKAGE_LIBEVENT),y)
> >> +LIBCAMERA_DEPENDENCIES += libevent
> >
> > You should also explicitly add this as autodetection logic may
> > otherwise mask errors:
> > LIBCAMERA_CONF_OPTS += -Dcam=enabled
> >
>
> Where do we draw the line between implicit enabling of build options and
> adding a proper Kconfig option? Because if the list of dependencies for
> cam increases, it'll get less and less obvious to the user how to enable
> cam support without reading the makefile of the package.

It's somewhat subjective, but generally if a feature is useful based on the
presence of a dependency auto-enabled via makefile is generally ok, provided
that it doesn't trigger a build time behavior change that can't be modified
via other means like overriding a runtime config via overlay.

Enabling/disabling via kconfig is usually needed when there's either a
behavior side effect for enabling an option or if one is using selection logic
from another kconfig option to enable something, but then things can
get more complex as one must ensure indirect dependencies are also
correctly propagated.

In general if one has a valid use case for enabling/disabling an option in
the build separately from the presence of a dependency then it should
probably have a kconfig option.

>
> But adding this line to the makefile makes the maintainer's life easier
> as it'll fail to build next time there's an upgrade and cam has added
> dependencies. So that's already a win :)
>
> Thanks,
> Quentin