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

[Kieran Bingham]

Hi Umang,

On 30/04/2021 10:52, Umang Jain wrote:
> 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.

Great, I believe that's handled by the 'install' handlers. If we were
doing any manual copying or installation outside of meson, we would have
to handle that though.

I think obtaining 'include' from the specific variable as suggested by
Laurent, or defining it as a system path (on top of the patch I'm about
to merge) makes sense though.



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