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

[Kieran Bingham]

Quoting James Hilliard (2022-07-11 14:28:59)
> 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.

As 'cam' is a specific test tool, I would of thought perhaps it
warrants a specific option to explicitly enable it or disable it.
Of course enabling it should then declare libevent as a dependency.

If a user wants to test their camera bring up - they likely want to
'enable cam' (or disable it when making a released image).

But they probably don't expect to 'Enable libevent to add in cam'..?

--
Kieran

> 
> >
> > 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