[libcamera-devel] meson: Fix usage of overwritten pipeline variable

18609 diff mbox series

When iterating over enabled pipelines and IPA modules, libcamera
descends into subdirectories in a recursive manner, which involves
nested loops. Both the outer and inner loops use the same loop variable
named 'pipeline'. As the outer loop uses the variable after descending
into the inner directory, it ends up using an incorrect value. Fix it by
moving all use of the variable before the subdir() call, and add a
comment that warns about the issue to avoid reintroducing it.

Fixes: e8526c0c2bc6 ("ipa: meson: Allow nested IPA directory structures")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/meson.build                | 5 ++++-
 src/libcamera/pipeline/meson.build | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)


base-commit: 1c512d406536d72a393c38c3f6a75fe0fdb9ecb2

Hi Laurent

On Mon, May 08, 2023 at 04:27:09PM +0300, Laurent Pinchart via libcamera-devel wrote:
> When iterating over enabled pipelines and IPA modules, libcamera
> descends into subdirectories in a recursive manner, which involves
> nested loops. Both the outer and inner loops use the same loop variable
> named 'pipeline'. As the outer loop uses the variable after descending
> into the inner directory, it ends up using an incorrect value. Fix it by
> moving all use of the variable before the subdir() call, and add a
> comment that warns about the issue to avoid reintroducing it.
>
> Fixes: e8526c0c2bc6 ("ipa: meson: Allow nested IPA directory structures")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Oh indeed

After running subdir(pipeline), pipelines is now ['rpi', 'vc4']

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  src/ipa/meson.build                | 5 ++++-
>  src/libcamera/pipeline/meson.build | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index 289f861c082d..903eb52ba60f 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -52,8 +52,11 @@ foreach pipeline : pipelines
>          continue
>      endif
>
> -    subdir(pipeline)
>      subdirs += pipeline
> +    subdir(pipeline)
> +
> +    # Don't reuse the pipeline variable below, the subdirectory may have
> +    # overwritten it.
>  endforeach
>
>  # The ipa-sign-install.sh script which uses the enabled_ipa_modules variable
> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> index b6160d346cf7..8a61991c0fec 100644
> --- a/src/libcamera/pipeline/meson.build
> +++ b/src/libcamera/pipeline/meson.build
> @@ -12,6 +12,9 @@ foreach pipeline : pipelines
>          continue
>      endif
>
> -    subdir(pipeline)
>      subdirs += pipeline
> +    subdir(pipeline)
> +
> +    # Don't reuse the pipeline variable below, the subdirectory may have
> +    # overwritten it.
>  endforeach
>
> base-commit: 1c512d406536d72a393c38c3f6a75fe0fdb9ecb2
> --
> Regards,
>
> Laurent Pinchart
>

Hi

On 5/8/23 7:46 PM, Jacopo Mondi via libcamera-devel wrote:
> Hi Laurent
>
> On Mon, May 08, 2023 at 04:27:09PM +0300, Laurent Pinchart via libcamera-devel wrote:
>> When iterating over enabled pipelines and IPA modules, libcamera
>> descends into subdirectories in a recursive manner, which involves
>> nested loops. Both the outer and inner loops use the same loop variable
>> named 'pipeline'. As the outer loop uses the variable after descending
>> into the inner directory, it ends up using an incorrect value. Fix it by
>> moving all use of the variable before the subdir() call, and add a
>> comment that warns about the issue to avoid reintroducing it.
>>
>> Fixes: e8526c0c2bc6 ("ipa: meson: Allow nested IPA directory structures")
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Oh indeed
>
> After running subdir(pipeline), pipelines is now ['rpi', 'vc4']
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>> ---
>>   src/ipa/meson.build                | 5 ++++-
>>   src/libcamera/pipeline/meson.build | 5 ++++-
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
>> index 289f861c082d..903eb52ba60f 100644
>> --- a/src/ipa/meson.build
>> +++ b/src/ipa/meson.build
>> @@ -52,8 +52,11 @@ foreach pipeline : pipelines
>>           continue
>>       endif
>>
>> -    subdir(pipeline)
>>       subdirs += pipeline
>> +    subdir(pipeline)
>> +
>> +    # Don't reuse the pipeline variable below, the subdirectory may have
>> +    # overwritten it.
>>   endforeach
>>
>>   # The ipa-sign-install.sh script which uses the enabled_ipa_modules variable
>> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
>> index b6160d346cf7..8a61991c0fec 100644
>> --- a/src/libcamera/pipeline/meson.build
>> +++ b/src/libcamera/pipeline/meson.build
>> @@ -12,6 +12,9 @@ foreach pipeline : pipelines
>>           continue
>>       endif
>>
>> -    subdir(pipeline)
>>       subdirs += pipeline
>> +    subdir(pipeline)
>> +
>> +    # Don't reuse the pipeline variable below, the subdirectory may have
>> +    # overwritten it.
>>   endforeach
>>
>> base-commit: 1c512d406536d72a393c38c3f6a75fe0fdb9ecb2
>> --
>> Regards,
>>
>> Laurent Pinchart
>>