[libcamera-devel] [PATCH v1] libcamera: tracing: fix header generation when built as subproject

[Laurent Pinchart]

Hi Barnabás,

On Mon, Nov 14, 2022 at 04:12:23PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> On Mon, 14 Nov 2022 16:12:23 +0000, Kieran Bingham wrote:
> > Quoting Paul Elder via libcamera-devel (2022-11-10 03:27:36)
> > > On Wed, Nov 09, 2022 at 04:36:14PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > > > Date: Wed, 09 Nov 2022 16:36:14 +0000
> > > > From: Barnabás Pőcze <pobrn at protonmail.com>
> > > > To: libcamera-devel at lists.libcamera.org
> > > > Subject: [PATCH v1] libcamera: tracing: fix header generation when built as
> > > >  subproject
> > > >
> > > > The previously used
> > > >
> > > >   path = output.replace('include/', '', 1)
> > > >
> > > > logic is not sufficient to correctly determine the proper path
> > > > when libcamera is built as a subproject. Fix it by using Python's
> > > > pathlib to calculate the relative path of the output file with
> > > > respect to the "include" directory of libcamera.
> > > >
> > > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > >
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > This looks ok to me, and doesn't break the builds for my integration
> > tests - but I haven't easily tested as a submodule yet.
> > 
> > However, given that I expect that has been tested by Barnabás,
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> I am using this with pipewire. For non-subproject builds it should generate
> the exact same path that was previously generated.
> 
> I did not elaborate in the commit message but the issue is that when built as a
> subproject, the value of `path` would be
> 
>   'subprojects/libcamera/include/libcamera/internal/tracepoints.h'.replace('include/', '', 1)
> 
> which evaluates to
> 
>   'subprojects/libcamera/libcamera/internal/tracepoints.h'
> 
> so the tracepoints.h header file will try to include:
> 
>   #define TRACEPOINT_INCLUDE "subprojects/libcamera/libcamera/internal/tracepoints.h"
> 
> which will fail.

Thank you for the explanation, I was wondering what was wrong with the
previous logic. Kieran, could you record this in the commit message when
pushing ?

> My understanding of the previous logic was that it tried to set `path` to
> the path of the output file relative to the libcamera include directory.
> 
> The
> 
>   pathlib.Path(output).absolute().relative_to(argv[1])
> 
> expression is supposed to do the same.
> 
> > > > ---
> > > >  include/libcamera/internal/meson.build |  2 +-
> > > >  include/meson.build                    |  2 ++
> > > >  utils/tracepoints/gen-tp-header.py     | 13 +++++++------
> > > >  3 files changed, 10 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > index 7a780d48ee57..f8be86e040c5 100644
> > > > --- a/include/libcamera/internal/meson.build
> > > > +++ b/include/libcamera/internal/meson.build
> > > > @@ -6,7 +6,7 @@ libcamera_tracepoint_header = custom_target(
> > > >      'tp_header',
> > > >      input: ['tracepoints.h.in', tracepoint_files],
> > > >      output: 'tracepoints.h',
> > > > -    command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'],
> > > > +    command: [gen_tracepoints_header, include_build_dir, '@OUTPUT@', '@INPUT@'],
> > > >  )
> > > >
> > > >  libcamera_internal_headers = files([
> > > > diff --git a/include/meson.build b/include/meson.build
> > > > index 27ce2f41c534..19b93a7bd753 100644
> > > > --- a/include/meson.build
> > > > +++ b/include/meson.build
> > > > @@ -1,4 +1,6 @@
> > > >  # SPDX-License-Identifier: CC0-1.0
> > > >
> > > > +include_build_dir = meson.current_build_dir()
> > > > +
> > > >  subdir('android')
> > > >  subdir('libcamera')
> > > > diff --git a/utils/tracepoints/gen-tp-header.py b/utils/tracepoints/gen-tp-header.py
> > > > index bbd472d972d0..a454615e4625 100755
> > > > --- a/utils/tracepoints/gen-tp-header.py
> > > > +++ b/utils/tracepoints/gen-tp-header.py
> > > > @@ -8,22 +8,23 @@
> > > >
> > > >  import datetime
> > > >  import jinja2
> > > > +import pathlib
> > > >  import os
> > > >  import sys
> > > >
> > > >  def main(argv):
> > > > -    if len(argv) < 3:
> > > > -        print(f'Usage: {argv[0]} output template tp_files...')
> > > > +    if len(argv) < 4:
> > > > +        print(f'Usage: {argv[0]} include_build_dir output template tp_files...')
> > > >          return 1
> > > >
> > > > -    output = argv[1]
> > > > -    template = argv[2]
> > > > +    output = argv[2]
> > > > +    template = argv[3]
> > > >
> > > >      year = datetime.datetime.now().year
> > > > -    path = output.replace('include/', '', 1)
> > > > +    path = pathlib.Path(output).absolute().relative_to(argv[1])
> > > >
> > > >      source = ''
> > > > -    for fname in argv[3:]:
> > > > +    for fname in argv[4:]:
> > > >          source += open(fname, 'r', encoding='utf-8').read() + '\n\n'
> > > >
> > > >      template = jinja2.Template(open(template, 'r', encoding='utf-8').read())

-- 
Regards,

Laurent Pinchart