| Message ID | 20220904162715.286574-1-Rauch.Christian@gmx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Christian, Thank you for the patch. On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: > Compiling with tests adds a significant amount of build targets to the > meson build. However, tests are only of interest when a feature has been > implemented or for continuous integration. > Disable tests by default to reduce the computational resource requirements > and the duration of a complete libcamera built. > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> As most people building libcamera won't run tests, this seems reasonable to me. I'm however a bit worried that it would lead to lower compilation-test coverage, as people may send patches without realizing they break compilation of tests. We already have that problem for runtime breakages of tests, and it's most likely be addressed by test automation on patches submitted upstream, so I think I'm fine with the change. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > meson_options.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/meson_options.txt b/meson_options.txt > index 7a9aecfc..f1d67808 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -47,6 +47,7 @@ option('qcam', > > option('test', > type : 'boolean', > + value : false, > description: 'Compile and include the tests') > > option('tracing',
Hi Christian, Thank you for the patch. On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: > Hi Christian, > > Thank you for the patch. > > On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: >> Compiling with tests adds a significant amount of build targets to the >> meson build. However, tests are only of interest when a feature has been >> implemented or for continuous integration. >> Disable tests by default to reduce the computational resource requirements >> and the duration of a complete libcamera built. >> >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > As most people building libcamera won't run tests, this seems reasonable > to me. I'm however a bit worried that it would lead to lower > compilation-test coverage, as people may send patches without realizing > they break compilation of tests. We already have that problem for I'm concerned about this as well. > runtime breakages of tests, and it's most likely be addressed by test > automation on patches submitted upstream, so I think I'm fine with the > change. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- >> meson_options.txt | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/meson_options.txt b/meson_options.txt >> index 7a9aecfc..f1d67808 100644 >> --- a/meson_options.txt >> +++ b/meson_options.txt >> @@ -47,6 +47,7 @@ option('qcam', >> >> option('test', >> type : 'boolean', >> + value : false, >> description: 'Compile and include the tests') >> >> option('tracing',
Hi All, Quoting Umang Jain via libcamera-devel (2022-09-05 07:41:50) > Hi Christian, > > Thank you for the patch. > > On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: > > Hi Christian, > > > > Thank you for the patch. > > > > On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: > >> Compiling with tests adds a significant amount of build targets to the > >> meson build. However, tests are only of interest when a feature has been > >> implemented or for continuous integration. > >> Disable tests by default to reduce the computational resource requirements > >> and the duration of a complete libcamera built. > >> > >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > As most people building libcamera won't run tests, this seems reasonable > > to me. I'm however a bit worried that it would lead to lower > > compilation-test coverage, as people may send patches without realizing > > they break compilation of tests. We already have that problem for > > I'm concerned about this as well. When I first saw this patch over the weekend, indeed my reaction was "Ahhh" ... but I also agree this is a better default to have off right now. We would like patches to be submitted after having all tests run, but the majority of people who compile libcamera (currently) may not be developers. We have lots of people compiling on RPi / IPU3 and low power devices directly, and they certainly don't care for running tests. So to improve user experience, I do think this should be off by default (at least now) and that stance may change depending on our upcoming release strategies ... but developers should be able to enable tests anyway. Ideally of course these tests will then be enabled and run on any branch push to the common public hosting services, so I think we should integrate a gitlab and github CI script that could build and run the unit tests. (These could skip the kernel related tests initially). Anyway, Yes - Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > runtime breakages of tests, and it's most likely be addressed by test > > automation on patches submitted upstream, so I think I'm fine with the > > change. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > >> --- > >> meson_options.txt | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/meson_options.txt b/meson_options.txt > >> index 7a9aecfc..f1d67808 100644 > >> --- a/meson_options.txt > >> +++ b/meson_options.txt > >> @@ -47,6 +47,7 @@ option('qcam', > >> > >> option('test', > >> type : 'boolean', > >> + value : false, > >> description: 'Compile and include the tests') > >> > >> option('tracing', >
On Mon, Sep 05, 2022 at 10:51:40AM +0100, Kieran Bingham wrote: > Quoting Umang Jain via libcamera-devel (2022-09-05 07:41:50) > > On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: > > > On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: > > >> Compiling with tests adds a significant amount of build targets to the > > >> meson build. However, tests are only of interest when a feature has been > > >> implemented or for continuous integration. > > >> Disable tests by default to reduce the computational resource requirements > > >> and the duration of a complete libcamera built. > > >> > > >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > > As most people building libcamera won't run tests, this seems reasonable > > > to me. I'm however a bit worried that it would lead to lower > > > compilation-test coverage, as people may send patches without realizing > > > they break compilation of tests. We already have that problem for > > > > I'm concerned about this as well. > > When I first saw this patch over the weekend, indeed my reaction was > "Ahhh" ... but I also agree this is a better default to have off right > now. > > We would like patches to be submitted after having all tests run, but > the majority of people who compile libcamera (currently) may not be > developers. We have lots of people compiling on RPi / IPU3 and low power > devices directly, and they certainly don't care for running tests. > > So to improve user experience, I do think this should be off by default > (at least now) and that stance may change depending on our upcoming > release strategies ... but developers should be able to enable tests > anyway. > > Ideally of course these tests will then be enabled and run on any branch > push to the common public hosting services, so I think we should > integrate a gitlab and github CI script that could build and run the > unit tests. (These could skip the kernel related tests initially). Many of our tests indeed depend on vimc, vivid and vim2m, so that's a blocker for git..b. I'd be happy if we started with test runners that performed compile testing only, with all options enabled. > Anyway, > > Yes - > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > runtime breakages of tests, and it's most likely be addressed by test > > > automation on patches submitted upstream, so I think I'm fine with the > > > change. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > >> --- > > >> meson_options.txt | 1 + > > >> 1 file changed, 1 insertion(+) > > >> > > >> diff --git a/meson_options.txt b/meson_options.txt > > >> index 7a9aecfc..f1d67808 100644 > > >> --- a/meson_options.txt > > >> +++ b/meson_options.txt > > >> @@ -47,6 +47,7 @@ option('qcam', > > >> > > >> option('test', > > >> type : 'boolean', > > >> + value : false, > > >> description: 'Compile and include the tests') > > >> > > >> option('tracing',
Quoting Laurent Pinchart (2022-09-05 11:35:12) > On Mon, Sep 05, 2022 at 10:51:40AM +0100, Kieran Bingham wrote: > > Quoting Umang Jain via libcamera-devel (2022-09-05 07:41:50) > > > On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: > > > > On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: > > > >> Compiling with tests adds a significant amount of build targets to the > > > >> meson build. However, tests are only of interest when a feature has been > > > >> implemented or for continuous integration. > > > >> Disable tests by default to reduce the computational resource requirements > > > >> and the duration of a complete libcamera built. > > > >> > > > >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > > > As most people building libcamera won't run tests, this seems reasonable > > > > to me. I'm however a bit worried that it would lead to lower > > > > compilation-test coverage, as people may send patches without realizing > > > > they break compilation of tests. We already have that problem for > > > > > > I'm concerned about this as well. > > > > When I first saw this patch over the weekend, indeed my reaction was > > "Ahhh" ... but I also agree this is a better default to have off right > > now. > > > > We would like patches to be submitted after having all tests run, but > > the majority of people who compile libcamera (currently) may not be > > developers. We have lots of people compiling on RPi / IPU3 and low power > > devices directly, and they certainly don't care for running tests. > > > > So to improve user experience, I do think this should be off by default > > (at least now) and that stance may change depending on our upcoming > > release strategies ... but developers should be able to enable tests > > anyway. > > > > Ideally of course these tests will then be enabled and run on any branch > > push to the common public hosting services, so I think we should > > integrate a gitlab and github CI script that could build and run the > > unit tests. (These could skip the kernel related tests initially). > > Many of our tests indeed depend on vimc, vivid and vim2m, so that's a > blocker for git..b. I'd be happy if we started with test runners that > performed compile testing only, with all options enabled. For that I'd really like to continue to investigate (or for anyone else to continue to investigate) running the tests under a compiled kernel in either UML (which seemed to be a deadend for linux-media driver support due to large rabbit holes) - or as a microvm, with a custom minimal kernel that contains VIMC/VIVID/VIM2M. It's something I started to research, but haven't been able to complete, but I beleive would be a way of fully automating the tests with the virtual drivers. If anyone wants to explore this, I'm happy to share where I got to on it. -- Kieran > > > Anyway, > > > > Yes - > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > runtime breakages of tests, and it's most likely be addressed by test > > > > automation on patches submitted upstream, so I think I'm fine with the > > > > change. > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > >> --- > > > >> meson_options.txt | 1 + > > > >> 1 file changed, 1 insertion(+) > > > >> > > > >> diff --git a/meson_options.txt b/meson_options.txt > > > >> index 7a9aecfc..f1d67808 100644 > > > >> --- a/meson_options.txt > > > >> +++ b/meson_options.txt > > > >> @@ -47,6 +47,7 @@ option('qcam', > > > >> > > > >> option('test', > > > >> type : 'boolean', > > > >> + value : false, > > > >> description: 'Compile and include the tests') > > > >> > > > >> option('tracing', > > -- > Regards, > > Laurent Pinchart
Hi Kieran, Am 05.09.22 um 11:51 schrieb Kieran Bingham: > Hi All, > > Quoting Umang Jain via libcamera-devel (2022-09-05 07:41:50) >> Hi Christian, >> >> Thank you for the patch. >> >> On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: >>> Hi Christian, >>> >>> Thank you for the patch. >>> >>> On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: >>>> Compiling with tests adds a significant amount of build targets to the >>>> meson build. However, tests are only of interest when a feature has been >>>> implemented or for continuous integration. >>>> Disable tests by default to reduce the computational resource requirements >>>> and the duration of a complete libcamera built. >>>> >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>> As most people building libcamera won't run tests, this seems reasonable >>> to me. I'm however a bit worried that it would lead to lower >>> compilation-test coverage, as people may send patches without realizing >>> they break compilation of tests. We already have that problem for >> >> I'm concerned about this as well. > > When I first saw this patch over the weekend, indeed my reaction was > "Ahhh" ... but I also agree this is a better default to have off right > now. > > We would like patches to be submitted after having all tests run, but > the majority of people who compile libcamera (currently) may not be > developers. We have lots of people compiling on RPi / IPU3 and low power > devices directly, and they certainly don't care for running tests. I compile libcamera more often than I submit patches. I also compile on a Raspberry Pi Zero 2. So compiling the tests is a huge waste of resources for me. > So to improve user experience, I do think this should be off by default > (at least now) and that stance may change depending on our upcoming > release strategies ... but developers should be able to enable tests > anyway. I would argue that tests should only be enabled when it is intended to run them. > Ideally of course these tests will then be enabled and run on any branch > push to the common public hosting services, so I think we should > integrate a gitlab and github CI script that could build and run the > unit tests. (These could skip the kernel related tests initially). I am also used to have tests run automatically on a CI server when I send PRs to other projects. Having some public libcamera CI instance that compiles and tests sent patches would be very helpful. If you intend to use GitHub actions, I can contribute the scripts that I already use for my own purpose. > > Anyway, > > Yes - > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> runtime breakages of tests, and it's most likely be addressed by test >>> automation on patches submitted upstream, so I think I'm fine with the >>> change. >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> >>> >>>> --- >>>> meson_options.txt | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/meson_options.txt b/meson_options.txt >>>> index 7a9aecfc..f1d67808 100644 >>>> --- a/meson_options.txt >>>> +++ b/meson_options.txt >>>> @@ -47,6 +47,7 @@ option('qcam', >>>> >>>> option('test', >>>> type : 'boolean', >>>> + value : false, >>>> description: 'Compile and include the tests') >>>> >>>> option('tracing', >>
Quoting Christian Rauch (2022-09-05 20:26:19) > Hi Kieran, > > Am 05.09.22 um 11:51 schrieb Kieran Bingham: > > Hi All, > > > > Quoting Umang Jain via libcamera-devel (2022-09-05 07:41:50) > >> Hi Christian, > >> > >> Thank you for the patch. > >> > >> On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: > >>> Hi Christian, > >>> > >>> Thank you for the patch. > >>> > >>> On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: > >>>> Compiling with tests adds a significant amount of build targets to the > >>>> meson build. However, tests are only of interest when a feature has been > >>>> implemented or for continuous integration. > >>>> Disable tests by default to reduce the computational resource requirements > >>>> and the duration of a complete libcamera built. > >>>> > >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > >>> As most people building libcamera won't run tests, this seems reasonable > >>> to me. I'm however a bit worried that it would lead to lower > >>> compilation-test coverage, as people may send patches without realizing > >>> they break compilation of tests. We already have that problem for > >> > >> I'm concerned about this as well. > > > > When I first saw this patch over the weekend, indeed my reaction was > > "Ahhh" ... but I also agree this is a better default to have off right > > now. > > > > We would like patches to be submitted after having all tests run, but > > the majority of people who compile libcamera (currently) may not be > > developers. We have lots of people compiling on RPi / IPU3 and low power > > devices directly, and they certainly don't care for running tests. > > I compile libcamera more often than I submit patches. I also compile on > a Raspberry Pi Zero 2. So compiling the tests is a huge waste of > resources for me. Yes, I understand that. If libcamera were 'well packaged, stable, and finished' ... there would be less users having to compile it though. So some time in the <waves hand> future... libcamera should become rarely built, and tests won't be such a burden. Also - compiling on an RPi02 is certainly a pain. Did I share the RPi cross compile environment with you already ? > > So to improve user experience, I do think this should be off by default > > (at least now) and that stance may change depending on our upcoming > > release strategies ... but developers should be able to enable tests > > anyway. > > I would argue that tests should only be enabled when it is intended to > run them. > > > Ideally of course these tests will then be enabled and run on any branch > > push to the common public hosting services, so I think we should > > integrate a gitlab and github CI script that could build and run the > > unit tests. (These could skip the kernel related tests initially). > > I am also used to have tests run automatically on a CI server when I > send PRs to other projects. Having some public libcamera CI instance > that compiles and tests sent patches would be very helpful. > If you intend to use GitHub actions, I can contribute the scripts that I > already use for my own purpose. As mentioned in the thread with Laurent - this is more complex because we need a custom kernel with virtual drivers (as you've experienced). I think this can be solved, but that requires some investigations that I haven't had time for. 'We' won't use github actions, but I think integrating a github actions script to the repository is certainly reasonable, if it helps anyone who pushes a branch to their repository compile test in a consistent manner. If you have something usable for that, please do post it to the list. -- Kieran > > > > > Anyway, > > > > Yes - > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >>> runtime breakages of tests, and it's most likely be addressed by test > >>> automation on patches submitted upstream, so I think I'm fine with the > >>> change. > >>> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> > >>> > >>>> --- > >>>> meson_options.txt | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/meson_options.txt b/meson_options.txt > >>>> index 7a9aecfc..f1d67808 100644 > >>>> --- a/meson_options.txt > >>>> +++ b/meson_options.txt > >>>> @@ -47,6 +47,7 @@ option('qcam', > >>>> > >>>> option('test', > >>>> type : 'boolean', > >>>> + value : false, > >>>> description: 'Compile and include the tests') > >>>> > >>>> option('tracing', > >>
Hi Kieran, Am 05.09.22 um 21:40 schrieb Kieran Bingham: > Quoting Christian Rauch (2022-09-05 20:26:19) >> Hi Kieran, >> >> Am 05.09.22 um 11:51 schrieb Kieran Bingham: >>> Hi All, >>> >>> Quoting Umang Jain via libcamera-devel (2022-09-05 07:41:50) >>>> Hi Christian, >>>> >>>> Thank you for the patch. >>>> >>>> On 9/5/22 3:29 AM, Laurent Pinchart via libcamera-devel wrote: >>>>> Hi Christian, >>>>> >>>>> Thank you for the patch. >>>>> >>>>> On Sun, Sep 04, 2022 at 06:27:15PM +0200, Christian Rauch via libcamera-devel wrote: >>>>>> Compiling with tests adds a significant amount of build targets to the >>>>>> meson build. However, tests are only of interest when a feature has been >>>>>> implemented or for continuous integration. >>>>>> Disable tests by default to reduce the computational resource requirements >>>>>> and the duration of a complete libcamera built. >>>>>> >>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>>>> As most people building libcamera won't run tests, this seems reasonable >>>>> to me. I'm however a bit worried that it would lead to lower >>>>> compilation-test coverage, as people may send patches without realizing >>>>> they break compilation of tests. We already have that problem for >>>> >>>> I'm concerned about this as well. >>> >>> When I first saw this patch over the weekend, indeed my reaction was >>> "Ahhh" ... but I also agree this is a better default to have off right >>> now. >>> >>> We would like patches to be submitted after having all tests run, but >>> the majority of people who compile libcamera (currently) may not be >>> developers. We have lots of people compiling on RPi / IPU3 and low power >>> devices directly, and they certainly don't care for running tests. >> >> I compile libcamera more often than I submit patches. I also compile on >> a Raspberry Pi Zero 2. So compiling the tests is a huge waste of >> resources for me. > > Yes, I understand that. If libcamera were 'well packaged, stable, and > finished' ... there would be less users having to compile it though. > > So some time in the <waves hand> future... libcamera should become > rarely built, and tests won't be such a burden. > > Also - compiling on an RPi02 is certainly a pain. Did I share the RPi > cross compile environment with you already ? > > > >>> So to improve user experience, I do think this should be off by default >>> (at least now) and that stance may change depending on our upcoming >>> release strategies ... but developers should be able to enable tests >>> anyway. >> >> I would argue that tests should only be enabled when it is intended to >> run them. >> >>> Ideally of course these tests will then be enabled and run on any branch >>> push to the common public hosting services, so I think we should >>> integrate a gitlab and github CI script that could build and run the >>> unit tests. (These could skip the kernel related tests initially). >> >> I am also used to have tests run automatically on a CI server when I >> send PRs to other projects. Having some public libcamera CI instance >> that compiles and tests sent patches would be very helpful. >> If you intend to use GitHub actions, I can contribute the scripts that I >> already use for my own purpose. > > As mentioned in the thread with Laurent - this is more complex because > we need a custom kernel with virtual drivers (as you've experienced). > > I think this can be solved, but that requires some investigations that I > haven't had time for. > > 'We' won't use github actions, but I think integrating a github actions > script to the repository is certainly reasonable, if it helps anyone who > pushes a branch to their repository compile test in a consistent manner. > > If you have something usable for that, please do post it to the list. I gave this a shot with the kernel modules on a GitHub runner: https://github.com/christianrauch/libcamera-ci/blob/ci/.github/workflows/main_runner.yml After installing the kernel modules vimc, vim2m and vivid on Ubuntu 22.04, I was able to run some of the tests that require a virtual camera device. But some other tests fail because of timeouts in "ipc_pipe_unixsocket.cpp:134". I think there are still some peculiarities of running in some kind of VM, but testing with a virtual camera in a CI is certainly possible. > > -- > Kieran > > >> >>> >>> Anyway, >>> >>> Yes - >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>>> runtime breakages of tests, and it's most likely be addressed by test >>>>> automation on patches submitted upstream, so I think I'm fine with the >>>>> change. >>>>> >>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> >>>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >>>> >>>>> >>>>>> --- >>>>>> meson_options.txt | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/meson_options.txt b/meson_options.txt >>>>>> index 7a9aecfc..f1d67808 100644 >>>>>> --- a/meson_options.txt >>>>>> +++ b/meson_options.txt >>>>>> @@ -47,6 +47,7 @@ option('qcam', >>>>>> >>>>>> option('test', >>>>>> type : 'boolean', >>>>>> + value : false, >>>>>> description: 'Compile and include the tests') >>>>>> >>>>>> option('tracing', >>>>
diff --git a/meson_options.txt b/meson_options.txt index 7a9aecfc..f1d67808 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -47,6 +47,7 @@ option('qcam', option('test', type : 'boolean', + value : false, description: 'Compile and include the tests') option('tracing',
Compiling with tests adds a significant amount of build targets to the meson build. However, tests are only of interest when a feature has been implemented or for continuous integration. Disable tests by default to reduce the computational resource requirements and the duration of a complete libcamera built. Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> --- meson_options.txt | 1 + 1 file changed, 1 insertion(+) -- 2.34.1