[{"id":4071,"web_url":"https://patchwork.libcamera.org/comment/4071/","msgid":"<20200318124344.GG4733@pendragon.ideasonboard.com>","date":"2020-03-18T12:43:44","subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3 4/6] libcamera:\n\tipa_proxy: search for proxy in build tree","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kaaira,\n\nThank you for the patch.\n\nOn Wed, Mar 18, 2020 at 05:28:44PM +0530, Kaaira Gupta wrote:\n> When libcamera is built and tested before installing, it will\n> be unable to locate the path to proxy files, or previously\n\nI would write \"the path to the proxy workers\" (proxy workers are the\nstandalone executables that load an IPA module and handle communication\nwith the main libcamera process).\n\n> installed files in the system path may be incorrect to load.\n> \n> Hence, when libcamera is not installed and running from a build\n\nThis is a bit ambiguous, it would be understood as \"not installed and\nnot running from a build tree\". How about \"when libcamera is not\ninstalled, but is running from a build tree\" ?\n\n> tree, identify the location of that tree by using libcameraPath(), and\n> from that point add relative path to the proxy file.\n\ns/file/workers directory/ (the workers are all contained in a\ndirectory).\n\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> src/libcamera/ipa_proxy.cpp | 21 +++++++++++++++++----\n> 1 file changed, 17 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index b409e1d..1014e79 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -91,10 +91,23 @@ std::string IPAProxy::resolvePath(const std::string &file) const\n> \t\t}\n> \t}\n> \n> -\t/* Try finding the exec target from the install directory. */\n> -\tstd::string proxyPath = std::string(IPA_PROXY_DIR) + proxyFile;\n> -\tif (!access(proxyPath.c_str(), X_OK))\n> -\t\treturn proxyPath;\n\nThis shouldn't be removed, otherwise, when libcamera *is* installed,\nwe won't load the proxies from the normal installation directory. It\nshould instead be moved after the !installed case.\n\n> +\t/*\n> +\t * When libcamera is used before it is installed, load proxies from the\n\ns/proxies/proxy workers/\n\n> +\t * same build directory as the libcamera directory itself. This requires\n> +\t * identifying the path of the libcamera.so, and referencing a relative\n> +\t * path for the proxies from that point.\n\ns/proxies/proxy workers/\n\n> +\t */\n> +\tif (!utils::isLibcameraInstalled()) {\n> +\t\tstd::string ipaProxyDir = utils::dirname(utils::libcameraPath()) + \"/../../proxy\";\n\nThis isn't the correct path. In the source tree, the workers are located\nin src/libcamera/proxy/worker/. In the build directory that should thus\nbe + \"/proxy/worker\".\n\nCould you also wrap this line at a 80 columns limit ?\n\n\t\tstd::string ipaProxyDir = utils::dirname(utils::libcameraPath())\n\t\t\t\t\t+ \"/proxy/worker\";\n\n> +\n> +\t\tLOG(IPAProxy, Info)\n> +\t\t\t<< \"libcamera is not installed. Adding '\"\n> +\t\t\t<< ipaProxyDir << \"' to the Proxy search path\";\n\nHere you're not adding the directory to a search path, but you will end\nup using that directory only (IPA modules can be searched in multiple\npaths, proxy workers don't need that mechanism). I would write this\n\n\t\tLOG(IPAProxy, Info)\n\t\t\t<< \"libcamera is not installed. Loading proxy workers from '\"\n\t\t\t<< ipaProxyDir << \"'\";\n\n> +\n> +\t\tstd::string proxyPath = ipaProxyDir + proxyFile;\n> +\t\tif (!access(proxyPath.c_str(), X_OK))\n> +\t\t\treturn proxyPath;\n\nI think you need a\n\n\t\treturn std::string();\n\nhere, at least if you move the IPA_PROXY_DIR code from above to below\nthe !installed case. The reason is that proxy workers are tightly\ncoupled with libcamera. IPA modules can be developped independently and\nlibcamera will maintain ABI stability, so loading them from the system\ndirectory makes sense as a fallback option even running a fresh\nlibcamera build from the build directory. For workers, we don't want the\nones installed on the system to be used at all if libcamera is not\ninstalled, there's no use case for that.\n\n> +\t}\n> \n> \treturn std::string();\n> }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AA2160418\n\tfor ;\n\tWed, 18 Mar 2020 13:43:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6E351F9;\n\tWed, 18 Mar 2020 13:43:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584535429;\n\tbh=YxJE0l3kBnkX21HwX9+PDFZr3mQQ6GPR24OQf4XqxdE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Vt9xPICzxDVYRT709nsI6hK936ffoAt44PLFEiZmVVlsIDfYeBE53yj5AcI6Juuc/\n\t0jEKfdvg8euj7jnttNSoIlk2fhfeCHXOiXdBMNrbdw9l5wwLzr7ko4YREnFXe75tsP\n\tL99BZEMDcUetNx552gV5iJ1XNGBJluWglAuWXq64=","Date":"Wed, 18 Mar 2020 14:43:44 +0200","From":"Laurent Pinchart ","To":"Kaaira Gupta ","Cc":"Helen Koike ,\n\tVaishali Thakkar ,\n\tkieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","Message-ID":"<20200318124344.GG4733@pendragon.ideasonboard.com>","References":"<20200318115846.7975-1-kgupta@es.iitr.ac.in>\n\t<20200318115846.7975-5-kgupta@es.iitr.ac.in>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200318115846.7975-5-kgupta@es.iitr.ac.in>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3 4/6] libcamera:\n\tipa_proxy: search for proxy in build tree","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","X-List-Received-Date":"Wed, 18 Mar 2020 12:43:50 -0000"}}]