[libcamera-devel] [PATCH LIBCAMERA v3] libcamera: utils: adapt libcameraPath to match use cases

[Kaaira Gupta]

On Thu, Mar 19, 2020 at 01:36:53PM +0000, Kieran Bingham wrote:
> On 19/03/2020 13:02, Kaaira Gupta wrote:
> > On Thu, Mar 19, 2020 at 12:52:01PM +0000, Kieran Bingham wrote:
> >> Hi Kaaira,
> >>
> >> On 19/03/2020 12:42, Kaaira Gupta wrote:
> >>> The two callers of functions libcameraPath() and isLibcameraInstalled()
> >>> end up using the same process and finally use the path with
> >>> libcamera.so. Hence write a function libcameraBuildPath() which
> >>> combines their functions and returns the root of the build sources
> >>> when the library has not been installed, but is running,thereby making
> >>> call sites simpler.
> >>>
> >>> When the library is imstalled, libcameraBuildPath() will return an empty
> >>
> >> /imstalled/installed/
> >>
> >> If that's the only comment I'll fix while applying.
> > 
> > Do you check this manually or is there some tool which maybe I can also
> > use from the next time so that there are no such errors?
> 
> In this instance it was my mail client highlighting spelling errors.
> 
> I hope to be able to add spell-check support to checkstyle.py. It's
> something I've started, but haven't completed and the attempts I've made
> so far were really quite slow in performance :-(

Can't the checkpatch.pl script used in kernel be used for this as well?
It almost checks everything

> 
> --
> Kieran
> 
> 
> > 
> >>
> >>> string.
> >>>
> >>> Make changes in the call sites accordingly.
> >>>
> >>> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >>> ---
> >>> Changes since v2:
> >>> 	- rename subject line and add necessary details in commit
> >>> 	  message.
> >>> 	- Solve whitespace issues.
> >>> 	- specify the final 'slash' for directory path in return
> >>> 	  statement itself.
> >>>
> >>> Changes since v1:
> >>>         - rename function to libcameraBuildPath() instead, and return
> >>>           the root of libcamera source, and not the source directory
> >>>           root.
> >>>         - Return empty string instead of nullptr when ret==0
> >>>
> >>>  src/libcamera/include/utils.h |  2 +-
> >>>  src/libcamera/ipa_manager.cpp |  5 +++--
> >>>  src/libcamera/ipa_proxy.cpp   |  6 +++---
> >>>  src/libcamera/utils.cpp       | 16 +++++++++-------
> >>>  4 files changed, 16 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> >>> index bc96e79..5dea8d2 100644
> >>> --- a/src/libcamera/include/utils.h
> >>> +++ b/src/libcamera/include/utils.h
> >>> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim);
> >>>  
> >>>  bool isLibcameraInstalled();
> >>>  
> >>> -std::string libcameraPath();
> >>> +std::string libcameraBuildPath();
> >>>  
> >>>  } /* namespace utils */
> >>>  
> >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >>> index 0bd280c..bcaae35 100644
> >>> --- a/src/libcamera/ipa_manager.cpp
> >>> +++ b/src/libcamera/ipa_manager.cpp
> >>> @@ -119,8 +119,9 @@ IPAManager::IPAManager()
> >>>  	 * path for the IPA from that point. We need to recurse one level of
> >>>  	 * sub-directories to match the build tree.
> >>>  	 */
> >>> -	if (!utils::isLibcameraInstalled()) {
> >>> -		std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa";
> >>> +	std::string root = utils::libcameraBuildPath();
> >>> +	if (!root.empty()) {
> >>> +		std::string ipaBuildPath = root + "src/ipa";
> >>>  		constexpr int maxDepth = 1;
> >>>  
> >>>  		LOG(IPAManager, Info)
> >>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> >>> index 2f866cc..5fd88a4 100644
> >>> --- a/src/libcamera/ipa_proxy.cpp
> >>> +++ b/src/libcamera/ipa_proxy.cpp
> >>> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const
> >>>  	 * This requires identifying the path of the libcamera.so, and
> >>>  	 * referencing a relative path for the proxy workers from that point.
> >>>  	 */
> >>> -	if (!utils::isLibcameraInstalled()) {
> >>> -		std::string ipaProxyDir = utils::dirname(utils::libcameraPath())
> >>> -					  + "/proxy/worker";
> >>> +	std::string root = utils::libcameraBuildPath();
> >>> +	if (!root.empty()) {
> >>> +		std::string ipaProxyDir = root + "src/libcamera/proxy/worker";
> >>>  
> >>>  		LOG(IPAProxy, Info)
> >>>  			<< "libcamera is not installed. Loading proxy workers from'"
> >>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> >>> index 7e118fa..42c5c76 100644
> >>> --- a/src/libcamera/utils.cpp
> >>> +++ b/src/libcamera/utils.cpp
> >>> @@ -347,16 +347,18 @@ bool isLibcameraInstalled()
> >>>   *
> >>>   * \return A string stating the path
> >>>   */
> >>> -std::string libcameraPath()
> >>> +std::string libcameraBuildPath()
> >>>  {
> >>> -	Dl_info info;
> >>> +	if (!isLibcameraInstalled()) {
> >>> +		Dl_info info;
> >>>  
> >>> -	/* Look up our own symbol. */
> >>> -	int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
> >>> -	if (ret == 0)
> >>> -		return nullptr;
> >>> +		/* Look up our own symbol. */
> >>> +		int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info);
> >>> +		if (ret)
> >>> +			return dirname(info.dli_fname) + "/../../";
> >>> +	}
> >>>  
> >>> -	return info.dli_fname;
> >>> +	return std::string();
> >>>  }
> >>>  
> >>>  } /* namespace utils */
> >>>
> >>
> >> -- 
> >> Regards
> >> --
> >> Kieran
> 
> -- 
> Regards
> --
> Kieran