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

[Laurent Pinchart]

Hi Kieran,

On Wed, May 22, 2019 at 08:39:44AM +0100, Kieran Bingham wrote:
> 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>
> 
> > ---
> >  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

I considered both options and found it more logical to keep the file
there. It doesn't get installed, so it shouldn't be an issue. If we
later extend the script to generate more headers, or more files, than
libcamera.h, then we could move it.

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

Good point. I however wonder how to treat copyright on auto-generated
files. The generation process doesn't create any additional work that
can be attributed to an entity entitled to copyrights as far as I
understand, so the copyright on libcamera.h would come from the
copyright on its source. In this case the source is the combination of
the directory layout (hardly copyrightable I believe) and this script.
It thus seems that the copyright year should be fixed in the script (and
should be 2018-2019), and should only change when the script is
significantly modified. I'm no expert in this field though.

> > + *
> > + * 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)

Yes, let's handle this later :-)

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

I'll fix these, thank you.

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

My bad :-( Will fix too.

> > +
> >  install_headers(libcamera_api,
> >                  subdir : 'libcamera')
> > 

-- 
Regards,

Laurent Pinchart