[v5,12/18] libcamera: Consolidate tracepoint header in libcamera_internal_headers

20798 diff mbox series

The libcamera_tracepoint_header variable stores the tracepoints.h header
custom target, for the sole purpose of being listed as a source of the
libcamera shared library, through the libcamera_internal_sources
variable.

Add the tracepoints.h header to libcamera_internal_headers instead of
libcamera_internal_sources, and list libcamera_internal_headers as a
source of the shared library, alongside libcamera_internal_sources. This
makes libcamera_internal_sources contain sources only, improving clarity
of the build system variables.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/meson.build | 16 +++++++++-------
 src/libcamera/meson.build              |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

Hi Laurent

On 05/08/2024 15:36, Laurent Pinchart wrote:
> The libcamera_tracepoint_header variable stores the tracepoints.h header
> custom target, for the sole purpose of being listed as a source of the
> libcamera shared library, through the libcamera_internal_sources
> variable.
>
> Add the tracepoints.h header to libcamera_internal_headers instead of
> libcamera_internal_sources, and list libcamera_internal_headers as a
> source of the shared library, alongside libcamera_internal_sources. This
> makes libcamera_internal_sources contain sources only, improving clarity
> of the build system variables.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
Makes sense to me: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>   include/libcamera/internal/meson.build | 16 +++++++++-------
>   src/libcamera/meson.build              |  2 +-
>   2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index f96cc5e37c23..39230facc8a4 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -2,13 +2,6 @@
>   
>   subdir('tracepoints')
>   
> -libcamera_tracepoint_header = custom_target(
> -    'tp_header',
> -    input : ['tracepoints.h.in', tracepoint_files],
> -    output : 'tracepoints.h',
> -    command : [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> -)
> -
>   libcamera_internal_headers = files([
>       'bayer_format.h',
>       'byte_stream_buffer.h',
> @@ -51,5 +44,14 @@ libcamera_internal_headers = files([
>       'yaml_parser.h',
>   ])
>   
> +tracepoints_h = custom_target(
> +    'tp_header',
> +    input : ['tracepoints.h.in', tracepoint_files],
> +    output : 'tracepoints.h',
> +    command : [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> +)
> +
> +libcamera_internal_headers += tracepoints_h
> +
>   subdir('converter')
>   subdir('software_isp')
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 86e8b88cc1b2..6a7c9d77dfd8 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -55,7 +55,6 @@ libcamera_internal_sources = files([
>   ])
>   
>   libcamera_public_sources += libcamera_public_headers
> -libcamera_internal_sources += libcamera_tracepoint_header
>   
>   includes = [
>       libcamera_includes,
> @@ -196,6 +195,7 @@ libcamera = shared_library('libcamera',
>                              [
>                                  libcamera_public_sources,
>                                  libcamera_ipa_headers,
> +                               libcamera_internal_headers,
>                                  libcamera_internal_sources,
>                              ],
>                              version : libcamera_version,

Quoting Laurent Pinchart (2024-08-05 15:36:48)
> The libcamera_tracepoint_header variable stores the tracepoints.h header
> custom target, for the sole purpose of being listed as a source of the
> libcamera shared library, through the libcamera_internal_sources
> variable.
> 
> Add the tracepoints.h header to libcamera_internal_headers instead of
> libcamera_internal_sources, and list libcamera_internal_headers as a
> source of the shared library, alongside libcamera_internal_sources. This
> makes libcamera_internal_sources contain sources only, improving clarity
> of the build system variables.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/meson.build | 16 +++++++++-------
>  src/libcamera/meson.build              |  2 +-
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index f96cc5e37c23..39230facc8a4 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -2,13 +2,6 @@
>  
>  subdir('tracepoints')
>  
> -libcamera_tracepoint_header = custom_target(
> -    'tp_header',
> -    input : ['tracepoints.h.in', tracepoint_files],
> -    output : 'tracepoints.h',
> -    command : [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> -)
> -
>  libcamera_internal_headers = files([
>      'bayer_format.h',
>      'byte_stream_buffer.h',
> @@ -51,5 +44,14 @@ libcamera_internal_headers = files([
>      'yaml_parser.h',
>  ])
>  
> +tracepoints_h = custom_target(
> +    'tp_header',
> +    input : ['tracepoints.h.in', tracepoint_files],
> +    output : 'tracepoints.h',
> +    command : [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> +)
> +
> +libcamera_internal_headers += tracepoints_h
> +
>  subdir('converter')
>  subdir('software_isp')
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 86e8b88cc1b2..6a7c9d77dfd8 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -55,7 +55,6 @@ libcamera_internal_sources = files([
>  ])
>  
>  libcamera_public_sources += libcamera_public_headers
> -libcamera_internal_sources += libcamera_tracepoint_header
>  
>  includes = [
>      libcamera_includes,
> @@ -196,6 +195,7 @@ libcamera = shared_library('libcamera',
>                             [
>                                 libcamera_public_sources,
>                                 libcamera_ipa_headers,
> +                               libcamera_internal_headers,

I think this brings in a set of the headers as dependencies now (a good
thing). I wonder if this is where we had that 'breakage requiring a full
clean rebuild' around v0.2.0 or v0.1.0 or such.

Anyway, it looks like a good thing to me.


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

>                                 libcamera_internal_sources,
>                             ],
>                             version : libcamera_version,
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Wed, Aug 07, 2024 at 12:19:17PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-08-05 15:36:48)
> > The libcamera_tracepoint_header variable stores the tracepoints.h header
> > custom target, for the sole purpose of being listed as a source of the
> > libcamera shared library, through the libcamera_internal_sources
> > variable.
> > 
> > Add the tracepoints.h header to libcamera_internal_headers instead of
> > libcamera_internal_sources, and list libcamera_internal_headers as a
> > source of the shared library, alongside libcamera_internal_sources. This
> > makes libcamera_internal_sources contain sources only, improving clarity
> > of the build system variables.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/meson.build | 16 +++++++++-------
> >  src/libcamera/meson.build              |  2 +-
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index f96cc5e37c23..39230facc8a4 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -2,13 +2,6 @@
> >  
> >  subdir('tracepoints')
> >  
> > -libcamera_tracepoint_header = custom_target(
> > -    'tp_header',
> > -    input : ['tracepoints.h.in', tracepoint_files],
> > -    output : 'tracepoints.h',
> > -    command : [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> > -)
> > -
> >  libcamera_internal_headers = files([
> >      'bayer_format.h',
> >      'byte_stream_buffer.h',
> > @@ -51,5 +44,14 @@ libcamera_internal_headers = files([
> >      'yaml_parser.h',
> >  ])
> >  
> > +tracepoints_h = custom_target(
> > +    'tp_header',
> > +    input : ['tracepoints.h.in', tracepoint_files],
> > +    output : 'tracepoints.h',
> > +    command : [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> > +)
> > +
> > +libcamera_internal_headers += tracepoints_h
> > +
> >  subdir('converter')
> >  subdir('software_isp')
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 86e8b88cc1b2..6a7c9d77dfd8 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -55,7 +55,6 @@ libcamera_internal_sources = files([
> >  ])
> >  
> >  libcamera_public_sources += libcamera_public_headers
> > -libcamera_internal_sources += libcamera_tracepoint_header
> >  
> >  includes = [
> >      libcamera_includes,
> > @@ -196,6 +195,7 @@ libcamera = shared_library('libcamera',
> >                             [
> >                                 libcamera_public_sources,
> >                                 libcamera_ipa_headers,
> > +                               libcamera_internal_headers,
> 
> I think this brings in a set of the headers as dependencies now (a good
> thing). I wonder if this is where we had that 'breakage requiring a full
> clean rebuild' around v0.2.0 or v0.1.0 or such.

I don't think it will make a difference in practice. Meson handles
dependencies on static header automatically. If a static header changes,
the source files that include it (directly or indirectly) will be
recompiled, and so libcamera will be relinked. Adding the static headers
to the sources here shouldn't change anything.

The problems we encountered a while ago were due to missing dependencies
on generated files if I recall correctly.

> Anyway, it looks like a good thing to me.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >                                 libcamera_internal_sources,
> >                             ],
> >                             version : libcamera_version,