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

[Kaaira Gupta]

On Thu, Mar 19, 2020 at 05:41:42PM +0200, Laurent Pinchart wrote:
> Hi Kaaira,
> 
> Thank you for the patch.
> 
> On Thu, Mar 19, 2020 at 08:55:33PM +0530, 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
> 
> s/running/running from the build directory/ ?

Yes I should clear that explicitly. I'll do that.

> 
> > call sites simpler.
> > 
> > When the library is installed, libcameraBuildPath() will return an empty
> > string.
> > 
> > Make changes in the call sites accordingly.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> > ---
> > Changes since v3:
> >         - removed isLibcameraInstalled() from utils.h
> >         - reformatted the code to reduce indentation of the code that
> >           implements actual logic, also, to catch the errors
> >           immediately.
> >         - changes in spellings and whitespaces in commit messages.
> > 
> > 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 |  4 +---
> >  src/libcamera/ipa_manager.cpp |  5 +++--
> >  src/libcamera/ipa_proxy.cpp   |  6 +++---
> >  src/libcamera/utils.cpp       | 11 +++++++----
> >  4 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> > index bc96e79..cfa620f 100644
> > --- a/src/libcamera/include/utils.h
> > +++ b/src/libcamera/include/utils.h
> > @@ -143,9 +143,7 @@ private:
> >  
> >  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..40a8367 100644
> > --- a/src/libcamera/utils.cpp
> > +++ b/src/libcamera/utils.cpp
> > @@ -347,16 +347,19 @@ bool isLibcameraInstalled()
> >   *
> >   * \return A string stating the path
> 
> I think you forgot to update the documentation :-) I propose the
> following.

Sorry, I should be more careful.

> 
> /**
>  * \brief Retrieve the path to the build directory
>  *
>  * During development, it is useful to run libcamera binaries directly from the
>  * build directory without installing them. This function helps components that
>  * need to locate resources, such as IPA modules or IPA proxy workers, by
>  * providing them with the path to the root of the build directory. Callers can
>  * then use it to complement or override searches in system-wide directories.
>  *
>  * If libcamera has been installed, the build directory path is not available
>  * and this function returns an empty string.
>  *
>  * \return The path to the build directory if running from a build, or an empty
>  * string otherwise
>  */
> 
> If that works for you, I'll add this when applying.

I'll do that and send another version now itself if that's fine.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> >   */
> > -std::string libcameraPath()
> > +std::string libcameraBuildPath()
> >  {
> > +	if (isLibcameraInstalled())
> > +		return std::string();
> > +
> >  	Dl_info info;
> >  
> >  	/* Look up our own symbol. */
> > -	int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
> > +	int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info);
> >  	if (ret == 0)
> > -		return nullptr;
> > +		return std::string();
> >  
> > -	return info.dli_fname;
> > +	return dirname(info.dli_fname) + "/../../";
> >  }
> >  
> >  } /* namespace utils */
> 

Thanks for the time!

> -- 
> Regards,
> 
> Laurent Pinchart