[libcamera-devel] [RFC PATCH] ipa: meson: Install mojom generated headers to include paths

[Laurent Pinchart]

Hi Umang,

On Thu, Apr 29, 2021 at 12:27:40PM +0530, Umang Jain wrote:
> On 4/28/21 7:49 PM, Kieran Bingham wrote:
> > On 28/04/2021 14:33, Umang Jain wrote:
> >> Generated IPA headers from mojom files need to be installed sy
> >> $INCLUDE_PATH in order to be available system-wide. Without this,
> >> out-of-tree IPAs won't be able to link and build themselves.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> Marked as RFC to discuss:
> >> - Whether it makes sense to provide -ipa_serializer.h
> >>    and -ipa_proxy.h system-wide too? I am not 100%s sure yet.
> >
> > I don't think it makes sense to install the serializer and proxy
> > headers. Those are used as part of the IPC internals I believe.
> >
> >> ---
> >>   include/libcamera/ipa/meson.build | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> >> index 2d72c1fc..096bd4dd 100644
> >> --- a/include/libcamera/ipa/meson.build
> >> +++ b/include/libcamera/ipa/meson.build
> >> @@ -11,6 +11,8 @@ install_headers(libcamera_ipa_headers,
> >>   
> >>   libcamera_generated_ipa_headers = []
> >>   
> >> +generated_ipa_headers_include_path = join_paths(get_option('prefix'),'include',
> >> +                                                libcamera_include_dir, 'ipa')
> >
> > Where possible, this should be factored out so the common parts are
> > shared with the installation of the libcamera_ipa_headers.
> >
> > However that uses install_headers() which we can't use directly on the
> > custom_target it seems.
> >
> > I would create at the top of the file
> >
> > libcamera_ipa_include_dir = libcamera_include_dir / 'ipa'
> >
> > and use that for the install_headers.
> >
> > Somewhat frustratingly we likely then have to use the following for the
> > install_dir, as 'install_headers' would otherwise have prefixed 'include':
> >
> > 	install : true,
> > 	install_dir : 'include' / libcamera_ipa_include_dir
>
> We need to add prefix too, no? I am sending a new RFC soon, please take 
> a look if that looks on the right path

In most cases, the paths we use in meson files start with a call to
get_option() (with 'prefix', 'datadir', ...). There are a few
exceptions, for generated include files for instance. It would be nice
to fix this, while at it, and use a consistent scheme.
get_option('includedir') seems better than a raw 'include'.

> >>   #
> >>   # Prepare IPA/IPC generation components
> >>   #
> >> @@ -31,6 +33,8 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_interface_h',
> >>                     input : ipa_mojom_core,
> >>                     output : 'core_ipa_interface.h',
> >>                     depends : mojom_templates,
> >> +                  install: true,
> >> +                  install_dir: generated_ipa_headers_include_path,
> >>                     command : [
> >>                         mojom_generator, 'generate',
> >>                         '-g', 'libcamera',
> >> @@ -45,6 +49,8 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',
> >>                     input : ipa_mojom_core,
> >>                     output : 'core_ipa_serializer.h',
> >>                     depends : mojom_templates,
> >> +                  install: true,
> >> +                  install_dir: generated_ipa_headers_include_path,
> >>                     command : [
> >>                         mojom_generator, 'generate',
> >>                         '-g', 'libcamera',
> >> @@ -93,6 +99,8 @@ foreach file : ipa_mojom_files
> >>                              input : mojom,
> >>                              output : name + '_ipa_interface.h',
> >>                              depends : mojom_templates,
> >> +                           install: true,
> >> +                           install_dir: generated_ipa_headers_include_path,
> >
> > Make sure you have a space around both sides of the ':' to match the
> > existing styles.
> >
> > Presumably, this is the only header that needs to actually be installed now?
> > But it will need checking.
> >
> >>                              command : [
> >>                                  mojom_generator, 'generate',
> >>                                  '-g', 'libcamera',
> >> @@ -107,6 +115,8 @@ foreach file : ipa_mojom_files
> >>                                  input : mojom,
> >>                                  output : name + '_ipa_serializer.h',
> >>                                  depends : mojom_templates,
> >> +                               install: true,
> >> +                               install_dir: generated_ipa_headers_include_path,
> >>                                  command : [
> >>                                      mojom_generator, 'generate',
> >>                                      '-g', 'libcamera',
> >> @@ -121,6 +131,8 @@ foreach file : ipa_mojom_files
> >>                                    input : mojom,
> >>                                    output : name + '_ipa_proxy.h',
> >>                                    depends : mojom_templates,
> >> +                                 install: true,
> >> +                                 install_dir: generated_ipa_headers_include_path,
> >>                                    command : [
> >>                                        mojom_generator, 'generate',
> >>                                        '-g', 'libcamera',

-- 
Regards,

Laurent Pinchart