[libcamera-devel,7/8] utils: ipc: generate.py: Disable attributes checker

19368 diff mbox series

The attributes checker ensures that .mojom files don't contain unknown
attributes. These check fail with the custom 'skipSerdes' and 'async'
libcamera attributes. Ideally the list of supported attributes should be
extended, but that can't easily be done without modifying the mojo
sources that we try to keep identical to the upstream version to make
updates easier. Disable the attributes checker completely for now to fix
this issue.

While at it, fix an indentation issue reported by checkstyle.py.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/ipc/generate.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> The attributes checker ensures that .mojom files don't contain unknown
> attributes. These check fail with the custom 'skipSerdes' and 'async'
> libcamera attributes. Ideally the list of supported attributes should be
> extended, but that can't easily be done without modifying the mojo
> sources 

Maybe it would be possible by overriding generator.LoadChecks in generate.py --
inspecting its return value for mojom_attributes_check module presence and
modifying mojom_attributes_check variables?  I'm not sure it qualifies as easy
while it's definitely hacky; but this change is hacky anyway.  It depends
whether we care more about the attribute checks or keeping the hack simple.
Looking at .mojom files, it seems to me the attribute checks are not that
important.

That said, I'm fine with this patch in any case.

> that we try to keep identical to the upstream version to make updates
> easier. Disable the attributes checker completely for now to fix this issue.
>
> While at it, fix an indentation issue reported by checkstyle.py.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  utils/ipc/generate.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/utils/ipc/generate.py b/utils/ipc/generate.py
> index afaf018b49d1..71bdee3b7545 100755
> --- a/utils/ipc/generate.py
> +++ b/utils/ipc/generate.py
> @@ -17,7 +17,15 @@ sys.path.insert(0, f'{os.path.dirname(__file__)}/mojo/public/tools/bindings')
>  import mojo.public.tools.bindings.mojom_bindings_generator as generator
>  
>  def _GetModulePath(path, output_dir):
> -  return os.path.join(output_dir, path.relative_path())
> +    return os.path.join(output_dir, path.relative_path())
> +
> +
> +# Disable the attribute checker to support our custom attributes. Ideally we
> +# should add the attributes to the list of allowed attributes in
> +# utils/ipc/mojo/public/tools/bindings/checks/mojom_attributes_check.py, but
> +# we're trying hard to use the upstream mojom as-is.
> +if hasattr(generator, '_BUILTIN_CHECKS'):
> +    del generator._BUILTIN_CHECKS['attributes']
>  
>  # Override the mojo code generator's generator list to only contain our
>  # libcamera generator

Quoting Laurent Pinchart via libcamera-devel (2024-01-04 15:15:47)
> The attributes checker ensures that .mojom files don't contain unknown
> attributes. These check fail with the custom 'skipSerdes' and 'async'
> libcamera attributes. Ideally the list of supported attributes should be
> extended, but that can't easily be done without modifying the mojo
> sources that we try to keep identical to the upstream version to make
> updates easier. Disable the attributes checker completely for now to fix
> this issue.
> 
> While at it, fix an indentation issue reported by checkstyle.py.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/ipc/generate.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/ipc/generate.py b/utils/ipc/generate.py
> index afaf018b49d1..71bdee3b7545 100755
> --- a/utils/ipc/generate.py
> +++ b/utils/ipc/generate.py
> @@ -17,7 +17,15 @@ sys.path.insert(0, f'{os.path.dirname(__file__)}/mojo/public/tools/bindings')
>  import mojo.public.tools.bindings.mojom_bindings_generator as generator
>  
>  def _GetModulePath(path, output_dir):
> -  return os.path.join(output_dir, path.relative_path())
> +    return os.path.join(output_dir, path.relative_path())
> +
> +

Double line space here vs single after. I think that's ok, as this is
double after a def scope block. As long as checkstyle doesn't shout (and
I'm sure you'd spot if it did) then I'm fine.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +# Disable the attribute checker to support our custom attributes. Ideally we
> +# should add the attributes to the list of allowed attributes in
> +# utils/ipc/mojo/public/tools/bindings/checks/mojom_attributes_check.py, but
> +# we're trying hard to use the upstream mojom as-is.
> +if hasattr(generator, '_BUILTIN_CHECKS'):
> +    del generator._BUILTIN_CHECKS['attributes']
>  
>  # Override the mojo code generator's generator list to only contain our
>  # libcamera generator
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Tue, Jan 09, 2024 at 12:13:48PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2024-01-04 15:15:47)
> > The attributes checker ensures that .mojom files don't contain unknown
> > attributes. These check fail with the custom 'skipSerdes' and 'async'
> > libcamera attributes. Ideally the list of supported attributes should be
> > extended, but that can't easily be done without modifying the mojo
> > sources that we try to keep identical to the upstream version to make
> > updates easier. Disable the attributes checker completely for now to fix
> > this issue.
> > 
> > While at it, fix an indentation issue reported by checkstyle.py.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  utils/ipc/generate.py | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/utils/ipc/generate.py b/utils/ipc/generate.py
> > index afaf018b49d1..71bdee3b7545 100755
> > --- a/utils/ipc/generate.py
> > +++ b/utils/ipc/generate.py
> > @@ -17,7 +17,15 @@ sys.path.insert(0, f'{os.path.dirname(__file__)}/mojo/public/tools/bindings')
> >  import mojo.public.tools.bindings.mojom_bindings_generator as generator
> >  
> >  def _GetModulePath(path, output_dir):
> > -  return os.path.join(output_dir, path.relative_path())
> > +    return os.path.join(output_dir, path.relative_path())
> > +
> > +
> 
> Double line space here vs single after. I think that's ok, as this is
> double after a def scope block. As long as checkstyle doesn't shout (and
> I'm sure you'd spot if it did) then I'm fine.

I've added the second blank line because checkstyle complained :-)

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +# Disable the attribute checker to support our custom attributes. Ideally we
> > +# should add the attributes to the list of allowed attributes in
> > +# utils/ipc/mojo/public/tools/bindings/checks/mojom_attributes_check.py, but
> > +# we're trying hard to use the upstream mojom as-is.
> > +if hasattr(generator, '_BUILTIN_CHECKS'):
> > +    del generator._BUILTIN_CHECKS['attributes']
> >  
> >  # Override the mojo code generator's generator list to only contain our
> >  # libcamera generator