[libcamera-devel,0/4] libcamera: determine IPA_PROXY_PATH at runtime
mbox series

Message ID 20200317173145.18226-1-kgupta@es.iitr.ac.in
Headers show
Series
  • libcamera: determine IPA_PROXY_PATH at runtime
Related show

Message

Kaaira Gupta March 17, 2020, 5:31 p.m. UTC 
This is a series of patches to determine IPA_PROXY_PATH at runtime and
removing hard-coded LIBCAMERA_IPA_PROXY_PATH from test.cpp

Kaaira Gupta (4):
  libcamera: ipa_proxy: use utils::split()
  libcamera: ipa_proxy: rearrange proxies precedence
  libcamera: ipa_proxy: search for proxy in build tree
  tests: remove IPA_PROXY_PATH environment variable

 src/libcamera/ipa_proxy.cpp | 72 ++++++++++++++++++++++++++++---------
 test/libtest/test.cpp       |  4 ---
 2 files changed, 55 insertions(+), 21 deletions(-)

Comments

Kieran Bingham March 17, 2020, 8:44 p.m. UTC | #1 
Hi Kaaira,

Thank you for these patches - they're a good start.

The most common issue in my review was just whitespace topics, which is
just a style thing. You could skim read

  https://www.libcamera.org/coding-style.html#coding-style-guidelines

But installing clang-format and using the pre-commit hook will
automatically identify style topics for the most part for you.

The only other topic that came up is that I hadn't realised (or had
forgotten) that we need to move the static helpers isLibcameraInstalled
and libcameraPath out to be re-usable helpers in the utils module.

I'm sorry - it would have been easier if I'd mentioned that first :-(

Could you do that move as a separate patch please?

Otherwise, it's looking good - and solves the issue!

Thanks

Kieran


On 17/03/2020 17:31, Kaaira Gupta wrote:
> This is a series of patches to determine IPA_PROXY_PATH at runtime and
> removing hard-coded LIBCAMERA_IPA_PROXY_PATH from test.cpp
> 
> Kaaira Gupta (4):
>   libcamera: ipa_proxy: use utils::split()
>   libcamera: ipa_proxy: rearrange proxies precedence
>   libcamera: ipa_proxy: search for proxy in build tree
>   tests: remove IPA_PROXY_PATH environment variable
> 
>  src/libcamera/ipa_proxy.cpp | 72 ++++++++++++++++++++++++++++---------
>  test/libtest/test.cpp       |  4 ---
>  2 files changed, 55 insertions(+), 21 deletions(-)
>
Kaaira Gupta March 17, 2020, 9:39 p.m. UTC | #2 
On Tue, Mar 17, 2020 at 08:44:05PM +0000, Kieran Bingham wrote:
> Hi Kaaira,
> 
> Thank you for these patches - they're a good start.

Thanks :D

> 
> The most common issue in my review was just whitespace topics, which is
> just a style thing. You could skim read

I'll fix them up

> 
>   https://www.libcamera.org/coding-style.html#coding-style-guidelines
> 
> But installing clang-format and using the pre-commit hook will
> automatically identify style topics for the most part for you.
> 
> The only other topic that came up is that I hadn't realised (or had
> forgotten) that we need to move the static helpers isLibcameraInstalled
> and libcameraPath out to be re-usable helpers in the utils module.
> 
> I'm sorry - it would have been easier if I'd mentioned that first :-(
> 
> Could you do that move as a separate patch please?

Yes, sure. I'll just send that as patch 3 of this series.

> 
> Otherwise, it's looking good - and solves the issue!
> 
> Thanks
> 
> Kieran
> 
> 
> On 17/03/2020 17:31, Kaaira Gupta wrote:
> > This is a series of patches to determine IPA_PROXY_PATH at runtime and
> > removing hard-coded LIBCAMERA_IPA_PROXY_PATH from test.cpp
> > 
> > Kaaira Gupta (4):
> >   libcamera: ipa_proxy: use utils::split()
> >   libcamera: ipa_proxy: rearrange proxies precedence
> >   libcamera: ipa_proxy: search for proxy in build tree
> >   tests: remove IPA_PROXY_PATH environment variable
> > 
> >  src/libcamera/ipa_proxy.cpp | 72 ++++++++++++++++++++++++++++---------
> >  test/libtest/test.cpp       |  4 ---
> >  2 files changed, 55 insertions(+), 21 deletions(-)
> > 
> 
> -- 
> Regards
> --
> Kieran