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

[Umang Jain]

Hi Kieran

On 4/29/21 3:35 PM, Kieran Bingham wrote:
> 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?
Verified that prefix not required, meson takes care of installing it at 
the right place. I wasn't aware of it.
>
> --
> 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',
>>>>>