[{"id":4090,"web_url":"https://patchwork.libcamera.org/comment/4090/","msgid":"<510d7f39-a627-a21a-93fc-068f3fe3b501@ideasonboard.com>","date":"2020-03-19T12:25:46","subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v2] libcamera: utils: merge\n\tisLibcamerainstalled with libcameraPath","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira\n\nI think we need to adjust the Subject a little:\n\n libcamera: utils: merge isLibcamerainstalled with libcameraPath\n\nWe aren't merging them, we keep the two functions separate, we are\nadapting libcameraPath() to a new function to suit the goals of existing\ncall sites:\n\nPerhaps:\n libcamera: utils: Rework and rename libcameraPath to match use cases ?\n\n\nOn 19/03/2020 11:59, Kaaira Gupta wrote:\n> The two callers of functions libcameraPath() and isLibcameraInstalled()\n> end up using the same process and finally use the path with\n> libcamera.so. Hence write a function libcameraBuildPath() which\n> combines their functions and returns the root of the libcamera sources,\n\nIt returns the root of the build when the library has not been\ninstalled, not the root of the sources, which is potentially an\nimportant distinction to make (they are different directories).\n\n> thereby making call sites simpler.\n\nIt could be worth highlighting here for documentation sake,\n\n\"When the library is installed, libcameraBuildPath() will return an\nempty string.\"\n\n\n> Make changes in the call sites accordingly.\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> \n> Changes since v1:\n> \t- rename function to libcameraBuildPath() instead, and return\n> \t the root of libcamera source, and not the source directory\n> \t root.\n> \t- Return empty string instead of nullptr when ret==0\n> \n> src/libcamera/include/utils.h | 2 +-\n> src/libcamera/ipa_manager.cpp | 5 +++--\n> src/libcamera/ipa_proxy.cpp | 6 +++---\n> src/libcamera/utils.cpp | 17 +++++++++--------\n> 4 files changed, 16 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> index bc96e79..5dea8d2 100644\n> --- a/src/libcamera/include/utils.h\n> +++ b/src/libcamera/include/utils.h\n> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim);\n> \n> bool isLibcameraInstalled();\n> \n> -std::string libcameraPath();\n> +std::string libcameraBuildPath();\n> \n> } /* namespace utils */\n> \n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 0bd280c..4e294c2 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -119,8 +119,9 @@ IPAManager::IPAManager()\n> \t * path for the IPA from that point. We need to recurse one level of\n> \t * sub-directories to match the build tree.\n> \t */\n> -\tif (!utils::isLibcameraInstalled()) {\n> -\t\tstd::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + \"/../ipa\";\n> +\tstd::string root = utils::libcameraBuildPath();\n> +\tif (!root.empty()) {\n> +\t\tstd::string ipaBuildPath = root + \"/src/ipa\";\n\nGreat, I think that's much clearer intent, and to read...\n\n> \t\tconstexpr int maxDepth = 1;\n> \n> \t\tLOG(IPAManager, Info)\n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index 2f866cc..5fd88a4 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const\n> \t * This requires identifying the path of the libcamera.so, and\n> \t * referencing a relative path for the proxy workers from that point.\n> \t */\n> -\tif (!utils::isLibcameraInstalled()) {\n> -\t\tstd::string ipaProxyDir = utils::dirname(utils::libcameraPath())\n> -\t\t\t\t\t + \"/proxy/worker\";\n> +\tstd::string root = utils::libcameraBuildPath();\n> +\tif (!root.empty()) {\n> +\t\tstd::string ipaProxyDir = root + \"src/libcamera/proxy/worker\";\n\nAnd that too seems good to me!\n\n> \n> \t\tLOG(IPAProxy, Info)\n> \t\t\t<< \"libcamera is not installed. Loading proxy workers from'\"\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 7e118fa..eae682b 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -347,16 +347,17 @@ bool isLibcameraInstalled()\n> *\n> * \\return A string stating the path\n> */\n> -std::string libcameraPath()\n> +std::string libcameraBuildPath()\n> {\n> -\tDl_info info;\n> -\n> -\t/* Look up our own symbol. */\n> -\tint ret = dladdr(reinterpret_cast(libcameraPath), &info);\n> -\tif (ret == 0)\n> -\t\treturn nullptr;\n> +\tif (!isLibcameraInstalled()) {\n> +\t\tDl_info info;\n\nThe separating line here was probably worth keeping...\n\n\n> +\t\t/* Look up our own symbol. */\n> +\t\tint ret = dladdr(reinterpret_cast(libcameraBuildPath), &info);\n> +\t\tif (ret)\n> +\t\t\treturn dirname(info.dli_fname) + \"/../..\";\n\nOh ...\n\nWe have:\n\troot = dirname(info.dli_fname) + \"/../..\";\n\nand\n\tstd::string ipaBuildPath = root + \"/src/ipa\";\n\nbut\n\tstd::string ipaProxyDir = root + \"src/libcamera/proxy/worker\";\n\nI think I would prefer the ipaProxyDir variant (without needed to\nspecify the leading slash).\n--\nRegards\n\nKieran\n\n\n> +\t}\n> \n> -\treturn info.dli_fname;\n> +\treturn std::string();\n> }\n> \n> } /* namespace utils */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E94BF629AC\n\tfor ;\n\tThu, 19 Mar 2020 13:25:49 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1EF08A53;\n\tThu, 19 Mar 2020 13:25:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584620749;\n\tbh=VPDvMfMxnhDDfHjKaO+Fwd3OZQ3rvml9txhhnY9fS6c=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=RjhNrI311Tqv71qUoo2JYU9glmxfcXHd7cL+Ivtf8L59YpzIA5xox/1YCC1mogL4L\n\tJjLKN0q7ANGd/2rOrsVcmyHKGvrKGa1f491KujB340SBHlN0Ur6iFqmLRJ4UiwHk8H\n\tFMiZBMQx2OF8d7xFzEbf8LO+Po3jSF5eqTrhIzbc=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta ,\n\tHelen Koike ,\n\tVaishali Thakkar ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200319115908.GA30421@kaaira-HP-Pavilion-Notebook>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<510d7f39-a627-a21a-93fc-068f3fe3b501@ideasonboard.com>","Date":"Thu, 19 Mar 2020 12:25:46 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200319115908.GA30421@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v2] libcamera: utils: merge\n\tisLibcamerainstalled with libcameraPath","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":"Thu, 19 Mar 2020 12:25:50 -0000"}}]