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

[Kieran Bingham]

Hi Umang,

On 29/04/2021 10:32, Kieran Bingham wrote:
> Hi Umang,
> 
> On 29/04/2021 07:57, Umang Jain wrote:
>> Hi Kieran,
>>
>> On 4/28/21 7:49 PM, Kieran Bingham wrote:
>>> Hi Umang,
>>>
>>> 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
> 
> Aha possibly indeed.
> 
> Time for me to add a new test to my packaging tests, and make sure I set
> an obscure prefix...

I've just been playing around with this, and I'm not sure the prefix is
needed.

I'm not yet sure, or why, but perhaps the meson install helpers already
deal with prefix.

I'll submit a patch to add path configurations to the configuration
summary to help identify these paths when building.

Meanwhile, could you test on your patch if prefix is really required or not?

--
Kieran

> 
>>>
>>>
>>>>   #
>>>>   # 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.
>>>
>>> -- 
>>> Kieran
>>>
>>>
>>>>                              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
--
Kieran