[libcamera-devel] [PATCH] test: v4l2_videodevice: meson: Fix race with generated IPA headers

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-05-28 15:32:23 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wed, May 26, 2021 at 10:23:52AM +0200, Niklas Söderlund wrote:
> > The header v4l2_videodevice_test.h includes
> > libcamera/internal/camera_sensor.h which in turn depends on the auto
> > generated IPA header core_ipa_interface.h. This dependency is not
> > described in meson and building in an empty build directory can trigger
> > a build race.
> > 
> >   [52/349] Compiling C++ object test/v4l2_videodevice/v4l2_m2mdevice.p/v4l2_videodevice_test.cpp.o
> >   FAILED: test/v4l2_videodevice/v4l2_m2mdevice.p/v4l2_videodevice_test.cpp.o
> >   ccache c++ -Itest/v4l2_videodevice/v4l2_m2mdevice.p -Itest/v4l2_videodevice -I../../test/v4l2_videodevice -Itest/libtest -I../../test/libtest -Iinclude -I../../include -Iinclude/libcamera -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wshadow -include config.h -MD -MQ test/v4l2_videodevice/v4l2_m2mdevice.p/v4l2_videodevice_test.cpp.o -MF test/v4l2_videodevice/v4l2_m2mdevice.p/v4l2_videodevice_test.cpp.o.d -o test/v4l2_videodevice/v4l2_m2mdevice.p/v4l2_videodevice_test.cpp.o -c ../../test/v4l2_videodevice/v4l2_videodevice_test.cpp
> >   In file included from ../../test/v4l2_videodevice/v4l2_videodevice_test.h:14,
> >                    from ../../test/v4l2_videodevice/v4l2_videodevice_test.cpp:15:
> >   ../../include/libcamera/internal/camera_sensor.h:17:10: fatal error: libcamera/ipa/core_ipa_interface.h: No such file or directory
> >      17 | #include <libcamera/ipa/core_ipa_interface.h>
> >         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   compilation terminated.
> > 
> 
> If the generated IPA headers become a dependency for anything that uses
> the internal headers, we'll play a game of whack-a-mole trying to
> address the issue in executable individually. There's another similar
> race condition with test/camera-sensor.cpp as it includes
> camera_sensor.h, and other tests also include it indirectly through
> ipa_data_serializer.h
> 
> One option would be to add libcamera_generated_ipa_headers to the
> sources of libcamera_dep. This isn't ideal, as conceptually speaking,
> libcamera_dep is supposed to model the dependencies required for usage
> of the public API, not the internal API. Another option would be to
> include the generated headers in libcamera_ipa_headers, but that's also
> not ideal. libcamera_generated_ipa_headers groups the IPA interface,
> serializer and proxy headers, and that's too much for
> libcamera_ipa_headers (which is a bit ill-defined itself).
> 
> As we'll have to reorganize this when splitting some utilities from the
> libcamera core library into a separate library, I'd go for the simple
> solution of adding libcamera_generated_ipa_headers to libcamera_dep,
> with a todo comment. What do you think ?

I agree this is a whack-a-mole problem with no (easy) ideal solution. As 
you point out adding libcamera_generated_ipa_headers to libcamera_dep 
solves the immediate problem and we can solve it as we move forward.

> 
> > Fix this by describing the dependency in meson the same way it is done
> > for the IPA test cases.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  test/v4l2_videodevice/meson.build | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> > index e733518c0185fda5..ad43748f560908ce 100644
> > --- a/test/v4l2_videodevice/meson.build
> > +++ b/test/v4l2_videodevice/meson.build
> > @@ -15,7 +15,7 @@ v4l2_videodevice_tests = [
> >  ]
> >  
> >  foreach t : v4l2_videodevice_tests
> > -    exe = executable(t[0], [t[1], 'v4l2_videodevice_test.cpp'],
> > +    exe = executable(t[0], [t[1], 'v4l2_videodevice_test.cpp', libcamera_generated_ipa_headers],
> >                       dependencies : libcamera_dep,
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_internal)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund