[{"id":12536,"web_url":"https://patchwork.libcamera.org/comment/12536/","msgid":"<20200916014523.GH14954@pendragon.ideasonboard.com>","date":"2020-09-16T01:45:23","subject":"Re: [libcamera-devel] [PATCH 05/23] meson: ipa,\n\tproxy: Generate headers and proxy with mojo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Sep 15, 2020 at 11:20:20PM +0900, Paul Elder wrote:\n> Run mojo from meson to generate the header, serializer, and proxy files\n> for every pipeline's mojom data definition file. So far we only have the\n> raspberrypi mojom file.\n> \n> Signed-off-by: Paul Elder \n> ---\n> \n> Dictionaries with non-literal keys is apparently a meson 0.53 feature.\n\nAnd the latest stable Debian doesn't ship 0.53 :-S Let's see if we can\ndo without that feature. How about the following ?\n\ndiff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\nindex c80594d7db58..5d174750d6d2 100644\n--- a/include/libcamera/ipa/meson.build\n+++ b/include/libcamera/ipa/meson.build\n@@ -25,11 +25,16 @@ mojom_template = custom_target('mojom_template',\n \n mojom_parser = find_program('../../../utils/ipc/mojo/public/tools/mojom/mojom_parser.py')\n \n-# {pipeline}.mojom-module\n-ipa_mojoms = {}\n+ipa_mojoms = []\n+\n+#\n+# Generate headers from templates.\n+#\n \n foreach file : ipa_mojom_files\n name = file.split('.')[0]\n+\n+ # {pipeline}.mojom-module\n mojom = custom_target(file.split('.')[0] + '_mojom_module',\n input : file,\n output : file + '-module',\n@@ -38,21 +43,7 @@ foreach file : ipa_mojom_files\n '--input-root', meson.source_root(),\n '--mojoms', '@INPUT@'\n ])\n- ipa_mojoms += { name: mojom }\n-endforeach\n-\n-#\n-# Generate headers from templates.\n-#\n-\n-# {pipeline}_generated.h\n-# generate {pipeline}_serializer.h\n-# generate ipa_proxy_{pipeline}.h\n-ipa_headers = {}\n-ipa_serializers = {}\n-ipa_proxy_headers = {}\n-\n-foreach name, mojom : ipa_mojoms\n+ # {pipeline}_generated.h\n header = custom_target(name + '_generated_h',\n input : mojom,\n output : name + '_generated.h',\n@@ -65,6 +56,7 @@ foreach name, mojom : ipa_mojoms\n './' +'@INPUT@'\n ])\n \n+ # {pipeline}_serializer.h\n serializer = custom_target(name + '_serializer_h',\n input : mojom,\n output : name + '_serializer.h',\n@@ -77,6 +69,7 @@ foreach name, mojom : ipa_mojoms\n './' +'@INPUT@'\n ])\n \n+ # ipa_proxy_{pipeline}.h\n proxy_header = custom_target(name + '_proxy_h',\n input : mojom,\n output : 'ipa_proxy_' + name + '.h',\n@@ -89,11 +82,11 @@ foreach name, mojom : ipa_mojoms\n './' +'@INPUT@'\n ])\n \n- ipa_headers += { name: header }\n- ipa_serializers += { name: serializer }\n- ipa_proxy_headers += { name: proxy_header }\n-endforeach\n+ ipa_mojoms += {\n+ 'name': name,\n+ 'mojom': mojom,\n+ 'dependencies': [header, serializer, proxy_header],\n+ }\n \n-libcamera_internal_headers += ipa_proxy_headers\n-libcamera_internal_headers += ipa_headers\n-libcamera_internal_headers += ipa_serializers\n+ libcamera_internal_headers += [header, serializer, proxy_header]\n+endforeach\ndiff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build\nindex cfec0567bf5e..df260227a88a 100644\n--- a/src/libcamera/proxy/meson.build\n+++ b/src/libcamera/proxy/meson.build\n@@ -3,14 +3,12 @@\n # generate ipa_proxy_{pipeline}.cpp\n ipa_proxy_sources = []\n \n-foreach name, mojom : ipa_mojoms\n+foreach mojom : ipa_mojoms\n+ name = mojom['name']\n ipa_proxy_sources += custom_target(name + '_proxy_cpp',\n- input : mojom,\n+ input : mojom['mojom'],\n output : 'ipa_proxy_' + name + '.cpp',\n- depends : [mojom_template,\n- ipa_headers[name],\n- ipa_serializers[name],\n- ipa_proxy_headers[name]],\n+ depends : [mojom_template, mojom['dependencies']],\n command : [mojom_generator, 'generate',\n '-g', 'libcamera',\n '--bytecode_path', '.',\ndiff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build\nindex db4630656905..2a739af91a31 100644\n--- a/src/libcamera/proxy/worker/meson.build\n+++ b/src/libcamera/proxy/worker/meson.build\n@@ -2,16 +2,14 @@\n \n \n # generate ipa_proxy_{pipeline}_worker.cpp\n-ipa_proxy_sources = {}\n+ipa_proxy_sources = []\n \n-foreach name, mojom : ipa_mojoms\n+foreach mojom : ipa_mojoms\n+ name = mojom['name']\n worker = custom_target(name + '_proxy_worker',\n- input : mojom,\n+ input : mojom['mojom'],\n output : 'ipa_proxy_' + name + '_worker.cpp',\n- depends : [mojom_template,\n- ipa_headers[name],\n- ipa_serializers[name],\n- ipa_proxy_headers[name]],\n+ depends : [mojom_template, mojom['dependencies']],\n command : [mojom_generator, 'generate',\n '-g', 'libcamera',\n '--bytecode_path', '.',\n@@ -19,13 +17,16 @@ foreach name, mojom : ipa_mojoms\n '--libcamera_output_path=@OUTPUT@',\n './' + '@INPUT@'\n ])\n- ipa_proxy_sources += { 'ipa_proxy_' + name : worker }\n+ ipa_proxy_sources += {\n+ 'name': 'ipa_proxy_' + name,\n+ 'worker': worker,\n+ }\n endforeach\n \n proxy_install_dir = join_paths(get_option('libexecdir'), 'libcamera')\n \n-foreach k, t : ipa_proxy_sources\n- proxy = executable(k, t,\n+foreach src : ipa_proxy_sources\n+ proxy = executable(src['name'], src['worker'],\n install : true,\n install_dir : proxy_install_dir,\n dependencies : libcamera_dep)\n\nBy the way, the reason why arrays of arrays failed for you is likely due\nto the fact that when meson interprets the following code\n\nfoo = []\nfoo += [1,2,3]\nfoo += [4,5,6]\n\nfoo is equal to [1,2,3,4,5,6], not [[1,2,3],[4,5,6]]\n\nYou would need to write\n\nfoo = []\nfoo += [[1,2,3]]\nfoo += [[4,5,6]]\n\nBut dictionaries are nicer anyway, it's best to use a string literal as\na key than hardcoding an array index.\n\n> \n> include/libcamera/ipa/meson.build | 87 ++++++++++++++++++++++++++\n> src/libcamera/proxy/meson.build | 21 +++++++\n> src/libcamera/proxy/worker/meson.build | 25 +++++++-\n> 3 files changed, 131 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index 508c6bd1..f61a5a8f 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -8,3 +8,90 @@ libcamera_ipa_headers = files([\n> \n> install_headers(libcamera_ipa_headers,\n> subdir: join_paths(libcamera_include_dir, 'ipa'))\n> +\n> +#\n> +# Prepare IPA/IPC generation components\n> +#\n> +\n> +ipa_mojom_files = []\n> +\n> +mojom_generator = find_program('../../../utils/ipc/generate.py')\n\nThis should be rebased on top of \"[PATCH 0/3] Unify utils locations\", or\njust \"[PATCH 1/3] meson: Process utils first\". You can then add a\nmeson.build to utils/ipc/ and move this line there.\n\n> +\n> +mojom_template = custom_target('mojom_template',\n> + output : 'dummy',\n> + command : [mojom_generator, 'precompile'])\n\nYou will need to add inputs, otherwise mojom_template won't be rebuilt\nwhen the templates change. You don't need to use the inputs in the\ncommand, but they have to be listed. It will require keeping meson.build\nin sync with the list of templates, but that should be manageable,\nespecially if we add a meson.build to\nutils/ipc/generators/libcamera_templates.\n\n> +mojom_parser = find_program('../../../utils/ipc/mojo/public/tools/mojom/mojom_parser.py')\n\nAll this should also go to utils/ipc/meson.build.\n\n> +\n> +# {pipeline}.mojom-module\n> +ipa_mojoms = {}\n> +\n> +foreach file : ipa_mojom_files\n> + name = file.split('.')[0]\n> + mojom = custom_target(file.split('.')[0] + '_mojom_module',\n> + input : file,\n> + output : file + '-module',\n> + command : [mojom_parser,\n> + '--output-root', meson.build_root(),\n> + '--input-root', meson.source_root(),\n> + '--mojoms', '@INPUT@'\n> + ])\n\nI wonder if generator() could help, here and below.\n\n> + ipa_mojoms += { name: mojom }\n> +endforeach\n> +\n> +#\n> +# Generate headers from templates.\n> +#\n> +\n> +# {pipeline}_generated.h\n> +# generate {pipeline}_serializer.h\n> +# generate ipa_proxy_{pipeline}.h\n> +ipa_headers = {}\n> +ipa_serializers = {}\n> +ipa_proxy_headers = {}\n> +\n> +foreach name, mojom : ipa_mojoms\n\nI think the two loops should be merged, a shown above.\n\n> + header = custom_target(name + '_generated_h',\n> + input : mojom,\n> + output : name + '_generated.h',\n> + depends : mojom_template,\n> + command : [mojom_generator, 'generate',\n> + '-g', 'libcamera',\n> + '--bytecode_path', '.',\n> + '--libcamera_generate_header',\n> + '--libcamera_output_path=@OUTPUT@',\n> + './' +'@INPUT@'\n> + ])\n> +\n> + serializer = custom_target(name + '_serializer_h',\n> + input : mojom,\n> + output : name + '_serializer.h',\n> + depends : mojom_template,\n> + command : [mojom_generator, 'generate',\n> + '-g', 'libcamera',\n> + '--bytecode_path', '.',\n> + '--libcamera_generate_serializer',\n> + '--libcamera_output_path=@OUTPUT@',\n> + './' +'@INPUT@'\n> + ])\n> +\n> + proxy_header = custom_target(name + '_proxy_h',\n> + input : mojom,\n> + output : 'ipa_proxy_' + name + '.h',\n> + depends : mojom_template,\n> + command : [mojom_generator, 'generate',\n> + '-g', 'libcamera',\n> + '--bytecode_path', '.',\n> + '--libcamera_generate_proxy_h',\n> + '--libcamera_output_path=@OUTPUT@',\n> + './' +'@INPUT@'\n> + ])\n> +\n> + ipa_headers += { name: header }\n> + ipa_serializers += { name: serializer }\n> + ipa_proxy_headers += { name: proxy_header }\n> +endforeach\n> +\n> +libcamera_internal_headers += ipa_proxy_headers\n> +libcamera_internal_headers += ipa_headers\n> +libcamera_internal_headers += ipa_serializers\n\nThis won't do much. libcamera_internal_headers is used to generate\ndocumentation, which is out of scope for the generated headers, and\nthat's it. However, I think we should handle dependencies differently,\nand this will then be needed. See below.\n\n> diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build\n> index bd804750..b0d35646 100644\n> --- a/src/libcamera/proxy/meson.build\n> +++ b/src/libcamera/proxy/meson.build\n> @@ -4,3 +4,24 @@ libcamera_sources += files([\n> 'ipa_proxy_linux.cpp',\n> 'ipa_proxy_thread.cpp',\n> ])\n> +\n> +# generate ipa_proxy_{pipeline}.cpp\n> +ipa_proxy_sources = []\n> +\n> +foreach name, mojom : ipa_mojoms\n> + ipa_proxy_sources += custom_target(name + '_proxy_cpp',\n\n proxy = custom_target(name + '_proxy_cpp',\n\nThe idea is to keep the name short as longer names, especially with an\nipa_ prefix, usually hint they are shared between files (and it will\nalso lower indentation, which is nice too). There's unfortunately no\n'local' keyword in meson (this is however being discussed, see\nhttps://github.com/mesonbuild/meson/issues/2607). Maybe we should have\nour own custom naming convention, but that's out of scope for this\nseries.\n\n> + input : mojom,\n> + output : 'ipa_proxy_' + name + '.cpp',\n> + depends : [mojom_template,\n> + ipa_headers[name],\n> + ipa_serializers[name],\n> + ipa_proxy_headers[name]],\n\nAre the dependencies right, does generation of the .cpp file depend on\nthe generated headers ? As long as the generated headers are added to\nlibcamera_sources, meson will ensure that they will be generated before\nany source is compiled. The dependency on the headers to compile the ipa\nproxy source will come from gcc -M.\n\nThe reason we've had trouble with generated files before was, I believe,\nbecause the generated headers were not added to libcamera_sources. Their\ngeneration could race with compilation of sources, either in a way that\nwould make the build fail from time to time, or in a nastier way that\nmade gcc -M miss the dependency as gcc will ignore any header it can't\nfind.\n\nSo, if we add the generated headers to libcamera_internal_headers, and\nadd libcamera_internal_headers to libcamera_sources in\nsrc/libcamera/meson.build, the same way we do with\nlibcamera_public_headers, everything should be fine. I'd add\nlibcamera_internal_headers to libcamera_sources in a separate patch.\n\n> + command : [mojom_generator, 'generate',\n> + '-g', 'libcamera',\n> + '--bytecode_path', '.',\n> + '--libcamera_generate_proxy_cpp',\n> + '--libcamera_output_path=@OUTPUT@',\n> + './' + '@INPUT@'])\n\n libcamera_sources += [proxy_source]\n\n> +endforeach\n> +\n> +libcamera_sources += ipa_proxy_sources\n\nAnd you can drop this line, as well as the ipa_proxy_sources array.\n\n> diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build\n> index ac0310a7..c35be70c 100644\n> --- a/src/libcamera/proxy/worker/meson.build\n> +++ b/src/libcamera/proxy/worker/meson.build\n> @@ -4,10 +4,31 @@ ipa_proxy_sources = [\n> ['ipa_proxy_linux', 'ipa_proxy_linux_worker.cpp']\n> ]\n> \n> +# generate ipa_proxy_{pipeline}_worker.cpp\n> +ipa_proxy_sources = {}\n> +\n> +foreach name, mojom : ipa_mojoms\n> + worker = custom_target(name + '_proxy_worker',\n> + input : mojom,\n> + output : 'ipa_proxy_' + name + '_worker.cpp',\n> + depends : [mojom_template,\n> + ipa_headers[name],\n> + ipa_serializers[name],\n> + ipa_proxy_headers[name]],\n> + command : [mojom_generator, 'generate',\n> + '-g', 'libcamera',\n> + '--bytecode_path', '.',\n> + '--libcamera_generate_proxy_worker',\n> + '--libcamera_output_path=@OUTPUT@',\n> + './' + '@INPUT@'\n> + ])\n> + ipa_proxy_sources += { 'ipa_proxy_' + name : worker }\n> +endforeach\n> +\n> proxy_install_dir = join_paths(get_option('libexecdir'), 'libcamera')\n> \n> -foreach t : ipa_proxy_sources\n> - proxy = executable(t[0], t[1],\n> +foreach k, t : ipa_proxy_sources\n\nYou can merge this loop with the one above, removing the\nipa_proxy_sources variable.\n\n> + proxy = executable(k, t,\n> install : true,\n> install_dir : proxy_install_dir,\n> dependencies : libcamera_dep)\n\nAnd here we will also need to add the generated headers that the worker\nmay depend on to the sources. One option is to add all the public and\nprivate headers, it shouldn't hurt much. I'd also do so in a separate\npatch.\n\nIcing on the cake, once all of this will be addressed, you should test\nthe dependencies by touching selected files and making sure everything\ngets properly rebuilt with ninja -v.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B45DEBF01C\n\tfor ;\n\tWed, 16 Sep 2020 01:45:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B70E62D27;\n\tWed, 16 Sep 2020 03:45:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE19362C8C\n\tfor ;\n\tWed, 16 Sep 2020 03:45:53 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09AC657F;\n\tWed, 16 Sep 2020 03:45:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Urzfy4KN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600220753;\n\tbh=oYzP2JbxDaSjiS4p6sEx8gHypA6o9Hg2OBz+zbl9IVE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Urzfy4KNhl5ty3GmESuuWIxHWUhe9B4jybquzEr+q7HWOKEUtSn8dYiKJOz6ndukx\n\tQBsWI5RzlMsDKxYlEu4EclWK9x/K3XYSybUahQ6npAyo9QZJ7u9ITiPbdlF5Z8KQgF\n\t4c76Rvi9IYzOKPUgIPQGWt4XeZ2kxVflhpgF1hsU=","Date":"Wed, 16 Sep 2020 04:45:23 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200916014523.GH14954@pendragon.ideasonboard.com>","References":"<20200915142038.28757-1-paul.elder@ideasonboard.com>\n\t<20200915142038.28757-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915142038.28757-6-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 05/23] meson: ipa,\n\tproxy: Generate headers and proxy with mojo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]