| Message ID | 20200319152533.GA4938@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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/ ? > 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@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. /** * \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. Reviewed-by: Laurent Pinchart <laurent.pinchart@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 */
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@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@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
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 */ -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 */
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 installed, libcameraBuildPath() will return an empty string. Make changes in the call sites accordingly. Signed-off-by: Kaaira Gupta <kgupta@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(-)