[libcamera-devel] [PATCH] libcamera: Auto-generate libcamera.h

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2019-05-22 08:39:44 +0100, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 21/05/2019 17:45, Laurent Pinchart wrote:
> > As shown by two missing includes, keeping the libcamera.h file in sync
> > when adding or removing headers is an error-prone manual process.
> > Automate it by generating the header automatically.
> 
> Great idea!
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> With the shellcheck issues considered, and the tabs fixed up in meson.build:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

With Kieran's comments addressed,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> 
> 
> > ---
> >  include/libcamera/gen-header.sh | 26 ++++++++++++++++++++++++++
> >  include/libcamera/libcamera.h   | 20 --------------------
> >  include/libcamera/meson.build   | 10 +++++++++-
> >  3 files changed, 35 insertions(+), 21 deletions(-)
> >  create mode 100755 include/libcamera/gen-header.sh
> >  delete mode 100644 include/libcamera/libcamera.h
> > 
> > diff --git a/include/libcamera/gen-header.sh b/include/libcamera/gen-header.sh
> > new file mode 100755
> > index 000000000000..ea5b79a62520
> > --- /dev/null
> > +++ b/include/libcamera/gen-header.sh
> 
> Hrm... shouldn't this live in a utils directory?
> 
> Although there are some strong arguments for keeping it in
> include/libcamera, such as:
> 
>  - It keeps the directory layout correct in git
>  - The helper is in the place that the header will be output so it's
>    easy to find
> 
> > @@ -0,0 +1,26 @@
> > +#!/bin/sh
> > +
> > +src_dir=$1
> > +dst_file=$2
> > +
> > +cat <<EOF > $dst_file
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/* This file is auto-generated, do not edit! */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> 
> Why not add `date +'%Y'` as it's autogenerated ? :-D
> 
> > + *
> > + * libcamera.h - libcamera public API
> > + */
> > +#ifndef __LIBCAMERA_LIBCAMERA_H__
> > +#define __LIBCAMERA_LIBCAMERA_H__
> > +
> > +EOF
> > +
> > +for header in $src_dir/*.h ; do
> > +	echo "#include <libcamera/$(basename $header)>" >> $dst_file
> > +done
> 
> 
> Are we ever expecting to have subdirs here?
> I don't think so at the moment, and if we do - then we can update this
> at that point
> 
> (I'm sure someone will notice if they can't include their headers :D)
> 
> > +
> > +cat <<EOF >> $dst_file
> > +
> > +#endif /* __LIBCAMERA_LIBCAMERA_H__ */
> > +EOF
> 
> 
> Here's the output of running shellcheck on this script:
> 
> > shellcheck ./include/libcamera/gen-header.sh 
> > 
> > In ./include/libcamera/gen-header.sh line 6:
> > cat <<EOF > $dst_file
> >             ^-- SC2086: Double quote to prevent globbing and word splitting.
> > 
> > 
> > In ./include/libcamera/gen-header.sh line 19:
> > for header in $src_dir/*.h ; do
> >               ^-- SC2231: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt .
> > 
> > 
> > In ./include/libcamera/gen-header.sh line 20:
> >         echo "#include <libcamera/$(basename $header)>" >> $dst_file
> >                                              ^-- SC2086: Double quote to prevent globbing and word splitting.
> >                                                            ^-- SC2086: Double quote to prevent globbing and word splitting.
> > 
> > 
> > In ./include/libcamera/gen-header.sh line 23:
> > cat <<EOF >> $dst_file
> >              ^-- SC2086: Double quote to prevent globbing and word splitting.
> 
> 
> 
> 
> 
> > diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> > deleted file mode 100644
> > index dda576e906fb..000000000000
> > --- a/include/libcamera/libcamera.h
> > +++ /dev/null
> > @@ -1,20 +0,0 @@
> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > -/*
> > - * Copyright (C) 2018, Google Inc.
> > - *
> > - * libcamera.h - libcamera public API
> > - */
> > -#ifndef __LIBCAMERA_LIBCAMERA_H__
> > -#define __LIBCAMERA_LIBCAMERA_H__
> > -
> > -#include <libcamera/buffer.h>
> > -#include <libcamera/camera.h>
> > -#include <libcamera/camera_manager.h>
> > -#include <libcamera/event_dispatcher.h>
> > -#include <libcamera/event_notifier.h>
> > -#include <libcamera/request.h>
> > -#include <libcamera/signal.h>
> > -#include <libcamera/stream.h>
> > -#include <libcamera/timer.h>
> > -
> > -#endif /* __LIBCAMERA_LIBCAMERA_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 83d226ac5078..46aff0b6bc2f 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -5,7 +5,6 @@ libcamera_api = files([
> >      'event_dispatcher.h',
> >      'event_notifier.h',
> >      'geometry.h',
> > -    'libcamera.h',
> >      'object.h',
> >      'request.h',
> >      'signal.h',
> > @@ -13,5 +12,14 @@ libcamera_api = files([
> >      'timer.h',
> >  ])
> >  
> > +gen_header = join_paths(meson.current_source_dir(), 'gen-header.sh')
> > +
> > +custom_target('gen-header',
> > +	      input: meson.current_source_dir(),
> > +              output: 'libcamera.h',
> > +	      command: [gen_header, '@INPUT@', '@OUTPUT@'],
> > +	      install: true,
> > +	      install_dir: 'include/libcamera')
> 
> I think some tabs have snuck in there.
> 
> 
> 
> 
> > +
> >  install_headers(libcamera_api,
> >                  subdir : 'libcamera')
> > 
> 
> -- 
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund