[libcamera-devel,05/23] meson: ipa, proxy: Generate headers and proxy with mojo
diff mbox series

Message ID 20200915142038.28757-6-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • IPA isolation implementation
Related show

Commit Message

Paul Elder Sept. 15, 2020, 2:20 p.m. UTC 
Run mojo from meson to generate the header, serializer, and proxy files
for every pipeline's mojom data definition file. So far we only have the
raspberrypi mojom file.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---

Dictionaries with non-literal keys is apparently a meson 0.53 feature.

 include/libcamera/ipa/meson.build      | 87 ++++++++++++++++++++++++++
 src/libcamera/proxy/meson.build        | 21 +++++++
 src/libcamera/proxy/worker/meson.build | 25 +++++++-
 3 files changed, 131 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Sept. 16, 2020, 1:45 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Tue, Sep 15, 2020 at 11:20:20PM +0900, Paul Elder wrote:
> Run mojo from meson to generate the header, serializer, and proxy files
> for every pipeline's mojom data definition file. So far we only have the
> raspberrypi mojom file.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> 
> Dictionaries with non-literal keys is apparently a meson 0.53 feature.

And the latest stable Debian doesn't ship 0.53 :-S Let's see if we can
do without that feature. How about the following ?

diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
index c80594d7db58..5d174750d6d2 100644
--- a/include/libcamera/ipa/meson.build
+++ b/include/libcamera/ipa/meson.build
@@ -25,11 +25,16 @@ mojom_template = custom_target('mojom_template',
 
 mojom_parser = find_program('../../../utils/ipc/mojo/public/tools/mojom/mojom_parser.py')
 
-# {pipeline}.mojom-module
-ipa_mojoms = {}
+ipa_mojoms = []
+
+#
+# Generate headers from templates.
+#
 
 foreach file : ipa_mojom_files
     name = file.split('.')[0]
+
+    # {pipeline}.mojom-module
     mojom = custom_target(file.split('.')[0] + '_mojom_module',
                           input : file,
                           output : file + '-module',
@@ -38,21 +43,7 @@ foreach file : ipa_mojom_files
                                      '--input-root', meson.source_root(),
                                      '--mojoms', '@INPUT@'
                           ])
-    ipa_mojoms += { name: mojom }
-endforeach
-
-#
-# Generate headers from templates.
-#
-
-# {pipeline}_generated.h
-# generate {pipeline}_serializer.h
-# generate ipa_proxy_{pipeline}.h
-ipa_headers = {}
-ipa_serializers = {}
-ipa_proxy_headers = {}
-
-foreach name, mojom : ipa_mojoms
+    # {pipeline}_generated.h
     header = custom_target(name + '_generated_h',
                            input : mojom,
                            output : name + '_generated.h',
@@ -65,6 +56,7 @@ foreach name, mojom : ipa_mojoms
                                       './' +'@INPUT@'
                            ])
 
+    # {pipeline}_serializer.h
     serializer = custom_target(name + '_serializer_h',
                                input : mojom,
                                output : name + '_serializer.h',
@@ -77,6 +69,7 @@ foreach name, mojom : ipa_mojoms
                                           './' +'@INPUT@'
                                ])
 
+    # ipa_proxy_{pipeline}.h
     proxy_header = custom_target(name + '_proxy_h',
                                  input : mojom,
                                  output : 'ipa_proxy_' + name + '.h',
@@ -89,11 +82,11 @@ foreach name, mojom : ipa_mojoms
                                             './' +'@INPUT@'
                                  ])
 
-    ipa_headers += { name: header }
-    ipa_serializers += { name: serializer }
-    ipa_proxy_headers += { name: proxy_header }
-endforeach
+    ipa_mojoms += {
+        'name': name,
+        'mojom': mojom,
+        'dependencies': [header, serializer, proxy_header],
+    }
 
-libcamera_internal_headers += ipa_proxy_headers
-libcamera_internal_headers += ipa_headers
-libcamera_internal_headers += ipa_serializers
+    libcamera_internal_headers += [header, serializer, proxy_header]
+endforeach
diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
index cfec0567bf5e..df260227a88a 100644
--- a/src/libcamera/proxy/meson.build
+++ b/src/libcamera/proxy/meson.build
@@ -3,14 +3,12 @@
 # generate ipa_proxy_{pipeline}.cpp
 ipa_proxy_sources = []
 
-foreach name, mojom : ipa_mojoms
+foreach mojom : ipa_mojoms
+    name = mojom['name']
     ipa_proxy_sources += custom_target(name + '_proxy_cpp',
-                                       input : mojom,
+                                       input : mojom['mojom'],
                                        output : 'ipa_proxy_' + name + '.cpp',
-                                       depends : [mojom_template,
-                                                  ipa_headers[name],
-                                                  ipa_serializers[name],
-                                                  ipa_proxy_headers[name]],
+                                       depends : [mojom_template, mojom['dependencies']],
                                        command : [mojom_generator, 'generate',
                                                   '-g', 'libcamera',
                                                   '--bytecode_path', '.',
diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build
index db4630656905..2a739af91a31 100644
--- a/src/libcamera/proxy/worker/meson.build
+++ b/src/libcamera/proxy/worker/meson.build
@@ -2,16 +2,14 @@
 
 
 # generate ipa_proxy_{pipeline}_worker.cpp
-ipa_proxy_sources = {}
+ipa_proxy_sources = []
 
-foreach name, mojom : ipa_mojoms
+foreach mojom : ipa_mojoms
+    name = mojom['name']
     worker = custom_target(name + '_proxy_worker',
-                           input : mojom,
+                           input : mojom['mojom'],
                            output : 'ipa_proxy_' + name + '_worker.cpp',
-                           depends : [mojom_template,
-                                      ipa_headers[name],
-                                      ipa_serializers[name],
-                                      ipa_proxy_headers[name]],
+                           depends : [mojom_template, mojom['dependencies']],
                            command : [mojom_generator, 'generate',
                                       '-g', 'libcamera',
                                       '--bytecode_path', '.',
@@ -19,13 +17,16 @@ foreach name, mojom : ipa_mojoms
                                       '--libcamera_output_path=@OUTPUT@',
                                       './' + '@INPUT@'
                            ])
-    ipa_proxy_sources += { 'ipa_proxy_' + name : worker }
+    ipa_proxy_sources += {
+        'name': 'ipa_proxy_' + name,
+        'worker': worker,
+    }
 endforeach
 
 proxy_install_dir = join_paths(get_option('libexecdir'), 'libcamera')
 
-foreach k, t : ipa_proxy_sources
-    proxy = executable(k, t,
+foreach src : ipa_proxy_sources
+    proxy = executable(src['name'], src['worker'],
                        install : true,
                        install_dir : proxy_install_dir,
                        dependencies : libcamera_dep)

By the way, the reason why arrays of arrays failed for you is likely due
to the fact that when meson interprets the following code

foo = []
foo += [1,2,3]
foo += [4,5,6]

foo is equal to [1,2,3,4,5,6], not [[1,2,3],[4,5,6]]

You would need to write

foo = []
foo += [[1,2,3]]
foo += [[4,5,6]]

But dictionaries are nicer anyway, it's best to use a string literal as
a key than hardcoding an array index.

> 
>  include/libcamera/ipa/meson.build      | 87 ++++++++++++++++++++++++++
>  src/libcamera/proxy/meson.build        | 21 +++++++
>  src/libcamera/proxy/worker/meson.build | 25 +++++++-
>  3 files changed, 131 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index 508c6bd1..f61a5a8f 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -8,3 +8,90 @@ libcamera_ipa_headers = files([
>  
>  install_headers(libcamera_ipa_headers,
>                  subdir: join_paths(libcamera_include_dir, 'ipa'))
> +
> +#
> +# Prepare IPA/IPC generation components
> +#
> +
> +ipa_mojom_files = []
> +
> +mojom_generator = find_program('../../../utils/ipc/generate.py')

This should be rebased on top of "[PATCH 0/3] Unify utils locations", or
just "[PATCH 1/3] meson: Process utils first". You can then add a
meson.build to utils/ipc/ and move this line there.

> +
> +mojom_template = custom_target('mojom_template',
> +                               output : 'dummy',
> +                               command : [mojom_generator, 'precompile'])

You will need to add inputs, otherwise mojom_template won't be rebuilt
when the templates change. You don't need to use the inputs in the
command, but they have to be listed. It will require keeping meson.build
in sync with the list of templates, but that should be manageable,
especially if we add a meson.build to
utils/ipc/generators/libcamera_templates.

> +mojom_parser = find_program('../../../utils/ipc/mojo/public/tools/mojom/mojom_parser.py')

All this should also go to utils/ipc/meson.build.

> +
> +# {pipeline}.mojom-module
> +ipa_mojoms = {}
> +
> +foreach file : ipa_mojom_files
> +    name = file.split('.')[0]
> +    mojom = custom_target(file.split('.')[0] + '_mojom_module',
> +                          input : file,
> +                          output : file + '-module',
> +                          command : [mojom_parser,
> +                                     '--output-root', meson.build_root(),
> +                                     '--input-root', meson.source_root(),
> +                                     '--mojoms', '@INPUT@'
> +                          ])

I wonder if generator() could help, here and below.

> +    ipa_mojoms += { name: mojom }
> +endforeach
> +
> +#
> +# Generate headers from templates.
> +#
> +
> +# {pipeline}_generated.h
> +# generate {pipeline}_serializer.h
> +# generate ipa_proxy_{pipeline}.h
> +ipa_headers = {}
> +ipa_serializers = {}
> +ipa_proxy_headers = {}
> +
> +foreach name, mojom : ipa_mojoms

I think the two loops should be merged, a shown above.

> +    header = custom_target(name + '_generated_h',
> +                           input : mojom,
> +                           output : name + '_generated.h',
> +                           depends : mojom_template,
> +                           command : [mojom_generator, 'generate',
> +                                      '-g', 'libcamera',
> +                                      '--bytecode_path', '.',
> +                                      '--libcamera_generate_header',
> +                                      '--libcamera_output_path=@OUTPUT@',
> +                                      './' +'@INPUT@'
> +                           ])
> +
> +    serializer = custom_target(name + '_serializer_h',
> +                               input : mojom,
> +                               output : name + '_serializer.h',
> +                               depends : mojom_template,
> +                               command : [mojom_generator, 'generate',
> +                                          '-g', 'libcamera',
> +                                          '--bytecode_path', '.',
> +                                          '--libcamera_generate_serializer',
> +                                          '--libcamera_output_path=@OUTPUT@',
> +                                          './' +'@INPUT@'
> +                               ])
> +
> +    proxy_header = custom_target(name + '_proxy_h',
> +                                 input : mojom,
> +                                 output : 'ipa_proxy_' + name + '.h',
> +                                 depends : mojom_template,
> +                                 command : [mojom_generator, 'generate',
> +                                            '-g', 'libcamera',
> +                                            '--bytecode_path', '.',
> +                                            '--libcamera_generate_proxy_h',
> +                                            '--libcamera_output_path=@OUTPUT@',
> +                                            './' +'@INPUT@'
> +                                 ])
> +
> +    ipa_headers += { name: header }
> +    ipa_serializers += { name: serializer }
> +    ipa_proxy_headers += { name: proxy_header }
> +endforeach
> +
> +libcamera_internal_headers += ipa_proxy_headers
> +libcamera_internal_headers += ipa_headers
> +libcamera_internal_headers += ipa_serializers

This won't do much. libcamera_internal_headers is used to generate
documentation, which is out of scope for the generated headers, and
that's it. However, I think we should handle dependencies differently,
and this will then be needed. See below.

> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
> index bd804750..b0d35646 100644
> --- a/src/libcamera/proxy/meson.build
> +++ b/src/libcamera/proxy/meson.build
> @@ -4,3 +4,24 @@ libcamera_sources += files([
>      'ipa_proxy_linux.cpp',
>      'ipa_proxy_thread.cpp',
>  ])
> +
> +# generate ipa_proxy_{pipeline}.cpp
> +ipa_proxy_sources = []
> +
> +foreach name, mojom : ipa_mojoms
> +    ipa_proxy_sources += custom_target(name + '_proxy_cpp',

    proxy = custom_target(name + '_proxy_cpp',

The idea is to keep the name short as longer names, especially with an
ipa_ prefix, usually hint they are shared between files (and it will
also lower indentation, which is nice too).  There's unfortunately no
'local' keyword in meson (this is however being discussed, see
https://github.com/mesonbuild/meson/issues/2607). Maybe we should have
our own custom naming convention, but that's out of scope for this
series.

> +                                       input : mojom,
> +                                       output : 'ipa_proxy_' + name + '.cpp',
> +                                       depends : [mojom_template,
> +                                                  ipa_headers[name],
> +                                                  ipa_serializers[name],
> +                                                  ipa_proxy_headers[name]],

Are the dependencies right, does generation of the .cpp file depend on
the generated headers ? As long as the generated headers are added to
libcamera_sources, meson will ensure that they will be generated before
any source is compiled. The dependency on the headers to compile the ipa
proxy source will come from gcc -M.

The reason we've had trouble with generated files before was, I believe,
because the generated headers were not added to libcamera_sources. Their
generation could race with compilation of sources, either in a way that
would make the build fail from time to time, or in a nastier way that
made gcc -M miss the dependency as gcc will ignore any header it can't
find.

So, if we add the generated headers to libcamera_internal_headers, and
add libcamera_internal_headers to libcamera_sources in
src/libcamera/meson.build, the same way we do with
libcamera_public_headers, everything should be fine. I'd add
libcamera_internal_headers to libcamera_sources in a separate patch.

> +                                       command : [mojom_generator, 'generate',
> +                                                  '-g', 'libcamera',
> +                                                  '--bytecode_path', '.',
> +                                                  '--libcamera_generate_proxy_cpp',
> +                                                  '--libcamera_output_path=@OUTPUT@',
> +                                                  './' + '@INPUT@'])

    libcamera_sources += [proxy_source]

> +endforeach
> +
> +libcamera_sources += ipa_proxy_sources

And you can drop this line, as well as the ipa_proxy_sources array.

> diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build
> index ac0310a7..c35be70c 100644
> --- a/src/libcamera/proxy/worker/meson.build
> +++ b/src/libcamera/proxy/worker/meson.build
> @@ -4,10 +4,31 @@ ipa_proxy_sources = [
>      ['ipa_proxy_linux', 'ipa_proxy_linux_worker.cpp']
>  ]
>  
> +# generate ipa_proxy_{pipeline}_worker.cpp
> +ipa_proxy_sources = {}
> +
> +foreach name, mojom : ipa_mojoms
> +    worker = custom_target(name + '_proxy_worker',
> +                           input : mojom,
> +                           output : 'ipa_proxy_' + name + '_worker.cpp',
> +                           depends : [mojom_template,
> +                                      ipa_headers[name],
> +                                      ipa_serializers[name],
> +                                      ipa_proxy_headers[name]],
> +                           command : [mojom_generator, 'generate',
> +                                      '-g', 'libcamera',
> +                                      '--bytecode_path', '.',
> +                                      '--libcamera_generate_proxy_worker',
> +                                      '--libcamera_output_path=@OUTPUT@',
> +                                      './' + '@INPUT@'
> +                           ])
> +    ipa_proxy_sources += { 'ipa_proxy_' + name : worker }
> +endforeach
> +
>  proxy_install_dir = join_paths(get_option('libexecdir'), 'libcamera')
>  
> -foreach t : ipa_proxy_sources
> -    proxy = executable(t[0], t[1],
> +foreach k, t : ipa_proxy_sources

You can merge this loop with the one above, removing the
ipa_proxy_sources variable.

> +    proxy = executable(k, t,
>                         install : true,
>                         install_dir : proxy_install_dir,
>                         dependencies : libcamera_dep)

And here we will also need to add the generated headers that the worker
may depend on to the sources. One option is to add all the public and
private headers, it shouldn't hurt much. I'd also do so in a separate
patch.

Icing on the cake, once all of this will be addressed, you should test
the dependencies by touching selected files and making sure everything
gets properly rebuilt with ninja -v.

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
index 508c6bd1..f61a5a8f 100644
--- a/include/libcamera/ipa/meson.build
+++ b/include/libcamera/ipa/meson.build
@@ -8,3 +8,90 @@  libcamera_ipa_headers = files([
 
 install_headers(libcamera_ipa_headers,
                 subdir: join_paths(libcamera_include_dir, 'ipa'))
+
+#
+# Prepare IPA/IPC generation components
+#
+
+ipa_mojom_files = []
+
+mojom_generator = find_program('../../../utils/ipc/generate.py')
+
+mojom_template = custom_target('mojom_template',
+                               output : 'dummy',
+                               command : [mojom_generator, 'precompile'])
+
+mojom_parser = find_program('../../../utils/ipc/mojo/public/tools/mojom/mojom_parser.py')
+
+# {pipeline}.mojom-module
+ipa_mojoms = {}
+
+foreach file : ipa_mojom_files
+    name = file.split('.')[0]
+    mojom = custom_target(file.split('.')[0] + '_mojom_module',
+                          input : file,
+                          output : file + '-module',
+                          command : [mojom_parser,
+                                     '--output-root', meson.build_root(),
+                                     '--input-root', meson.source_root(),
+                                     '--mojoms', '@INPUT@'
+                          ])
+    ipa_mojoms += { name: mojom }
+endforeach
+
+#
+# Generate headers from templates.
+#
+
+# {pipeline}_generated.h
+# generate {pipeline}_serializer.h
+# generate ipa_proxy_{pipeline}.h
+ipa_headers = {}
+ipa_serializers = {}
+ipa_proxy_headers = {}
+
+foreach name, mojom : ipa_mojoms
+    header = custom_target(name + '_generated_h',
+                           input : mojom,
+                           output : name + '_generated.h',
+                           depends : mojom_template,
+                           command : [mojom_generator, 'generate',
+                                      '-g', 'libcamera',
+                                      '--bytecode_path', '.',
+                                      '--libcamera_generate_header',
+                                      '--libcamera_output_path=@OUTPUT@',
+                                      './' +'@INPUT@'
+                           ])
+
+    serializer = custom_target(name + '_serializer_h',
+                               input : mojom,
+                               output : name + '_serializer.h',
+                               depends : mojom_template,
+                               command : [mojom_generator, 'generate',
+                                          '-g', 'libcamera',
+                                          '--bytecode_path', '.',
+                                          '--libcamera_generate_serializer',
+                                          '--libcamera_output_path=@OUTPUT@',
+                                          './' +'@INPUT@'
+                               ])
+
+    proxy_header = custom_target(name + '_proxy_h',
+                                 input : mojom,
+                                 output : 'ipa_proxy_' + name + '.h',
+                                 depends : mojom_template,
+                                 command : [mojom_generator, 'generate',
+                                            '-g', 'libcamera',
+                                            '--bytecode_path', '.',
+                                            '--libcamera_generate_proxy_h',
+                                            '--libcamera_output_path=@OUTPUT@',
+                                            './' +'@INPUT@'
+                                 ])
+
+    ipa_headers += { name: header }
+    ipa_serializers += { name: serializer }
+    ipa_proxy_headers += { name: proxy_header }
+endforeach
+
+libcamera_internal_headers += ipa_proxy_headers
+libcamera_internal_headers += ipa_headers
+libcamera_internal_headers += ipa_serializers
diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
index bd804750..b0d35646 100644
--- a/src/libcamera/proxy/meson.build
+++ b/src/libcamera/proxy/meson.build
@@ -4,3 +4,24 @@  libcamera_sources += files([
     'ipa_proxy_linux.cpp',
     'ipa_proxy_thread.cpp',
 ])
+
+# generate ipa_proxy_{pipeline}.cpp
+ipa_proxy_sources = []
+
+foreach name, mojom : ipa_mojoms
+    ipa_proxy_sources += custom_target(name + '_proxy_cpp',
+                                       input : mojom,
+                                       output : 'ipa_proxy_' + name + '.cpp',
+                                       depends : [mojom_template,
+                                                  ipa_headers[name],
+                                                  ipa_serializers[name],
+                                                  ipa_proxy_headers[name]],
+                                       command : [mojom_generator, 'generate',
+                                                  '-g', 'libcamera',
+                                                  '--bytecode_path', '.',
+                                                  '--libcamera_generate_proxy_cpp',
+                                                  '--libcamera_output_path=@OUTPUT@',
+                                                  './' + '@INPUT@'])
+endforeach
+
+libcamera_sources += ipa_proxy_sources
diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build
index ac0310a7..c35be70c 100644
--- a/src/libcamera/proxy/worker/meson.build
+++ b/src/libcamera/proxy/worker/meson.build
@@ -4,10 +4,31 @@  ipa_proxy_sources = [
     ['ipa_proxy_linux', 'ipa_proxy_linux_worker.cpp']
 ]
 
+# generate ipa_proxy_{pipeline}_worker.cpp
+ipa_proxy_sources = {}
+
+foreach name, mojom : ipa_mojoms
+    worker = custom_target(name + '_proxy_worker',
+                           input : mojom,
+                           output : 'ipa_proxy_' + name + '_worker.cpp',
+                           depends : [mojom_template,
+                                      ipa_headers[name],
+                                      ipa_serializers[name],
+                                      ipa_proxy_headers[name]],
+                           command : [mojom_generator, 'generate',
+                                      '-g', 'libcamera',
+                                      '--bytecode_path', '.',
+                                      '--libcamera_generate_proxy_worker',
+                                      '--libcamera_output_path=@OUTPUT@',
+                                      './' + '@INPUT@'
+                           ])
+    ipa_proxy_sources += { 'ipa_proxy_' + name : worker }
+endforeach
+
 proxy_install_dir = join_paths(get_option('libexecdir'), 'libcamera')
 
-foreach t : ipa_proxy_sources
-    proxy = executable(t[0], t[1],
+foreach k, t : ipa_proxy_sources
+    proxy = executable(k, t,
                        install : true,
                        install_dir : proxy_install_dir,
                        dependencies : libcamera_dep)