[libcamera-ci,RFC,v1,2/3] Separate the building and running of unit tests
diff mbox series

Message ID 20241212181655.112958-2-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • [RFC,v1,1/3] Enable `UDMABUF` in the kernel
Related show

Commit Message

Barnabás Pőcze Dec. 12, 2024, 6:16 p.m. UTC
The built artifacts will be reused in a later job, so split
the "test-unit" into the "build-test" and "test-unit" jobs.

The `libevent` development package cannot be installed in the container
directly because it is not multiarch compatible. It is, however, installed
in the architecture specific build jobs, right before building. To ensure
that the it is available for already built executables in different jobs,
install just the libraries in the container.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 .gitlab-ci/setup-container.sh |  3 +++
 gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 14 deletions(-)

--
2.47.1

Comments

Laurent Pinchart Dec. 15, 2024, 7:04 p.m. UTC | #1
Hi Barnabás,

Thank you for the patch.

On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> The built artifacts will be reused in a later job, so split
> the "test-unit" into the "build-test" and "test-unit" jobs.
> 
> The `libevent` development package cannot be installed in the container

I've write `libevent-dev` here to avoid ambiguities.

> directly because it is not multiarch compatible. It is, however, installed
> in the architecture specific build jobs, right before building. To ensure
> that the it is available for already built executables in different jobs,

"that the it is" ?

> install just the libraries in the container.

And name here `libevent`.

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .gitlab-ci/setup-container.sh |  3 +++
>  gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
>  2 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> index d2909c7..0658368 100755
> --- a/.gitlab-ci/setup-container.sh
> +++ b/.gitlab-ci/setup-container.sh
> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
>  'bookworm')
>  	# libclang-rt-dev for the clang ASan runtime.
>  	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> +	# For cam and lc-compliance
> +	# libevent-dev cannot be used here, see build-libcamera-common.sh
> +	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
>  	;;
>  'trixie')
>  	# gcc 13 to expand compilation testing coverage.
> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> index 8bc8bc2..c7448b8 100644
> --- a/gitlab-ci.yml
> +++ b/gitlab-ci.yml
> @@ -64,7 +64,7 @@ include:
>  .libcamera-ci.debian:12:
>    variables:
>      FDO_DISTRIBUTION_VERSION: 'bookworm'
> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> 
>  .libcamera-ci.debian:13:
>    variables:
> @@ -363,28 +363,18 @@ test-soraka:
>    script:
>      - submit .gitlab-ci/lava/soraka-camera-test.yml
> 
> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> -# the unit tests.
> -test-unit:
> +# Enable only the options exercised by the unit tests.
> +build-test:debug:

I'd call this build-package:amd64, as we have build-package:arm64 and
build-package:cros. I think it would also make sense to use the same
build options for the amd64 and arm64 packages (beside possibly the
selected pipeline handlers, although the 'auto' option may work for
both).

>    extends:
>      - .fdo.distribution-image@debian
>      - .libcamera-ci.debian:12
>      - .libcamera-ci.scripts
> -  stage: test
> +  stage: build
>    needs:
>      - job: container-debian:12
>        artifacts: false
> -  tags:
> -    - kvm
>    script:
>      - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> -  artifacts:
> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> -    when: always
> -    expire_in: 1 week
> -    paths:
> -      - build/meson-logs/
>    variables:
>      BUILD_TYPE: debug
>      MESON_OPTIONS: >-
> @@ -399,6 +389,30 @@ test-unit:
>        -D qcam=disabled
>        -D test=true
>        -D v4l2=true
> +  artifacts:
> +    paths:
> +      - build/

The whole build directory can be very large. Can't we do the same as
build-package:arm64 and run package-libcamera.sh to only package what we
need ? We'll need probably need an unpackage script for the test-unit
job.

> +    expire_in: 1 day
> +
> +# Run the unit tests in a virtual machine.
> +test-unit:
> +  extends:
> +    - .fdo.distribution-image@debian
> +    - .libcamera-ci.debian:12
> +    - .libcamera-ci.scripts
> +  stage: test
> +  needs:
> +    - job: build-test:debug
> +  tags:
> +    - kvm
> +  script:
> +    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> +  artifacts:
> +    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> +    when: always
> +    expire_in: 1 week
> +    paths:
> +      - build/meson-logs/
> 
>    # meson prior to 1.2.0 doesn't correctly escape non-printable characters
>    # when generating the testlog XML. This results in an unparseable file.
Laurent Pinchart Dec. 15, 2024, 7:43 p.m. UTC | #2
On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> > The built artifacts will be reused in a later job, so split
> > the "test-unit" into the "build-test" and "test-unit" jobs.
> > 
> > The `libevent` development package cannot be installed in the container
> 
> I've write `libevent-dev` here to avoid ambiguities.
> 
> > directly because it is not multiarch compatible. It is, however, installed
> > in the architecture specific build jobs, right before building. To ensure
> > that the it is available for already built executables in different jobs,
> 
> "that the it is" ?
> 
> > install just the libraries in the container.
> 
> And name here `libevent`.
> 
> > 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  .gitlab-ci/setup-container.sh |  3 +++
> >  gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> >  2 files changed, 31 insertions(+), 14 deletions(-)
> > 
> > diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> > index d2909c7..0658368 100755
> > --- a/.gitlab-ci/setup-container.sh
> > +++ b/.gitlab-ci/setup-container.sh
> > @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> >  'bookworm')
> >  	# libclang-rt-dev for the clang ASan runtime.
> >  	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> > +	# For cam and lc-compliance
> > +	# libevent-dev cannot be used here, see build-libcamera-common.sh
> > +	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> >  	;;
> >  'trixie')
> >  	# gcc 13 to expand compilation testing coverage.
> > diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > index 8bc8bc2..c7448b8 100644
> > --- a/gitlab-ci.yml
> > +++ b/gitlab-ci.yml
> > @@ -64,7 +64,7 @@ include:
> >  .libcamera-ci.debian:12:
> >    variables:
> >      FDO_DISTRIBUTION_VERSION: 'bookworm'
> > -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> > +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> > 
> >  .libcamera-ci.debian:13:
> >    variables:
> > @@ -363,28 +363,18 @@ test-soraka:
> >    script:
> >      - submit .gitlab-ci/lava/soraka-camera-test.yml
> > 
> > -# Run the unit tests in a virtual machine. Enable only the options exercised by
> > -# the unit tests.
> > -test-unit:
> > +# Enable only the options exercised by the unit tests.
> > +build-test:debug:
> 
> I'd call this build-package:amd64, as we have build-package:arm64 and
> build-package:cros. I think it would also make sense to use the same
> build options for the amd64 and arm64 packages (beside possibly the
> selected pipeline handlers, although the 'auto' option may work for
> both).
> 
> >    extends:
> >      - .fdo.distribution-image@debian
> >      - .libcamera-ci.debian:12
> >      - .libcamera-ci.scripts
> > -  stage: test
> > +  stage: build
> >    needs:
> >      - job: container-debian:12
> >        artifacts: false
> > -  tags:
> > -    - kvm
> >    script:
> >      - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> > -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > -  artifacts:
> > -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > -    when: always
> > -    expire_in: 1 week
> > -    paths:
> > -      - build/meson-logs/
> >    variables:
> >      BUILD_TYPE: debug
> >      MESON_OPTIONS: >-
> > @@ -399,6 +389,30 @@ test-unit:
> >        -D qcam=disabled
> >        -D test=true
> >        -D v4l2=true
> > +  artifacts:
> > +    paths:
> > +      - build/
> 
> The whole build directory can be very large. Can't we do the same as
> build-package:arm64 and run package-libcamera.sh to only package what we
> need ? We'll need probably need an unpackage script for the test-unit
> job.

But of course the unit test binaries don't get installed... Can we fix
that and install them ? You can specify "install_tag : 'tests'" in
meson.build so they won't be installed by default (an appropriate
install_dir is also needed). This in turn requires bumping the minimum
meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.

And now that I've said this, I realize we wouldn't be able to run "meson
test" to run the tests :-/ I'm not sure there's an appropriate solution
for this. If not, given the size of the build directory, and to avoid
transferring a large amount of data between runners, we may need to keep
building libcamera within the test-unit job :-(

A separate build-package target for lc-compliance would still make
sense.

> > +    expire_in: 1 day
> > +
> > +# Run the unit tests in a virtual machine.
> > +test-unit:
> > +  extends:
> > +    - .fdo.distribution-image@debian
> > +    - .libcamera-ci.debian:12
> > +    - .libcamera-ci.scripts
> > +  stage: test
> > +  needs:
> > +    - job: build-test:debug
> > +  tags:
> > +    - kvm
> > +  script:
> > +    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > +  artifacts:
> > +    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > +    when: always
> > +    expire_in: 1 week
> > +    paths:
> > +      - build/meson-logs/
> > 
> >    # meson prior to 1.2.0 doesn't correctly escape non-printable characters
> >    # when generating the testlog XML. This results in an unparseable file.
Laurent Pinchart Dec. 15, 2024, 8:43 p.m. UTC | #3
On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> > Hi Barnabás,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> > > The built artifacts will be reused in a later job, so split
> > > the "test-unit" into the "build-test" and "test-unit" jobs.
> > > 
> > > The `libevent` development package cannot be installed in the container
> > 
> > I've write `libevent-dev` here to avoid ambiguities.
> > 
> > > directly because it is not multiarch compatible. It is, however, installed
> > > in the architecture specific build jobs, right before building. To ensure
> > > that the it is available for already built executables in different jobs,
> > 
> > "that the it is" ?
> > 
> > > install just the libraries in the container.
> > 
> > And name here `libevent`.
> > 
> > > 
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >  .gitlab-ci/setup-container.sh |  3 +++
> > >  gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> > >  2 files changed, 31 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> > > index d2909c7..0658368 100755
> > > --- a/.gitlab-ci/setup-container.sh
> > > +++ b/.gitlab-ci/setup-container.sh
> > > @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> > >  'bookworm')
> > >  	# libclang-rt-dev for the clang ASan runtime.
> > >  	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> > > +	# For cam and lc-compliance
> > > +	# libevent-dev cannot be used here, see build-libcamera-common.sh
> > > +	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> > >  	;;
> > >  'trixie')
> > >  	# gcc 13 to expand compilation testing coverage.
> > > diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > > index 8bc8bc2..c7448b8 100644
> > > --- a/gitlab-ci.yml
> > > +++ b/gitlab-ci.yml
> > > @@ -64,7 +64,7 @@ include:
> > >  .libcamera-ci.debian:12:
> > >    variables:
> > >      FDO_DISTRIBUTION_VERSION: 'bookworm'
> > > -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> > > +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> > > 
> > >  .libcamera-ci.debian:13:
> > >    variables:
> > > @@ -363,28 +363,18 @@ test-soraka:
> > >    script:
> > >      - submit .gitlab-ci/lava/soraka-camera-test.yml
> > > 
> > > -# Run the unit tests in a virtual machine. Enable only the options exercised by
> > > -# the unit tests.
> > > -test-unit:
> > > +# Enable only the options exercised by the unit tests.
> > > +build-test:debug:
> > 
> > I'd call this build-package:amd64, as we have build-package:arm64 and
> > build-package:cros. I think it would also make sense to use the same
> > build options for the amd64 and arm64 packages (beside possibly the
> > selected pipeline handlers, although the 'auto' option may work for
> > both).
> > 
> > >    extends:
> > >      - .fdo.distribution-image@debian
> > >      - .libcamera-ci.debian:12
> > >      - .libcamera-ci.scripts
> > > -  stage: test
> > > +  stage: build
> > >    needs:
> > >      - job: container-debian:12
> > >        artifacts: false
> > > -  tags:
> > > -    - kvm
> > >    script:
> > >      - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> > > -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > > -  artifacts:
> > > -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > > -    when: always
> > > -    expire_in: 1 week
> > > -    paths:
> > > -      - build/meson-logs/
> > >    variables:
> > >      BUILD_TYPE: debug
> > >      MESON_OPTIONS: >-
> > > @@ -399,6 +389,30 @@ test-unit:
> > >        -D qcam=disabled
> > >        -D test=true
> > >        -D v4l2=true
> > > +  artifacts:
> > > +    paths:
> > > +      - build/
> > 
> > The whole build directory can be very large. Can't we do the same as
> > build-package:arm64 and run package-libcamera.sh to only package what we
> > need ? We'll need probably need an unpackage script for the test-unit
> > job.
> 
> But of course the unit test binaries don't get installed... Can we fix
> that and install them ? You can specify "install_tag : 'tests'" in
> meson.build so they won't be installed by default (an appropriate
> install_dir is also needed). This in turn requires bumping the minimum
> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.

I've been told on IRC that the motivation for the "tests" install tag in
meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
think we should switch to a separate runner for unit tests (the pain is
not worth the gain at this point in my opinion), but it could be useful
to tag lc-compliance with install_tag = 'tests'.

> And now that I've said this, I realize we wouldn't be able to run "meson
> test" to run the tests :-/ I'm not sure there's an appropriate solution
> for this. If not, given the size of the build directory, and to avoid
> transferring a large amount of data between runners, we may need to keep
> building libcamera within the test-unit job :-(
> 
> A separate build-package target for lc-compliance would still make
> sense.
> 
> > > +    expire_in: 1 day
> > > +
> > > +# Run the unit tests in a virtual machine.
> > > +test-unit:
> > > +  extends:
> > > +    - .fdo.distribution-image@debian
> > > +    - .libcamera-ci.debian:12
> > > +    - .libcamera-ci.scripts
> > > +  stage: test
> > > +  needs:
> > > +    - job: build-test:debug
> > > +  tags:
> > > +    - kvm
> > > +  script:
> > > +    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > > +  artifacts:
> > > +    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > > +    when: always
> > > +    expire_in: 1 week
> > > +    paths:
> > > +      - build/meson-logs/
> > > 
> > >    # meson prior to 1.2.0 doesn't correctly escape non-printable characters
> > >    # when generating the testlog XML. This results in an unparseable file.
Barnabás Pőcze Dec. 16, 2024, 9:13 a.m. UTC | #4
Hi


2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
>>> Hi Barnabás,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
>>>> The built artifacts will be reused in a later job, so split
>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
>>>>
>>>> The `libevent` development package cannot be installed in the container
>>>
>>> I've write `libevent-dev` here to avoid ambiguities.
>>>
>>>> directly because it is not multiarch compatible. It is, however, installed
>>>> in the architecture specific build jobs, right before building. To ensure
>>>> that the it is available for already built executables in different jobs,
>>>
>>> "that the it is" ?
>>>
>>>> install just the libraries in the container.
>>>
>>> And name here `libevent`.
>>>
>>>>
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>   .gitlab-ci/setup-container.sh |  3 +++
>>>>   gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
>>>>   2 files changed, 31 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
>>>> index d2909c7..0658368 100755
>>>> --- a/.gitlab-ci/setup-container.sh
>>>> +++ b/.gitlab-ci/setup-container.sh
>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
>>>>   'bookworm')
>>>>   	# libclang-rt-dev for the clang ASan runtime.
>>>>   	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
>>>> +	# For cam and lc-compliance
>>>> +	# libevent-dev cannot be used here, see build-libcamera-common.sh
>>>> +	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
>>>>   	;;
>>>>   'trixie')
>>>>   	# gcc 13 to expand compilation testing coverage.
>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
>>>> index 8bc8bc2..c7448b8 100644
>>>> --- a/gitlab-ci.yml
>>>> +++ b/gitlab-ci.yml
>>>> @@ -64,7 +64,7 @@ include:
>>>>   .libcamera-ci.debian:12:
>>>>     variables:
>>>>       FDO_DISTRIBUTION_VERSION: 'bookworm'
>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
>>>>
>>>>   .libcamera-ci.debian:13:
>>>>     variables:
>>>> @@ -363,28 +363,18 @@ test-soraka:
>>>>     script:
>>>>       - submit .gitlab-ci/lava/soraka-camera-test.yml
>>>>
>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
>>>> -# the unit tests.
>>>> -test-unit:
>>>> +# Enable only the options exercised by the unit tests.
>>>> +build-test:debug:
>>>
>>> I'd call this build-package:amd64, as we have build-package:arm64 and
>>> build-package:cros. I think it would also make sense to use the same
>>> build options for the amd64 and arm64 packages (beside possibly the
>>> selected pipeline handlers, although the 'auto' option may work for
>>> both).
>>>
>>>>     extends:
>>>>       - .fdo.distribution-image@debian
>>>>       - .libcamera-ci.debian:12
>>>>       - .libcamera-ci.scripts
>>>> -  stage: test
>>>> +  stage: build
>>>>     needs:
>>>>       - job: container-debian:12
>>>>         artifacts: false
>>>> -  tags:
>>>> -    - kvm
>>>>     script:
>>>>       - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
>>>> -  artifacts:
>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
>>>> -    when: always
>>>> -    expire_in: 1 week
>>>> -    paths:
>>>> -      - build/meson-logs/
>>>>     variables:
>>>>       BUILD_TYPE: debug
>>>>       MESON_OPTIONS: >-
>>>> @@ -399,6 +389,30 @@ test-unit:
>>>>         -D qcam=disabled
>>>>         -D test=true
>>>>         -D v4l2=true
>>>> +  artifacts:
>>>> +    paths:
>>>> +      - build/
>>>
>>> The whole build directory can be very large. Can't we do the same as
>>> build-package:arm64 and run package-libcamera.sh to only package what we
>>> need ? We'll need probably need an unpackage script for the test-unit
>>> job.
>>
>> But of course the unit test binaries don't get installed... Can we fix
>> that and install them ? You can specify "install_tag : 'tests'" in
>> meson.build so they won't be installed by default (an appropriate
>> install_dir is also needed). This in turn requires bumping the minimum
>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> 
> I've been told on IRC that the motivation for the "tests" install tag in
> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> think we should switch to a separate runner for unit tests (the pain is
> not worth the gain at this point in my opinion), but it could be useful
> to tag lc-compliance with install_tag = 'tests'.
> 
>> And now that I've said this, I realize we wouldn't be able to run "meson
>> test" to run the tests :-/ I'm not sure there's an appropriate solution
>> for this. If not, given the size of the build directory, and to avoid
>> transferring a large amount of data between runners, we may need to keep
>> building libcamera within the test-unit job :-(
>>
>> A separate build-package target for lc-compliance would still make
>> sense.

I think it would be unfortunate to give up the usage `meson test` as you
mentioned. I have not noticed that these build artifacts would put any
appreciable strain on the infrastructure. The compressed build directory
comes out to around 167 MiB; I am not sure if I would consider that a
large amount of data. It is definitely cheaper, in terms of time, than
building libcamera twice. Clearing the object files could be another
option. With `artifacts:exclude: build/**/*.o` we can seemingly
remove more than half of the uncompressed size, and about 1/4 of
the compressed size. Does this look acceptable?


Regards,
Barnabás Pőcze


>>
>>>> +    expire_in: 1 day
>>>> +
>>>> +# Run the unit tests in a virtual machine.
>>>> +test-unit:
>>>> +  extends:
>>>> +    - .fdo.distribution-image@debian
>>>> +    - .libcamera-ci.debian:12
>>>> +    - .libcamera-ci.scripts
>>>> +  stage: test
>>>> +  needs:
>>>> +    - job: build-test:debug
>>>> +  tags:
>>>> +    - kvm
>>>> +  script:
>>>> +    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
>>>> +  artifacts:
>>>> +    name: libcamera-unit-tests-${CI_COMMIT_SHA}
>>>> +    when: always
>>>> +    expire_in: 1 week
>>>> +    paths:
>>>> +      - build/meson-logs/
>>>>
>>>>     # meson prior to 1.2.0 doesn't correctly escape non-printable characters
>>>>     # when generating the testlog XML. This results in an unparseable file.
>
Laurent Pinchart Dec. 16, 2024, 11:04 a.m. UTC | #5
On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> > On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> >> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> >>> Hi Barnabás,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> >>>> The built artifacts will be reused in a later job, so split
> >>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> >>>>
> >>>> The `libevent` development package cannot be installed in the container
> >>>
> >>> I've write `libevent-dev` here to avoid ambiguities.
> >>>
> >>>> directly because it is not multiarch compatible. It is, however, installed
> >>>> in the architecture specific build jobs, right before building. To ensure
> >>>> that the it is available for already built executables in different jobs,
> >>>
> >>> "that the it is" ?
> >>>
> >>>> install just the libraries in the container.
> >>>
> >>> And name here `libevent`.
> >>>
> >>>>
> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>> ---
> >>>>   .gitlab-ci/setup-container.sh |  3 +++
> >>>>   gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> >>>>   2 files changed, 31 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> >>>> index d2909c7..0658368 100755
> >>>> --- a/.gitlab-ci/setup-container.sh
> >>>> +++ b/.gitlab-ci/setup-container.sh
> >>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> >>>>   'bookworm')
> >>>>   	# libclang-rt-dev for the clang ASan runtime.
> >>>>   	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> >>>> +	# For cam and lc-compliance
> >>>> +	# libevent-dev cannot be used here, see build-libcamera-common.sh
> >>>> +	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> >>>>   	;;
> >>>>   'trixie')
> >>>>   	# gcc 13 to expand compilation testing coverage.
> >>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> >>>> index 8bc8bc2..c7448b8 100644
> >>>> --- a/gitlab-ci.yml
> >>>> +++ b/gitlab-ci.yml
> >>>> @@ -64,7 +64,7 @@ include:
> >>>>   .libcamera-ci.debian:12:
> >>>>     variables:
> >>>>       FDO_DISTRIBUTION_VERSION: 'bookworm'
> >>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> >>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> >>>>
> >>>>   .libcamera-ci.debian:13:
> >>>>     variables:
> >>>> @@ -363,28 +363,18 @@ test-soraka:
> >>>>     script:
> >>>>       - submit .gitlab-ci/lava/soraka-camera-test.yml
> >>>>
> >>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> >>>> -# the unit tests.
> >>>> -test-unit:
> >>>> +# Enable only the options exercised by the unit tests.
> >>>> +build-test:debug:
> >>>
> >>> I'd call this build-package:amd64, as we have build-package:arm64 and
> >>> build-package:cros. I think it would also make sense to use the same
> >>> build options for the amd64 and arm64 packages (beside possibly the
> >>> selected pipeline handlers, although the 'auto' option may work for
> >>> both).
> >>>
> >>>>     extends:
> >>>>       - .fdo.distribution-image@debian
> >>>>       - .libcamera-ci.debian:12
> >>>>       - .libcamera-ci.scripts
> >>>> -  stage: test
> >>>> +  stage: build
> >>>>     needs:
> >>>>       - job: container-debian:12
> >>>>         artifacts: false
> >>>> -  tags:
> >>>> -    - kvm
> >>>>     script:
> >>>>       - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> >>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> >>>> -  artifacts:
> >>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> >>>> -    when: always
> >>>> -    expire_in: 1 week
> >>>> -    paths:
> >>>> -      - build/meson-logs/
> >>>>     variables:
> >>>>       BUILD_TYPE: debug
> >>>>       MESON_OPTIONS: >-
> >>>> @@ -399,6 +389,30 @@ test-unit:
> >>>>         -D qcam=disabled
> >>>>         -D test=true
> >>>>         -D v4l2=true
> >>>> +  artifacts:
> >>>> +    paths:
> >>>> +      - build/
> >>>
> >>> The whole build directory can be very large. Can't we do the same as
> >>> build-package:arm64 and run package-libcamera.sh to only package what we
> >>> need ? We'll need probably need an unpackage script for the test-unit
> >>> job.
> >>
> >> But of course the unit test binaries don't get installed... Can we fix
> >> that and install them ? You can specify "install_tag : 'tests'" in
> >> meson.build so they won't be installed by default (an appropriate
> >> install_dir is also needed). This in turn requires bumping the minimum
> >> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> > 
> > I've been told on IRC that the motivation for the "tests" install tag in
> > meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> > think we should switch to a separate runner for unit tests (the pain is
> > not worth the gain at this point in my opinion), but it could be useful
> > to tag lc-compliance with install_tag = 'tests'.
> > 
> >> And now that I've said this, I realize we wouldn't be able to run "meson
> >> test" to run the tests :-/ I'm not sure there's an appropriate solution
> >> for this. If not, given the size of the build directory, and to avoid
> >> transferring a large amount of data between runners, we may need to keep
> >> building libcamera within the test-unit job :-(
> >>
> >> A separate build-package target for lc-compliance would still make
> >> sense.
> 
> I think it would be unfortunate to give up the usage `meson test` as you
> mentioned.

We could work on a replacement, but it would require a significant
amount of work and I think there are better things to do.

> I have not noticed that these build artifacts would put any
> appreciable strain on the infrastructure. The compressed build directory
> comes out to around 167 MiB; I am not sure if I would consider that a
> large amount of data. It is definitely cheaper, in terms of time, than
> building libcamera twice. Clearing the object files could be another
> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> remove more than half of the uncompressed size, and about 1/4 of
> the compressed size. Does this look acceptable?

Possibly. We should probably ask the fdo sysadmins about what is
acceptable.

I gave it a try locally though, and deleting all *.o files in the build
directory results in "meson test" rebuilding everything.

For other uses of the artifacts (in particular deployment on real
devices), I would still prefer minimizing the bandwidth, creating a
package similarly to what build-package:arm64 does. How about keeping
test-unit as-is, at the cost of a recompilation, and creating a
build-package:amd64 that will be used by the lc-compliance test job ? We
can try to improve on top when/if needed.

> >>>> +    expire_in: 1 day
> >>>> +
> >>>> +# Run the unit tests in a virtual machine.
> >>>> +test-unit:
> >>>> +  extends:
> >>>> +    - .fdo.distribution-image@debian
> >>>> +    - .libcamera-ci.debian:12
> >>>> +    - .libcamera-ci.scripts
> >>>> +  stage: test
> >>>> +  needs:
> >>>> +    - job: build-test:debug
> >>>> +  tags:
> >>>> +    - kvm
> >>>> +  script:
> >>>> +    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> >>>> +  artifacts:
> >>>> +    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> >>>> +    when: always
> >>>> +    expire_in: 1 week
> >>>> +    paths:
> >>>> +      - build/meson-logs/
> >>>>
> >>>>     # meson prior to 1.2.0 doesn't correctly escape non-printable characters
> >>>>     # when generating the testlog XML. This results in an unparseable file.
Barnabás Pőcze Dec. 16, 2024, 5:28 p.m. UTC | #6
2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
>> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
>>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
>>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
>>>>> Hi Barnabás,
>>>>>
>>>>> Thank you for the patch.
>>>>>
>>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
>>>>>> The built artifacts will be reused in a later job, so split
>>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
>>>>>>
>>>>>> The `libevent` development package cannot be installed in the container
>>>>>
>>>>> I've write `libevent-dev` here to avoid ambiguities.
>>>>>
>>>>>> directly because it is not multiarch compatible. It is, however, installed
>>>>>> in the architecture specific build jobs, right before building. To ensure
>>>>>> that the it is available for already built executables in different jobs,
>>>>>
>>>>> "that the it is" ?
>>>>>
>>>>>> install just the libraries in the container.
>>>>>
>>>>> And name here `libevent`.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>>> ---
>>>>>>    .gitlab-ci/setup-container.sh |  3 +++
>>>>>>    gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
>>>>>>    2 files changed, 31 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
>>>>>> index d2909c7..0658368 100755
>>>>>> --- a/.gitlab-ci/setup-container.sh
>>>>>> +++ b/.gitlab-ci/setup-container.sh
>>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
>>>>>>    'bookworm')
>>>>>>    	# libclang-rt-dev for the clang ASan runtime.
>>>>>>    	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
>>>>>> +	# For cam and lc-compliance
>>>>>> +	# libevent-dev cannot be used here, see build-libcamera-common.sh
>>>>>> +	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
>>>>>>    	;;
>>>>>>    'trixie')
>>>>>>    	# gcc 13 to expand compilation testing coverage.
>>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
>>>>>> index 8bc8bc2..c7448b8 100644
>>>>>> --- a/gitlab-ci.yml
>>>>>> +++ b/gitlab-ci.yml
>>>>>> @@ -64,7 +64,7 @@ include:
>>>>>>    .libcamera-ci.debian:12:
>>>>>>      variables:
>>>>>>        FDO_DISTRIBUTION_VERSION: 'bookworm'
>>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
>>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
>>>>>>
>>>>>>    .libcamera-ci.debian:13:
>>>>>>      variables:
>>>>>> @@ -363,28 +363,18 @@ test-soraka:
>>>>>>      script:
>>>>>>        - submit .gitlab-ci/lava/soraka-camera-test.yml
>>>>>>
>>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
>>>>>> -# the unit tests.
>>>>>> -test-unit:
>>>>>> +# Enable only the options exercised by the unit tests.
>>>>>> +build-test:debug:
>>>>>
>>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
>>>>> build-package:cros. I think it would also make sense to use the same
>>>>> build options for the amd64 and arm64 packages (beside possibly the
>>>>> selected pipeline handlers, although the 'auto' option may work for
>>>>> both).
>>>>>
>>>>>>      extends:
>>>>>>        - .fdo.distribution-image@debian
>>>>>>        - .libcamera-ci.debian:12
>>>>>>        - .libcamera-ci.scripts
>>>>>> -  stage: test
>>>>>> +  stage: build
>>>>>>      needs:
>>>>>>        - job: container-debian:12
>>>>>>          artifacts: false
>>>>>> -  tags:
>>>>>> -    - kvm
>>>>>>      script:
>>>>>>        - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
>>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
>>>>>> -  artifacts:
>>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
>>>>>> -    when: always
>>>>>> -    expire_in: 1 week
>>>>>> -    paths:
>>>>>> -      - build/meson-logs/
>>>>>>      variables:
>>>>>>        BUILD_TYPE: debug
>>>>>>        MESON_OPTIONS: >-
>>>>>> @@ -399,6 +389,30 @@ test-unit:
>>>>>>          -D qcam=disabled
>>>>>>          -D test=true
>>>>>>          -D v4l2=true
>>>>>> +  artifacts:
>>>>>> +    paths:
>>>>>> +      - build/
>>>>>
>>>>> The whole build directory can be very large. Can't we do the same as
>>>>> build-package:arm64 and run package-libcamera.sh to only package what we
>>>>> need ? We'll need probably need an unpackage script for the test-unit
>>>>> job.
>>>>
>>>> But of course the unit test binaries don't get installed... Can we fix
>>>> that and install them ? You can specify "install_tag : 'tests'" in
>>>> meson.build so they won't be installed by default (an appropriate
>>>> install_dir is also needed). This in turn requires bumping the minimum
>>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
>>>
>>> I've been told on IRC that the motivation for the "tests" install tag in
>>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
>>> think we should switch to a separate runner for unit tests (the pain is
>>> not worth the gain at this point in my opinion), but it could be useful
>>> to tag lc-compliance with install_tag = 'tests'.
>>>
>>>> And now that I've said this, I realize we wouldn't be able to run "meson
>>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
>>>> for this. If not, given the size of the build directory, and to avoid
>>>> transferring a large amount of data between runners, we may need to keep
>>>> building libcamera within the test-unit job :-(
>>>>
>>>> A separate build-package target for lc-compliance would still make
>>>> sense.
>>
>> I think it would be unfortunate to give up the usage `meson test` as you
>> mentioned.
> 
> We could work on a replacement, but it would require a significant
> amount of work and I think there are better things to do.
> 
>> I have not noticed that these build artifacts would put any
>> appreciable strain on the infrastructure. The compressed build directory
>> comes out to around 167 MiB; I am not sure if I would consider that a
>> large amount of data. It is definitely cheaper, in terms of time, than
>> building libcamera twice. Clearing the object files could be another
>> option. With `artifacts:exclude: build/**/*.o` we can seemingly
>> remove more than half of the uncompressed size, and about 1/4 of
>> the compressed size. Does this look acceptable?
> 
> Possibly. We should probably ask the fdo sysadmins about what is
> acceptable.
> 
> I gave it a try locally though, and deleting all *.o files in the build
> directory results in "meson test" rebuilding everything.

As far as I can see that does not happen with the `--no-rebuild` option,
which is already used in `test-libcamera-qemu.sh`.

> 
> For other uses of the artifacts (in particular deployment on real
> devices), I would still prefer minimizing the bandwidth, creating a
> package similarly to what build-package:arm64 does. How about keeping
> test-unit as-is, at the cost of a recompilation, and creating a
> build-package:amd64 that will be used by the lc-compliance test job ? We
> can try to improve on top when/if needed.

Couple observations:

1. The virtual pipeline handler configuration is not installed, so
    that needs to be addressed. (Was this omitted intentionally?)

2. I am not a fan of the extra `tar` and `ldconfig`  calls that
    need to be sprinkled in. I think this would be much better
    if the package was not a mere tar archive but a proper deb/etc.
    package. I imagine that is a prerequisite of deploying on real
    hardware in any case, correct?

3. Ignoring `*.o` makes the sizes a lot closer in my opinion (for one
    particular build: uncompressed 163M vs. 143M, compressed 28M vs. 15M
    [probably largely due to the different compression schemes]).


In any case, please see the new version of these changes.


Regards,
Barnabás Pőcze


> 
>>>>>> +    expire_in: 1 day
>>>>>> +
>>>>>> +# Run the unit tests in a virtual machine.
>>>>>> +test-unit:
>>>>>> +  extends:
>>>>>> +    - .fdo.distribution-image@debian
>>>>>> +    - .libcamera-ci.debian:12
>>>>>> +    - .libcamera-ci.scripts
>>>>>> +  stage: test
>>>>>> +  needs:
>>>>>> +    - job: build-test:debug
>>>>>> +  tags:
>>>>>> +    - kvm
>>>>>> +  script:
>>>>>> +    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
>>>>>> +  artifacts:
>>>>>> +    name: libcamera-unit-tests-${CI_COMMIT_SHA}
>>>>>> +    when: always
>>>>>> +    expire_in: 1 week
>>>>>> +    paths:
>>>>>> +      - build/meson-logs/
>>>>>>
>>>>>>      # meson prior to 1.2.0 doesn't correctly escape non-printable characters
>>>>>>      # when generating the testlog XML. This results in an unparseable file.
>
Kieran Bingham Dec. 16, 2024, 6:26 p.m. UTC | #7
Quoting Barnabás Pőcze (2024-12-16 17:28:46)
> 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> > On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> >> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> >>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> >>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> >>>>> Hi Barnabás,
> >>>>>
> >>>>> Thank you for the patch.
> >>>>>
> >>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> >>>>>> The built artifacts will be reused in a later job, so split
> >>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> >>>>>>
> >>>>>> The `libevent` development package cannot be installed in the container
> >>>>>
> >>>>> I've write `libevent-dev` here to avoid ambiguities.
> >>>>>
> >>>>>> directly because it is not multiarch compatible. It is, however, installed
> >>>>>> in the architecture specific build jobs, right before building. To ensure
> >>>>>> that the it is available for already built executables in different jobs,
> >>>>>
> >>>>> "that the it is" ?
> >>>>>
> >>>>>> install just the libraries in the container.
> >>>>>
> >>>>> And name here `libevent`.
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>>>> ---
> >>>>>>    .gitlab-ci/setup-container.sh |  3 +++
> >>>>>>    gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> >>>>>>    2 files changed, 31 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> >>>>>> index d2909c7..0658368 100755
> >>>>>> --- a/.gitlab-ci/setup-container.sh
> >>>>>> +++ b/.gitlab-ci/setup-container.sh
> >>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> >>>>>>    'bookworm')
> >>>>>>          # libclang-rt-dev for the clang ASan runtime.
> >>>>>>          PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> >>>>>> +        # For cam and lc-compliance
> >>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
> >>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> >>>>>>          ;;
> >>>>>>    'trixie')
> >>>>>>          # gcc 13 to expand compilation testing coverage.
> >>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> >>>>>> index 8bc8bc2..c7448b8 100644
> >>>>>> --- a/gitlab-ci.yml
> >>>>>> +++ b/gitlab-ci.yml
> >>>>>> @@ -64,7 +64,7 @@ include:
> >>>>>>    .libcamera-ci.debian:12:
> >>>>>>      variables:
> >>>>>>        FDO_DISTRIBUTION_VERSION: 'bookworm'
> >>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> >>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> >>>>>>
> >>>>>>    .libcamera-ci.debian:13:
> >>>>>>      variables:
> >>>>>> @@ -363,28 +363,18 @@ test-soraka:
> >>>>>>      script:
> >>>>>>        - submit .gitlab-ci/lava/soraka-camera-test.yml
> >>>>>>
> >>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> >>>>>> -# the unit tests.
> >>>>>> -test-unit:
> >>>>>> +# Enable only the options exercised by the unit tests.
> >>>>>> +build-test:debug:
> >>>>>
> >>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
> >>>>> build-package:cros. I think it would also make sense to use the same
> >>>>> build options for the amd64 and arm64 packages (beside possibly the
> >>>>> selected pipeline handlers, although the 'auto' option may work for
> >>>>> both).
> >>>>>
> >>>>>>      extends:
> >>>>>>        - .fdo.distribution-image@debian
> >>>>>>        - .libcamera-ci.debian:12
> >>>>>>        - .libcamera-ci.scripts
> >>>>>> -  stage: test
> >>>>>> +  stage: build
> >>>>>>      needs:
> >>>>>>        - job: container-debian:12
> >>>>>>          artifacts: false
> >>>>>> -  tags:
> >>>>>> -    - kvm
> >>>>>>      script:
> >>>>>>        - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> >>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> >>>>>> -  artifacts:
> >>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> >>>>>> -    when: always
> >>>>>> -    expire_in: 1 week
> >>>>>> -    paths:
> >>>>>> -      - build/meson-logs/
> >>>>>>      variables:
> >>>>>>        BUILD_TYPE: debug
> >>>>>>        MESON_OPTIONS: >-
> >>>>>> @@ -399,6 +389,30 @@ test-unit:
> >>>>>>          -D qcam=disabled
> >>>>>>          -D test=true
> >>>>>>          -D v4l2=true
> >>>>>> +  artifacts:
> >>>>>> +    paths:
> >>>>>> +      - build/
> >>>>>
> >>>>> The whole build directory can be very large. Can't we do the same as
> >>>>> build-package:arm64 and run package-libcamera.sh to only package what we
> >>>>> need ? We'll need probably need an unpackage script for the test-unit
> >>>>> job.
> >>>>
> >>>> But of course the unit test binaries don't get installed... Can we fix
> >>>> that and install them ? You can specify "install_tag : 'tests'" in
> >>>> meson.build so they won't be installed by default (an appropriate
> >>>> install_dir is also needed). This in turn requires bumping the minimum
> >>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> >>>
> >>> I've been told on IRC that the motivation for the "tests" install tag in
> >>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> >>> think we should switch to a separate runner for unit tests (the pain is
> >>> not worth the gain at this point in my opinion), but it could be useful
> >>> to tag lc-compliance with install_tag = 'tests'.
> >>>
> >>>> And now that I've said this, I realize we wouldn't be able to run "meson
> >>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
> >>>> for this. If not, given the size of the build directory, and to avoid
> >>>> transferring a large amount of data between runners, we may need to keep
> >>>> building libcamera within the test-unit job :-(
> >>>>
> >>>> A separate build-package target for lc-compliance would still make
> >>>> sense.
> >>
> >> I think it would be unfortunate to give up the usage `meson test` as you
> >> mentioned.
> > 
> > We could work on a replacement, but it would require a significant
> > amount of work and I think there are better things to do.
> > 
> >> I have not noticed that these build artifacts would put any
> >> appreciable strain on the infrastructure. The compressed build directory
> >> comes out to around 167 MiB; I am not sure if I would consider that a
> >> large amount of data. It is definitely cheaper, in terms of time, than
> >> building libcamera twice. Clearing the object files could be another
> >> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> >> remove more than half of the uncompressed size, and about 1/4 of
> >> the compressed size. Does this look acceptable?
> > 
> > Possibly. We should probably ask the fdo sysadmins about what is
> > acceptable.
> > 
> > I gave it a try locally though, and deleting all *.o files in the build
> > directory results in "meson test" rebuilding everything.
> 
> As far as I can see that does not happen with the `--no-rebuild` option,
> which is already used in `test-libcamera-qemu.sh`.
> 
> > 
> > For other uses of the artifacts (in particular deployment on real
> > devices), I would still prefer minimizing the bandwidth, creating a
> > package similarly to what build-package:arm64 does. How about keeping
> > test-unit as-is, at the cost of a recompilation, and creating a
> > build-package:amd64 that will be used by the lc-compliance test job ? We
> > can try to improve on top when/if needed.
> 
> Couple observations:
> 
> 1. The virtual pipeline handler configuration is not installed, so
>     that needs to be addressed. (Was this omitted intentionally?)
> 
> 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
>     need to be sprinkled in. I think this would be much better
>     if the package was not a mere tar archive but a proper deb/etc.
>     package. I imagine that is a prerequisite of deploying on real
>     hardware in any case, correct?

Having the server build a 'real' deb would be a real benefit IMO, and
indeed could help with installation/set up on real targets for testing
in defined environments.

I've wanted to tackle that for a while but never got time.

--
Kieran
Laurent Pinchart Dec. 16, 2024, 8:01 p.m. UTC | #8
On Mon, Dec 16, 2024 at 06:26:14PM +0000, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2024-12-16 17:28:46)
> > 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> > > On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> > >> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> > >>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> > >>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> > >>>>> Hi Barnabás,
> > >>>>>
> > >>>>> Thank you for the patch.
> > >>>>>
> > >>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> > >>>>>> The built artifacts will be reused in a later job, so split
> > >>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> > >>>>>>
> > >>>>>> The `libevent` development package cannot be installed in the container
> > >>>>>
> > >>>>> I've write `libevent-dev` here to avoid ambiguities.
> > >>>>>
> > >>>>>> directly because it is not multiarch compatible. It is, however, installed
> > >>>>>> in the architecture specific build jobs, right before building. To ensure
> > >>>>>> that the it is available for already built executables in different jobs,
> > >>>>>
> > >>>>> "that the it is" ?
> > >>>>>
> > >>>>>> install just the libraries in the container.
> > >>>>>
> > >>>>> And name here `libevent`.
> > >>>>>
> > >>>>>>
> > >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >>>>>> ---
> > >>>>>>    .gitlab-ci/setup-container.sh |  3 +++
> > >>>>>>    gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> > >>>>>>    2 files changed, 31 insertions(+), 14 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> > >>>>>> index d2909c7..0658368 100755
> > >>>>>> --- a/.gitlab-ci/setup-container.sh
> > >>>>>> +++ b/.gitlab-ci/setup-container.sh
> > >>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> > >>>>>>    'bookworm')
> > >>>>>>          # libclang-rt-dev for the clang ASan runtime.
> > >>>>>>          PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> > >>>>>> +        # For cam and lc-compliance
> > >>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
> > >>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> > >>>>>>          ;;
> > >>>>>>    'trixie')
> > >>>>>>          # gcc 13 to expand compilation testing coverage.
> > >>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > >>>>>> index 8bc8bc2..c7448b8 100644
> > >>>>>> --- a/gitlab-ci.yml
> > >>>>>> +++ b/gitlab-ci.yml
> > >>>>>> @@ -64,7 +64,7 @@ include:
> > >>>>>>    .libcamera-ci.debian:12:
> > >>>>>>      variables:
> > >>>>>>        FDO_DISTRIBUTION_VERSION: 'bookworm'
> > >>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> > >>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> > >>>>>>
> > >>>>>>    .libcamera-ci.debian:13:
> > >>>>>>      variables:
> > >>>>>> @@ -363,28 +363,18 @@ test-soraka:
> > >>>>>>      script:
> > >>>>>>        - submit .gitlab-ci/lava/soraka-camera-test.yml
> > >>>>>>
> > >>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> > >>>>>> -# the unit tests.
> > >>>>>> -test-unit:
> > >>>>>> +# Enable only the options exercised by the unit tests.
> > >>>>>> +build-test:debug:
> > >>>>>
> > >>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
> > >>>>> build-package:cros. I think it would also make sense to use the same
> > >>>>> build options for the amd64 and arm64 packages (beside possibly the
> > >>>>> selected pipeline handlers, although the 'auto' option may work for
> > >>>>> both).
> > >>>>>
> > >>>>>>      extends:
> > >>>>>>        - .fdo.distribution-image@debian
> > >>>>>>        - .libcamera-ci.debian:12
> > >>>>>>        - .libcamera-ci.scripts
> > >>>>>> -  stage: test
> > >>>>>> +  stage: build
> > >>>>>>      needs:
> > >>>>>>        - job: container-debian:12
> > >>>>>>          artifacts: false
> > >>>>>> -  tags:
> > >>>>>> -    - kvm
> > >>>>>>      script:
> > >>>>>>        - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> > >>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > >>>>>> -  artifacts:
> > >>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > >>>>>> -    when: always
> > >>>>>> -    expire_in: 1 week
> > >>>>>> -    paths:
> > >>>>>> -      - build/meson-logs/
> > >>>>>>      variables:
> > >>>>>>        BUILD_TYPE: debug
> > >>>>>>        MESON_OPTIONS: >-
> > >>>>>> @@ -399,6 +389,30 @@ test-unit:
> > >>>>>>          -D qcam=disabled
> > >>>>>>          -D test=true
> > >>>>>>          -D v4l2=true
> > >>>>>> +  artifacts:
> > >>>>>> +    paths:
> > >>>>>> +      - build/
> > >>>>>
> > >>>>> The whole build directory can be very large. Can't we do the same as
> > >>>>> build-package:arm64 and run package-libcamera.sh to only package what we
> > >>>>> need ? We'll need probably need an unpackage script for the test-unit
> > >>>>> job.
> > >>>>
> > >>>> But of course the unit test binaries don't get installed... Can we fix
> > >>>> that and install them ? You can specify "install_tag : 'tests'" in
> > >>>> meson.build so they won't be installed by default (an appropriate
> > >>>> install_dir is also needed). This in turn requires bumping the minimum
> > >>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> > >>>
> > >>> I've been told on IRC that the motivation for the "tests" install tag in
> > >>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> > >>> think we should switch to a separate runner for unit tests (the pain is
> > >>> not worth the gain at this point in my opinion), but it could be useful
> > >>> to tag lc-compliance with install_tag = 'tests'.
> > >>>
> > >>>> And now that I've said this, I realize we wouldn't be able to run "meson
> > >>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
> > >>>> for this. If not, given the size of the build directory, and to avoid
> > >>>> transferring a large amount of data between runners, we may need to keep
> > >>>> building libcamera within the test-unit job :-(
> > >>>>
> > >>>> A separate build-package target for lc-compliance would still make
> > >>>> sense.
> > >>
> > >> I think it would be unfortunate to give up the usage `meson test` as you
> > >> mentioned.
> > > 
> > > We could work on a replacement, but it would require a significant
> > > amount of work and I think there are better things to do.
> > > 
> > >> I have not noticed that these build artifacts would put any
> > >> appreciable strain on the infrastructure. The compressed build directory
> > >> comes out to around 167 MiB; I am not sure if I would consider that a
> > >> large amount of data. It is definitely cheaper, in terms of time, than
> > >> building libcamera twice. Clearing the object files could be another
> > >> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> > >> remove more than half of the uncompressed size, and about 1/4 of
> > >> the compressed size. Does this look acceptable?
> > > 
> > > Possibly. We should probably ask the fdo sysadmins about what is
> > > acceptable.
> > > 
> > > I gave it a try locally though, and deleting all *.o files in the build
> > > directory results in "meson test" rebuilding everything.
> > 
> > As far as I can see that does not happen with the `--no-rebuild` option,
> > which is already used in `test-libcamera-qemu.sh`.

Ah, good point, I missed that.

> > > For other uses of the artifacts (in particular deployment on real
> > > devices), I would still prefer minimizing the bandwidth, creating a
> > > package similarly to what build-package:arm64 does. How about keeping
> > > test-unit as-is, at the cost of a recompilation, and creating a
> > > build-package:amd64 that will be used by the lc-compliance test job ? We
> > > can try to improve on top when/if needed.
> > 
> > Couple observations:
> > 
> > 1. The virtual pipeline handler configuration is not installed, so
> >     that needs to be addressed. (Was this omitted intentionally?)

I don't know. We have to be careful here, we don't want to virtual
pipeline handler to end up being enabled with a default valid
configuration in distributions, otherwise people will all of a sudden
see virtual cameras poluting their devices list.

> > 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
> >     need to be sprinkled in. I think this would be much better
> >     if the package was not a mere tar archive but a proper deb/etc.
> >     package. I imagine that is a prerequisite of deploying on real
> >     hardware in any case, correct?
> 
> Having the server build a 'real' deb would be a real benefit IMO, and
> indeed could help with installation/set up on real targets for testing
> in defined environments.
> 
> I've wanted to tackle that for a while but never got time.

Same, I think it's probably worth it, but it will require some effort
and I didn't have enough time when I implemented build-package:arm64.
Barnabás Pőcze Dec. 17, 2024, 4:09 p.m. UTC | #9
2024. 12. 16. 21:01 keltezéssel, Laurent Pinchart írta:
> On Mon, Dec 16, 2024 at 06:26:14PM +0000, Kieran Bingham wrote:
>> Quoting Barnabás Pőcze (2024-12-16 17:28:46)
>>> 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
>>>> On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
>>>>> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
>>>>>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
>>>>>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
>>>>>>>> Hi Barnabás,
>>>>>>>>
>>>>>>>> Thank you for the patch.
>>>>>>>>
>>>>>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
>>>>>>>>> The built artifacts will be reused in a later job, so split
>>>>>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
>>>>>>>>>
>>>>>>>>> The `libevent` development package cannot be installed in the container
>>>>>>>>
>>>>>>>> I've write `libevent-dev` here to avoid ambiguities.
>>>>>>>>
>>>>>>>>> directly because it is not multiarch compatible. It is, however, installed
>>>>>>>>> in the architecture specific build jobs, right before building. To ensure
>>>>>>>>> that the it is available for already built executables in different jobs,
>>>>>>>>
>>>>>>>> "that the it is" ?
>>>>>>>>
>>>>>>>>> install just the libraries in the container.
>>>>>>>>
>>>>>>>> And name here `libevent`.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>>>>>> ---
>>>>>>>>>     .gitlab-ci/setup-container.sh |  3 +++
>>>>>>>>>     gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
>>>>>>>>>     2 files changed, 31 insertions(+), 14 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
>>>>>>>>> index d2909c7..0658368 100755
>>>>>>>>> --- a/.gitlab-ci/setup-container.sh
>>>>>>>>> +++ b/.gitlab-ci/setup-container.sh
>>>>>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
>>>>>>>>>     'bookworm')
>>>>>>>>>           # libclang-rt-dev for the clang ASan runtime.
>>>>>>>>>           PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
>>>>>>>>> +        # For cam and lc-compliance
>>>>>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
>>>>>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
>>>>>>>>>           ;;
>>>>>>>>>     'trixie')
>>>>>>>>>           # gcc 13 to expand compilation testing coverage.
>>>>>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
>>>>>>>>> index 8bc8bc2..c7448b8 100644
>>>>>>>>> --- a/gitlab-ci.yml
>>>>>>>>> +++ b/gitlab-ci.yml
>>>>>>>>> @@ -64,7 +64,7 @@ include:
>>>>>>>>>     .libcamera-ci.debian:12:
>>>>>>>>>       variables:
>>>>>>>>>         FDO_DISTRIBUTION_VERSION: 'bookworm'
>>>>>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
>>>>>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
>>>>>>>>>
>>>>>>>>>     .libcamera-ci.debian:13:
>>>>>>>>>       variables:
>>>>>>>>> @@ -363,28 +363,18 @@ test-soraka:
>>>>>>>>>       script:
>>>>>>>>>         - submit .gitlab-ci/lava/soraka-camera-test.yml
>>>>>>>>>
>>>>>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
>>>>>>>>> -# the unit tests.
>>>>>>>>> -test-unit:
>>>>>>>>> +# Enable only the options exercised by the unit tests.
>>>>>>>>> +build-test:debug:
>>>>>>>>
>>>>>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
>>>>>>>> build-package:cros. I think it would also make sense to use the same
>>>>>>>> build options for the amd64 and arm64 packages (beside possibly the
>>>>>>>> selected pipeline handlers, although the 'auto' option may work for
>>>>>>>> both).
>>>>>>>>
>>>>>>>>>       extends:
>>>>>>>>>         - .fdo.distribution-image@debian
>>>>>>>>>         - .libcamera-ci.debian:12
>>>>>>>>>         - .libcamera-ci.scripts
>>>>>>>>> -  stage: test
>>>>>>>>> +  stage: build
>>>>>>>>>       needs:
>>>>>>>>>         - job: container-debian:12
>>>>>>>>>           artifacts: false
>>>>>>>>> -  tags:
>>>>>>>>> -    - kvm
>>>>>>>>>       script:
>>>>>>>>>         - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
>>>>>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
>>>>>>>>> -  artifacts:
>>>>>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
>>>>>>>>> -    when: always
>>>>>>>>> -    expire_in: 1 week
>>>>>>>>> -    paths:
>>>>>>>>> -      - build/meson-logs/
>>>>>>>>>       variables:
>>>>>>>>>         BUILD_TYPE: debug
>>>>>>>>>         MESON_OPTIONS: >-
>>>>>>>>> @@ -399,6 +389,30 @@ test-unit:
>>>>>>>>>           -D qcam=disabled
>>>>>>>>>           -D test=true
>>>>>>>>>           -D v4l2=true
>>>>>>>>> +  artifacts:
>>>>>>>>> +    paths:
>>>>>>>>> +      - build/
>>>>>>>>
>>>>>>>> The whole build directory can be very large. Can't we do the same as
>>>>>>>> build-package:arm64 and run package-libcamera.sh to only package what we
>>>>>>>> need ? We'll need probably need an unpackage script for the test-unit
>>>>>>>> job.
>>>>>>>
>>>>>>> But of course the unit test binaries don't get installed... Can we fix
>>>>>>> that and install them ? You can specify "install_tag : 'tests'" in
>>>>>>> meson.build so they won't be installed by default (an appropriate
>>>>>>> install_dir is also needed). This in turn requires bumping the minimum
>>>>>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
>>>>>>
>>>>>> I've been told on IRC that the motivation for the "tests" install tag in
>>>>>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
>>>>>> think we should switch to a separate runner for unit tests (the pain is
>>>>>> not worth the gain at this point in my opinion), but it could be useful
>>>>>> to tag lc-compliance with install_tag = 'tests'.
>>>>>>
>>>>>>> And now that I've said this, I realize we wouldn't be able to run "meson
>>>>>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
>>>>>>> for this. If not, given the size of the build directory, and to avoid
>>>>>>> transferring a large amount of data between runners, we may need to keep
>>>>>>> building libcamera within the test-unit job :-(
>>>>>>>
>>>>>>> A separate build-package target for lc-compliance would still make
>>>>>>> sense.
>>>>>
>>>>> I think it would be unfortunate to give up the usage `meson test` as you
>>>>> mentioned.
>>>>
>>>> We could work on a replacement, but it would require a significant
>>>> amount of work and I think there are better things to do.
>>>>
>>>>> I have not noticed that these build artifacts would put any
>>>>> appreciable strain on the infrastructure. The compressed build directory
>>>>> comes out to around 167 MiB; I am not sure if I would consider that a
>>>>> large amount of data. It is definitely cheaper, in terms of time, than
>>>>> building libcamera twice. Clearing the object files could be another
>>>>> option. With `artifacts:exclude: build/**/*.o` we can seemingly
>>>>> remove more than half of the uncompressed size, and about 1/4 of
>>>>> the compressed size. Does this look acceptable?
>>>>
>>>> Possibly. We should probably ask the fdo sysadmins about what is
>>>> acceptable.
>>>>
>>>> I gave it a try locally though, and deleting all *.o files in the build
>>>> directory results in "meson test" rebuilding everything.
>>>
>>> As far as I can see that does not happen with the `--no-rebuild` option,
>>> which is already used in `test-libcamera-qemu.sh`.
> 
> Ah, good point, I missed that.
> 
>>>> For other uses of the artifacts (in particular deployment on real
>>>> devices), I would still prefer minimizing the bandwidth, creating a
>>>> package similarly to what build-package:arm64 does. How about keeping
>>>> test-unit as-is, at the cost of a recompilation, and creating a
>>>> build-package:amd64 that will be used by the lc-compliance test job ? We
>>>> can try to improve on top when/if needed.
>>>
>>> Couple observations:
>>>
>>> 1. The virtual pipeline handler configuration is not installed, so
>>>      that needs to be addressed. (Was this omitted intentionally?)
> 
> I don't know. We have to be careful here, we don't want to virtual
> pipeline handler to end up being enabled with a default valid
> configuration in distributions, otherwise people will all of a sudden
> see virtual cameras poluting their devices list.

I am not sure what should be done. Maybe a separate meson `install_tag`.
Or I suppose the configuration file could be created right before
running it?


> 
>>> 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
>>>      need to be sprinkled in. I think this would be much better
>>>      if the package was not a mere tar archive but a proper deb/etc.
>>>      package. I imagine that is a prerequisite of deploying on real
>>>      hardware in any case, correct?
>>
>> Having the server build a 'real' deb would be a real benefit IMO, and
>> indeed could help with installation/set up on real targets for testing
>> in defined environments.
>>
>> I've wanted to tackle that for a while but never got time.
> 
> Same, I think it's probably worth it, but it will require some effort
> and I didn't have enough time when I implemented build-package:arm64.
>
Laurent Pinchart Dec. 17, 2024, 5:24 p.m. UTC | #10
Hi Barnabás,

On Tue, Dec 17, 2024 at 05:09:45PM +0100, Barnabás Pőcze wrote:
> 2024. 12. 16. 21:01 keltezéssel, Laurent Pinchart írta:
> > On Mon, Dec 16, 2024 at 06:26:14PM +0000, Kieran Bingham wrote:
> >> Quoting Barnabás Pőcze (2024-12-16 17:28:46)
> >>> 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> >>>> On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> >>>>> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> >>>>>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> >>>>>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> >>>>>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> >>>>>>>>> The built artifacts will be reused in a later job, so split
> >>>>>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> >>>>>>>>>
> >>>>>>>>> The `libevent` development package cannot be installed in the container
> >>>>>>>>
> >>>>>>>> I've write `libevent-dev` here to avoid ambiguities.
> >>>>>>>>
> >>>>>>>>> directly because it is not multiarch compatible. It is, however, installed
> >>>>>>>>> in the architecture specific build jobs, right before building. To ensure
> >>>>>>>>> that the it is available for already built executables in different jobs,
> >>>>>>>>
> >>>>>>>> "that the it is" ?
> >>>>>>>>
> >>>>>>>>> install just the libraries in the container.
> >>>>>>>>
> >>>>>>>> And name here `libevent`.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>>>>>>> ---
> >>>>>>>>>     .gitlab-ci/setup-container.sh |  3 +++
> >>>>>>>>>     gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> >>>>>>>>>     2 files changed, 31 insertions(+), 14 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> >>>>>>>>> index d2909c7..0658368 100755
> >>>>>>>>> --- a/.gitlab-ci/setup-container.sh
> >>>>>>>>> +++ b/.gitlab-ci/setup-container.sh
> >>>>>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> >>>>>>>>>     'bookworm')
> >>>>>>>>>           # libclang-rt-dev for the clang ASan runtime.
> >>>>>>>>>           PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> >>>>>>>>> +        # For cam and lc-compliance
> >>>>>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
> >>>>>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> >>>>>>>>>           ;;
> >>>>>>>>>     'trixie')
> >>>>>>>>>           # gcc 13 to expand compilation testing coverage.
> >>>>>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> >>>>>>>>> index 8bc8bc2..c7448b8 100644
> >>>>>>>>> --- a/gitlab-ci.yml
> >>>>>>>>> +++ b/gitlab-ci.yml
> >>>>>>>>> @@ -64,7 +64,7 @@ include:
> >>>>>>>>>     .libcamera-ci.debian:12:
> >>>>>>>>>       variables:
> >>>>>>>>>         FDO_DISTRIBUTION_VERSION: 'bookworm'
> >>>>>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> >>>>>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> >>>>>>>>>
> >>>>>>>>>     .libcamera-ci.debian:13:
> >>>>>>>>>       variables:
> >>>>>>>>> @@ -363,28 +363,18 @@ test-soraka:
> >>>>>>>>>       script:
> >>>>>>>>>         - submit .gitlab-ci/lava/soraka-camera-test.yml
> >>>>>>>>>
> >>>>>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> >>>>>>>>> -# the unit tests.
> >>>>>>>>> -test-unit:
> >>>>>>>>> +# Enable only the options exercised by the unit tests.
> >>>>>>>>> +build-test:debug:
> >>>>>>>>
> >>>>>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
> >>>>>>>> build-package:cros. I think it would also make sense to use the same
> >>>>>>>> build options for the amd64 and arm64 packages (beside possibly the
> >>>>>>>> selected pipeline handlers, although the 'auto' option may work for
> >>>>>>>> both).
> >>>>>>>>
> >>>>>>>>>       extends:
> >>>>>>>>>         - .fdo.distribution-image@debian
> >>>>>>>>>         - .libcamera-ci.debian:12
> >>>>>>>>>         - .libcamera-ci.scripts
> >>>>>>>>> -  stage: test
> >>>>>>>>> +  stage: build
> >>>>>>>>>       needs:
> >>>>>>>>>         - job: container-debian:12
> >>>>>>>>>           artifacts: false
> >>>>>>>>> -  tags:
> >>>>>>>>> -    - kvm
> >>>>>>>>>       script:
> >>>>>>>>>         - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> >>>>>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> >>>>>>>>> -  artifacts:
> >>>>>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> >>>>>>>>> -    when: always
> >>>>>>>>> -    expire_in: 1 week
> >>>>>>>>> -    paths:
> >>>>>>>>> -      - build/meson-logs/
> >>>>>>>>>       variables:
> >>>>>>>>>         BUILD_TYPE: debug
> >>>>>>>>>         MESON_OPTIONS: >-
> >>>>>>>>> @@ -399,6 +389,30 @@ test-unit:
> >>>>>>>>>           -D qcam=disabled
> >>>>>>>>>           -D test=true
> >>>>>>>>>           -D v4l2=true
> >>>>>>>>> +  artifacts:
> >>>>>>>>> +    paths:
> >>>>>>>>> +      - build/
> >>>>>>>>
> >>>>>>>> The whole build directory can be very large. Can't we do the same as
> >>>>>>>> build-package:arm64 and run package-libcamera.sh to only package what we
> >>>>>>>> need ? We'll need probably need an unpackage script for the test-unit
> >>>>>>>> job.
> >>>>>>>
> >>>>>>> But of course the unit test binaries don't get installed... Can we fix
> >>>>>>> that and install them ? You can specify "install_tag : 'tests'" in
> >>>>>>> meson.build so they won't be installed by default (an appropriate
> >>>>>>> install_dir is also needed). This in turn requires bumping the minimum
> >>>>>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> >>>>>>
> >>>>>> I've been told on IRC that the motivation for the "tests" install tag in
> >>>>>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> >>>>>> think we should switch to a separate runner for unit tests (the pain is
> >>>>>> not worth the gain at this point in my opinion), but it could be useful
> >>>>>> to tag lc-compliance with install_tag = 'tests'.
> >>>>>>
> >>>>>>> And now that I've said this, I realize we wouldn't be able to run "meson
> >>>>>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
> >>>>>>> for this. If not, given the size of the build directory, and to avoid
> >>>>>>> transferring a large amount of data between runners, we may need to keep
> >>>>>>> building libcamera within the test-unit job :-(
> >>>>>>>
> >>>>>>> A separate build-package target for lc-compliance would still make
> >>>>>>> sense.
> >>>>>
> >>>>> I think it would be unfortunate to give up the usage `meson test` as you
> >>>>> mentioned.
> >>>>
> >>>> We could work on a replacement, but it would require a significant
> >>>> amount of work and I think there are better things to do.
> >>>>
> >>>>> I have not noticed that these build artifacts would put any
> >>>>> appreciable strain on the infrastructure. The compressed build directory
> >>>>> comes out to around 167 MiB; I am not sure if I would consider that a
> >>>>> large amount of data. It is definitely cheaper, in terms of time, than
> >>>>> building libcamera twice. Clearing the object files could be another
> >>>>> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> >>>>> remove more than half of the uncompressed size, and about 1/4 of
> >>>>> the compressed size. Does this look acceptable?
> >>>>
> >>>> Possibly. We should probably ask the fdo sysadmins about what is
> >>>> acceptable.
> >>>>
> >>>> I gave it a try locally though, and deleting all *.o files in the build
> >>>> directory results in "meson test" rebuilding everything.
> >>>
> >>> As far as I can see that does not happen with the `--no-rebuild` option,
> >>> which is already used in `test-libcamera-qemu.sh`.
> > 
> > Ah, good point, I missed that.
> > 
> >>>> For other uses of the artifacts (in particular deployment on real
> >>>> devices), I would still prefer minimizing the bandwidth, creating a
> >>>> package similarly to what build-package:arm64 does. How about keeping
> >>>> test-unit as-is, at the cost of a recompilation, and creating a
> >>>> build-package:amd64 that will be used by the lc-compliance test job ? We
> >>>> can try to improve on top when/if needed.
> >>>
> >>> Couple observations:
> >>>
> >>> 1. The virtual pipeline handler configuration is not installed, so
> >>>      that needs to be addressed. (Was this omitted intentionally?)
> > 
> > I don't know. We have to be careful here, we don't want to virtual
> > pipeline handler to end up being enabled with a default valid
> > configuration in distributions, otherwise people will all of a sudden
> > see virtual cameras poluting their devices list.
> 
> I am not sure what should be done. Maybe a separate meson `install_tag`.
> Or I suppose the configuration file could be created right before
> running it?

Hmmmm... If this is a sample configuration file that we don't want to
install by default in the location where it will be lookup up, how about
installing it as an example in doc_install_dir (e.g.
/usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
to be a fairly common pattern.

You can move the doc_install_dir definition from
Documentation/meson.build to src/meson.build and rename it to
libcamera_docdir.

> >>> 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
> >>>      need to be sprinkled in. I think this would be much better
> >>>      if the package was not a mere tar archive but a proper deb/etc.
> >>>      package. I imagine that is a prerequisite of deploying on real
> >>>      hardware in any case, correct?
> >>
> >> Having the server build a 'real' deb would be a real benefit IMO, and
> >> indeed could help with installation/set up on real targets for testing
> >> in defined environments.
> >>
> >> I've wanted to tackle that for a while but never got time.
> > 
> > Same, I think it's probably worth it, but it will require some effort
> > and I didn't have enough time when I implemented build-package:arm64.
Laurent Pinchart Dec. 17, 2024, 5:32 p.m. UTC | #11
On Tue, Dec 17, 2024 at 07:25:00PM +0200, Laurent Pinchart wrote:
> On Tue, Dec 17, 2024 at 05:09:45PM +0100, Barnabás Pőcze wrote:
> > 2024. 12. 16. 21:01 keltezéssel, Laurent Pinchart írta:
> > > On Mon, Dec 16, 2024 at 06:26:14PM +0000, Kieran Bingham wrote:
> > >> Quoting Barnabás Pőcze (2024-12-16 17:28:46)
> > >>> 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> > >>>> On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> > >>>>> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> > >>>>>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> > >>>>>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> > >>>>>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> > >>>>>>>>> The built artifacts will be reused in a later job, so split
> > >>>>>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> > >>>>>>>>>
> > >>>>>>>>> The `libevent` development package cannot be installed in the container
> > >>>>>>>>
> > >>>>>>>> I've write `libevent-dev` here to avoid ambiguities.
> > >>>>>>>>
> > >>>>>>>>> directly because it is not multiarch compatible. It is, however, installed
> > >>>>>>>>> in the architecture specific build jobs, right before building. To ensure
> > >>>>>>>>> that the it is available for already built executables in different jobs,
> > >>>>>>>>
> > >>>>>>>> "that the it is" ?
> > >>>>>>>>
> > >>>>>>>>> install just the libraries in the container.
> > >>>>>>>>
> > >>>>>>>> And name here `libevent`.
> > >>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >>>>>>>>> ---
> > >>>>>>>>>     .gitlab-ci/setup-container.sh |  3 +++
> > >>>>>>>>>     gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> > >>>>>>>>>     2 files changed, 31 insertions(+), 14 deletions(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> > >>>>>>>>> index d2909c7..0658368 100755
> > >>>>>>>>> --- a/.gitlab-ci/setup-container.sh
> > >>>>>>>>> +++ b/.gitlab-ci/setup-container.sh
> > >>>>>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> > >>>>>>>>>     'bookworm')
> > >>>>>>>>>           # libclang-rt-dev for the clang ASan runtime.
> > >>>>>>>>>           PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> > >>>>>>>>> +        # For cam and lc-compliance
> > >>>>>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
> > >>>>>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> > >>>>>>>>>           ;;
> > >>>>>>>>>     'trixie')
> > >>>>>>>>>           # gcc 13 to expand compilation testing coverage.
> > >>>>>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > >>>>>>>>> index 8bc8bc2..c7448b8 100644
> > >>>>>>>>> --- a/gitlab-ci.yml
> > >>>>>>>>> +++ b/gitlab-ci.yml
> > >>>>>>>>> @@ -64,7 +64,7 @@ include:
> > >>>>>>>>>     .libcamera-ci.debian:12:
> > >>>>>>>>>       variables:
> > >>>>>>>>>         FDO_DISTRIBUTION_VERSION: 'bookworm'
> > >>>>>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> > >>>>>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> > >>>>>>>>>
> > >>>>>>>>>     .libcamera-ci.debian:13:
> > >>>>>>>>>       variables:
> > >>>>>>>>> @@ -363,28 +363,18 @@ test-soraka:
> > >>>>>>>>>       script:
> > >>>>>>>>>         - submit .gitlab-ci/lava/soraka-camera-test.yml
> > >>>>>>>>>
> > >>>>>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> > >>>>>>>>> -# the unit tests.
> > >>>>>>>>> -test-unit:
> > >>>>>>>>> +# Enable only the options exercised by the unit tests.
> > >>>>>>>>> +build-test:debug:
> > >>>>>>>>
> > >>>>>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
> > >>>>>>>> build-package:cros. I think it would also make sense to use the same
> > >>>>>>>> build options for the amd64 and arm64 packages (beside possibly the
> > >>>>>>>> selected pipeline handlers, although the 'auto' option may work for
> > >>>>>>>> both).
> > >>>>>>>>
> > >>>>>>>>>       extends:
> > >>>>>>>>>         - .fdo.distribution-image@debian
> > >>>>>>>>>         - .libcamera-ci.debian:12
> > >>>>>>>>>         - .libcamera-ci.scripts
> > >>>>>>>>> -  stage: test
> > >>>>>>>>> +  stage: build
> > >>>>>>>>>       needs:
> > >>>>>>>>>         - job: container-debian:12
> > >>>>>>>>>           artifacts: false
> > >>>>>>>>> -  tags:
> > >>>>>>>>> -    - kvm
> > >>>>>>>>>       script:
> > >>>>>>>>>         - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> > >>>>>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > >>>>>>>>> -  artifacts:
> > >>>>>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > >>>>>>>>> -    when: always
> > >>>>>>>>> -    expire_in: 1 week
> > >>>>>>>>> -    paths:
> > >>>>>>>>> -      - build/meson-logs/
> > >>>>>>>>>       variables:
> > >>>>>>>>>         BUILD_TYPE: debug
> > >>>>>>>>>         MESON_OPTIONS: >-
> > >>>>>>>>> @@ -399,6 +389,30 @@ test-unit:
> > >>>>>>>>>           -D qcam=disabled
> > >>>>>>>>>           -D test=true
> > >>>>>>>>>           -D v4l2=true
> > >>>>>>>>> +  artifacts:
> > >>>>>>>>> +    paths:
> > >>>>>>>>> +      - build/
> > >>>>>>>>
> > >>>>>>>> The whole build directory can be very large. Can't we do the same as
> > >>>>>>>> build-package:arm64 and run package-libcamera.sh to only package what we
> > >>>>>>>> need ? We'll need probably need an unpackage script for the test-unit
> > >>>>>>>> job.
> > >>>>>>>
> > >>>>>>> But of course the unit test binaries don't get installed... Can we fix
> > >>>>>>> that and install them ? You can specify "install_tag : 'tests'" in
> > >>>>>>> meson.build so they won't be installed by default (an appropriate
> > >>>>>>> install_dir is also needed). This in turn requires bumping the minimum
> > >>>>>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> > >>>>>>
> > >>>>>> I've been told on IRC that the motivation for the "tests" install tag in
> > >>>>>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> > >>>>>> think we should switch to a separate runner for unit tests (the pain is
> > >>>>>> not worth the gain at this point in my opinion), but it could be useful
> > >>>>>> to tag lc-compliance with install_tag = 'tests'.
> > >>>>>>
> > >>>>>>> And now that I've said this, I realize we wouldn't be able to run "meson
> > >>>>>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
> > >>>>>>> for this. If not, given the size of the build directory, and to avoid
> > >>>>>>> transferring a large amount of data between runners, we may need to keep
> > >>>>>>> building libcamera within the test-unit job :-(
> > >>>>>>>
> > >>>>>>> A separate build-package target for lc-compliance would still make
> > >>>>>>> sense.
> > >>>>>
> > >>>>> I think it would be unfortunate to give up the usage `meson test` as you
> > >>>>> mentioned.
> > >>>>
> > >>>> We could work on a replacement, but it would require a significant
> > >>>> amount of work and I think there are better things to do.
> > >>>>
> > >>>>> I have not noticed that these build artifacts would put any
> > >>>>> appreciable strain on the infrastructure. The compressed build directory
> > >>>>> comes out to around 167 MiB; I am not sure if I would consider that a
> > >>>>> large amount of data. It is definitely cheaper, in terms of time, than
> > >>>>> building libcamera twice. Clearing the object files could be another
> > >>>>> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> > >>>>> remove more than half of the uncompressed size, and about 1/4 of
> > >>>>> the compressed size. Does this look acceptable?
> > >>>>
> > >>>> Possibly. We should probably ask the fdo sysadmins about what is
> > >>>> acceptable.
> > >>>>
> > >>>> I gave it a try locally though, and deleting all *.o files in the build
> > >>>> directory results in "meson test" rebuilding everything.
> > >>>
> > >>> As far as I can see that does not happen with the `--no-rebuild` option,
> > >>> which is already used in `test-libcamera-qemu.sh`.
> > > 
> > > Ah, good point, I missed that.
> > > 
> > >>>> For other uses of the artifacts (in particular deployment on real
> > >>>> devices), I would still prefer minimizing the bandwidth, creating a
> > >>>> package similarly to what build-package:arm64 does. How about keeping
> > >>>> test-unit as-is, at the cost of a recompilation, and creating a
> > >>>> build-package:amd64 that will be used by the lc-compliance test job ? We
> > >>>> can try to improve on top when/if needed.
> > >>>
> > >>> Couple observations:
> > >>>
> > >>> 1. The virtual pipeline handler configuration is not installed, so
> > >>>      that needs to be addressed. (Was this omitted intentionally?)
> > > 
> > > I don't know. We have to be careful here, we don't want to virtual
> > > pipeline handler to end up being enabled with a default valid
> > > configuration in distributions, otherwise people will all of a sudden
> > > see virtual cameras poluting their devices list.
> > 
> > I am not sure what should be done. Maybe a separate meson `install_tag`.
> > Or I suppose the configuration file could be created right before
> > running it?
> 
> Hmmmm... If this is a sample configuration file that we don't want to
> install by default in the location where it will be lookup up, how about
> installing it as an example in doc_install_dir (e.g.
> /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
> to be a fairly common pattern.

And then in CI we would take that file and copy it to the expected
location, renaming it to virtual.yaml.

There's also the question of what install_tag to use. Other data files
use 'runtime', but those are installed to their runtime location. 'doc'
could be an option, we can use that even if we disable documentation
build in CI, but it may feel a bit weird, and possibly later install
other types of documentation that we way not want (although I suppose
installation of future "real" documentation could be conditioned by the
documentation meson option).

Another option is to create it from scratch in CI (possibly with the
file just added to the CI repository and copied in the test job).

> You can move the doc_install_dir definition from
> Documentation/meson.build to src/meson.build and rename it to
> libcamera_docdir.
> 
> > >>> 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
> > >>>      need to be sprinkled in. I think this would be much better
> > >>>      if the package was not a mere tar archive but a proper deb/etc.
> > >>>      package. I imagine that is a prerequisite of deploying on real
> > >>>      hardware in any case, correct?
> > >>
> > >> Having the server build a 'real' deb would be a real benefit IMO, and
> > >> indeed could help with installation/set up on real targets for testing
> > >> in defined environments.
> > >>
> > >> I've wanted to tackle that for a while but never got time.
> > > 
> > > Same, I think it's probably worth it, but it will require some effort
> > > and I didn't have enough time when I implemented build-package:arm64.
Kieran Bingham Dec. 17, 2024, 5:38 p.m. UTC | #12
Quoting Laurent Pinchart (2024-12-17 17:24:59)
> Hi Barnabás,
> 
> On Tue, Dec 17, 2024 at 05:09:45PM +0100, Barnabás Pőcze wrote:
> > 2024. 12. 16. 21:01 keltezéssel, Laurent Pinchart írta:
> > > On Mon, Dec 16, 2024 at 06:26:14PM +0000, Kieran Bingham wrote:
> > >> Quoting Barnabás Pőcze (2024-12-16 17:28:46)
> > >>> 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> > >>>> On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> > >>>>> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> > >>>>>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> > >>>>>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> > >>>>>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> > >>>>>>>>> The built artifacts will be reused in a later job, so split
> > >>>>>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> > >>>>>>>>>
> > >>>>>>>>> The `libevent` development package cannot be installed in the container
> > >>>>>>>>
> > >>>>>>>> I've write `libevent-dev` here to avoid ambiguities.
> > >>>>>>>>
> > >>>>>>>>> directly because it is not multiarch compatible. It is, however, installed
> > >>>>>>>>> in the architecture specific build jobs, right before building. To ensure
> > >>>>>>>>> that the it is available for already built executables in different jobs,
> > >>>>>>>>
> > >>>>>>>> "that the it is" ?
> > >>>>>>>>
> > >>>>>>>>> install just the libraries in the container.
> > >>>>>>>>
> > >>>>>>>> And name here `libevent`.
> > >>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >>>>>>>>> ---
> > >>>>>>>>>     .gitlab-ci/setup-container.sh |  3 +++
> > >>>>>>>>>     gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> > >>>>>>>>>     2 files changed, 31 insertions(+), 14 deletions(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> > >>>>>>>>> index d2909c7..0658368 100755
> > >>>>>>>>> --- a/.gitlab-ci/setup-container.sh
> > >>>>>>>>> +++ b/.gitlab-ci/setup-container.sh
> > >>>>>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> > >>>>>>>>>     'bookworm')
> > >>>>>>>>>           # libclang-rt-dev for the clang ASan runtime.
> > >>>>>>>>>           PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> > >>>>>>>>> +        # For cam and lc-compliance
> > >>>>>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
> > >>>>>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> > >>>>>>>>>           ;;
> > >>>>>>>>>     'trixie')
> > >>>>>>>>>           # gcc 13 to expand compilation testing coverage.
> > >>>>>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > >>>>>>>>> index 8bc8bc2..c7448b8 100644
> > >>>>>>>>> --- a/gitlab-ci.yml
> > >>>>>>>>> +++ b/gitlab-ci.yml
> > >>>>>>>>> @@ -64,7 +64,7 @@ include:
> > >>>>>>>>>     .libcamera-ci.debian:12:
> > >>>>>>>>>       variables:
> > >>>>>>>>>         FDO_DISTRIBUTION_VERSION: 'bookworm'
> > >>>>>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> > >>>>>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> > >>>>>>>>>
> > >>>>>>>>>     .libcamera-ci.debian:13:
> > >>>>>>>>>       variables:
> > >>>>>>>>> @@ -363,28 +363,18 @@ test-soraka:
> > >>>>>>>>>       script:
> > >>>>>>>>>         - submit .gitlab-ci/lava/soraka-camera-test.yml
> > >>>>>>>>>
> > >>>>>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> > >>>>>>>>> -# the unit tests.
> > >>>>>>>>> -test-unit:
> > >>>>>>>>> +# Enable only the options exercised by the unit tests.
> > >>>>>>>>> +build-test:debug:
> > >>>>>>>>
> > >>>>>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
> > >>>>>>>> build-package:cros. I think it would also make sense to use the same
> > >>>>>>>> build options for the amd64 and arm64 packages (beside possibly the
> > >>>>>>>> selected pipeline handlers, although the 'auto' option may work for
> > >>>>>>>> both).
> > >>>>>>>>
> > >>>>>>>>>       extends:
> > >>>>>>>>>         - .fdo.distribution-image@debian
> > >>>>>>>>>         - .libcamera-ci.debian:12
> > >>>>>>>>>         - .libcamera-ci.scripts
> > >>>>>>>>> -  stage: test
> > >>>>>>>>> +  stage: build
> > >>>>>>>>>       needs:
> > >>>>>>>>>         - job: container-debian:12
> > >>>>>>>>>           artifacts: false
> > >>>>>>>>> -  tags:
> > >>>>>>>>> -    - kvm
> > >>>>>>>>>       script:
> > >>>>>>>>>         - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> > >>>>>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > >>>>>>>>> -  artifacts:
> > >>>>>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > >>>>>>>>> -    when: always
> > >>>>>>>>> -    expire_in: 1 week
> > >>>>>>>>> -    paths:
> > >>>>>>>>> -      - build/meson-logs/
> > >>>>>>>>>       variables:
> > >>>>>>>>>         BUILD_TYPE: debug
> > >>>>>>>>>         MESON_OPTIONS: >-
> > >>>>>>>>> @@ -399,6 +389,30 @@ test-unit:
> > >>>>>>>>>           -D qcam=disabled
> > >>>>>>>>>           -D test=true
> > >>>>>>>>>           -D v4l2=true
> > >>>>>>>>> +  artifacts:
> > >>>>>>>>> +    paths:
> > >>>>>>>>> +      - build/
> > >>>>>>>>
> > >>>>>>>> The whole build directory can be very large. Can't we do the same as
> > >>>>>>>> build-package:arm64 and run package-libcamera.sh to only package what we
> > >>>>>>>> need ? We'll need probably need an unpackage script for the test-unit
> > >>>>>>>> job.
> > >>>>>>>
> > >>>>>>> But of course the unit test binaries don't get installed... Can we fix
> > >>>>>>> that and install them ? You can specify "install_tag : 'tests'" in
> > >>>>>>> meson.build so they won't be installed by default (an appropriate
> > >>>>>>> install_dir is also needed). This in turn requires bumping the minimum
> > >>>>>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> > >>>>>>
> > >>>>>> I've been told on IRC that the motivation for the "tests" install tag in
> > >>>>>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> > >>>>>> think we should switch to a separate runner for unit tests (the pain is
> > >>>>>> not worth the gain at this point in my opinion), but it could be useful
> > >>>>>> to tag lc-compliance with install_tag = 'tests'.
> > >>>>>>
> > >>>>>>> And now that I've said this, I realize we wouldn't be able to run "meson
> > >>>>>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
> > >>>>>>> for this. If not, given the size of the build directory, and to avoid
> > >>>>>>> transferring a large amount of data between runners, we may need to keep
> > >>>>>>> building libcamera within the test-unit job :-(
> > >>>>>>>
> > >>>>>>> A separate build-package target for lc-compliance would still make
> > >>>>>>> sense.
> > >>>>>
> > >>>>> I think it would be unfortunate to give up the usage `meson test` as you
> > >>>>> mentioned.
> > >>>>
> > >>>> We could work on a replacement, but it would require a significant
> > >>>> amount of work and I think there are better things to do.
> > >>>>
> > >>>>> I have not noticed that these build artifacts would put any
> > >>>>> appreciable strain on the infrastructure. The compressed build directory
> > >>>>> comes out to around 167 MiB; I am not sure if I would consider that a
> > >>>>> large amount of data. It is definitely cheaper, in terms of time, than
> > >>>>> building libcamera twice. Clearing the object files could be another
> > >>>>> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> > >>>>> remove more than half of the uncompressed size, and about 1/4 of
> > >>>>> the compressed size. Does this look acceptable?
> > >>>>
> > >>>> Possibly. We should probably ask the fdo sysadmins about what is
> > >>>> acceptable.
> > >>>>
> > >>>> I gave it a try locally though, and deleting all *.o files in the build
> > >>>> directory results in "meson test" rebuilding everything.
> > >>>
> > >>> As far as I can see that does not happen with the `--no-rebuild` option,
> > >>> which is already used in `test-libcamera-qemu.sh`.
> > > 
> > > Ah, good point, I missed that.
> > > 
> > >>>> For other uses of the artifacts (in particular deployment on real
> > >>>> devices), I would still prefer minimizing the bandwidth, creating a
> > >>>> package similarly to what build-package:arm64 does. How about keeping
> > >>>> test-unit as-is, at the cost of a recompilation, and creating a
> > >>>> build-package:amd64 that will be used by the lc-compliance test job ? We
> > >>>> can try to improve on top when/if needed.
> > >>>
> > >>> Couple observations:
> > >>>
> > >>> 1. The virtual pipeline handler configuration is not installed, so
> > >>>      that needs to be addressed. (Was this omitted intentionally?)
> > > 
> > > I don't know. We have to be careful here, we don't want to virtual
> > > pipeline handler to end up being enabled with a default valid
> > > configuration in distributions, otherwise people will all of a sudden
> > > see virtual cameras poluting their devices list.
> > 
> > I am not sure what should be done. Maybe a separate meson `install_tag`.
> > Or I suppose the configuration file could be created right before
> > running it?
> 
> Hmmmm... If this is a sample configuration file that we don't want to
> install by default in the location where it will be lookup up, how about
> installing it as an example in doc_install_dir (e.g.
> /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
> to be a fairly common pattern.


That sounds fairly reasonable.

> You can move the doc_install_dir definition from
> Documentation/meson.build to src/meson.build and rename it to
> libcamera_docdir.

I'm fine with that. It would only get installed there if the virtual
pipeline handler is enabled, and I suspect distro's won't include that ?


Will the CI define it's own 'virtual camera config' file to support it's
own virtual requirements?

--
Kieran


> 
> > >>> 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
> > >>>      need to be sprinkled in. I think this would be much better
> > >>>      if the package was not a mere tar archive but a proper deb/etc.
> > >>>      package. I imagine that is a prerequisite of deploying on real
> > >>>      hardware in any case, correct?
> > >>
> > >> Having the server build a 'real' deb would be a real benefit IMO, and
> > >> indeed could help with installation/set up on real targets for testing
> > >> in defined environments.
> > >>
> > >> I've wanted to tackle that for a while but never got time.
> > > 
> > > Same, I think it's probably worth it, but it will require some effort
> > > and I didn't have enough time when I implemented build-package:arm64.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Kieran Bingham Dec. 17, 2024, 5:39 p.m. UTC | #13
Quoting Laurent Pinchart (2024-12-17 17:32:25)
> On Tue, Dec 17, 2024 at 07:25:00PM +0200, Laurent Pinchart wrote:
> > > >>> Couple observations:
> > > >>>
> > > >>> 1. The virtual pipeline handler configuration is not installed, so
> > > >>>      that needs to be addressed. (Was this omitted intentionally?)
> > > > 
> > > > I don't know. We have to be careful here, we don't want to virtual
> > > > pipeline handler to end up being enabled with a default valid
> > > > configuration in distributions, otherwise people will all of a sudden
> > > > see virtual cameras poluting their devices list.
> > > 
> > > I am not sure what should be done. Maybe a separate meson `install_tag`.
> > > Or I suppose the configuration file could be created right before
> > > running it?
> > 
> > Hmmmm... If this is a sample configuration file that we don't want to
> > install by default in the location where it will be lookup up, how about
> > installing it as an example in doc_install_dir (e.g.
> > /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
> > to be a fairly common pattern.
> 
> And then in CI we would take that file and copy it to the expected
> location, renaming it to virtual.yaml.
> 
> There's also the question of what install_tag to use. Other data files
> use 'runtime', but those are installed to their runtime location. 'doc'
> could be an option, we can use that even if we disable documentation
> build in CI, but it may feel a bit weird, and possibly later install
> other types of documentation that we way not want (although I suppose
> installation of future "real" documentation could be conditioned by the
> documentation meson option).
> 
> Another option is to create it from scratch in CI (possibly with the
> file just added to the CI repository and copied in the test job).

Ultimately that's perhaps the 'easiest' option. But it would be nice to
keep the CI aligned with any updates to the example virtual
configuration too.

--
Kieran
Laurent Pinchart Dec. 18, 2024, 1:07 a.m. UTC | #14
On Tue, Dec 17, 2024 at 05:38:03PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-12-17 17:24:59)
> > On Tue, Dec 17, 2024 at 05:09:45PM +0100, Barnabás Pőcze wrote:
> > > 2024. 12. 16. 21:01 keltezéssel, Laurent Pinchart írta:
> > > > On Mon, Dec 16, 2024 at 06:26:14PM +0000, Kieran Bingham wrote:
> > > >> Quoting Barnabás Pőcze (2024-12-16 17:28:46)
> > > >>> 2024. 12. 16. 12:04 keltezéssel, Laurent Pinchart írta:
> > > >>>> On Mon, Dec 16, 2024 at 10:13:54AM +0100, Barnabás Pőcze wrote:
> > > >>>>> 2024. 12. 15. 21:43 keltezéssel, Laurent Pinchart írta:
> > > >>>>>> On Sun, Dec 15, 2024 at 09:43:20PM +0200, Laurent Pinchart wrote:
> > > >>>>>>> On Sun, Dec 15, 2024 at 09:04:08PM +0200, Laurent Pinchart wrote:
> > > >>>>>>>> On Thu, Dec 12, 2024 at 07:16:54PM +0100, Barnabás Pőcze wrote:
> > > >>>>>>>>> The built artifacts will be reused in a later job, so split
> > > >>>>>>>>> the "test-unit" into the "build-test" and "test-unit" jobs.
> > > >>>>>>>>>
> > > >>>>>>>>> The `libevent` development package cannot be installed in the container
> > > >>>>>>>>
> > > >>>>>>>> I've write `libevent-dev` here to avoid ambiguities.
> > > >>>>>>>>
> > > >>>>>>>>> directly because it is not multiarch compatible. It is, however, installed
> > > >>>>>>>>> in the architecture specific build jobs, right before building. To ensure
> > > >>>>>>>>> that the it is available for already built executables in different jobs,
> > > >>>>>>>>
> > > >>>>>>>> "that the it is" ?
> > > >>>>>>>>
> > > >>>>>>>>> install just the libraries in the container.
> > > >>>>>>>>
> > > >>>>>>>> And name here `libevent`.
> > > >>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > >>>>>>>>> ---
> > > >>>>>>>>>     .gitlab-ci/setup-container.sh |  3 +++
> > > >>>>>>>>>     gitlab-ci.yml                 | 42 +++++++++++++++++++++++------------
> > > >>>>>>>>>     2 files changed, 31 insertions(+), 14 deletions(-)
> > > >>>>>>>>>
> > > >>>>>>>>> diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
> > > >>>>>>>>> index d2909c7..0658368 100755
> > > >>>>>>>>> --- a/.gitlab-ci/setup-container.sh
> > > >>>>>>>>> +++ b/.gitlab-ci/setup-container.sh
> > > >>>>>>>>> @@ -103,6 +103,9 @@ case $FDO_DISTRIBUTION_VERSION in
> > > >>>>>>>>>     'bookworm')
> > > >>>>>>>>>           # libclang-rt-dev for the clang ASan runtime.
> > > >>>>>>>>>           PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
> > > >>>>>>>>> +        # For cam and lc-compliance
> > > >>>>>>>>> +        # libevent-dev cannot be used here, see build-libcamera-common.sh
> > > >>>>>>>>> +        PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
> > > >>>>>>>>>           ;;
> > > >>>>>>>>>     'trixie')
> > > >>>>>>>>>           # gcc 13 to expand compilation testing coverage.
> > > >>>>>>>>> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > > >>>>>>>>> index 8bc8bc2..c7448b8 100644
> > > >>>>>>>>> --- a/gitlab-ci.yml
> > > >>>>>>>>> +++ b/gitlab-ci.yml
> > > >>>>>>>>> @@ -64,7 +64,7 @@ include:
> > > >>>>>>>>>     .libcamera-ci.debian:12:
> > > >>>>>>>>>       variables:
> > > >>>>>>>>>         FDO_DISTRIBUTION_VERSION: 'bookworm'
> > > >>>>>>>>> -    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
> > > >>>>>>>>> +    FDO_DISTRIBUTION_TAG: '2024-12-12.2'
> > > >>>>>>>>>
> > > >>>>>>>>>     .libcamera-ci.debian:13:
> > > >>>>>>>>>       variables:
> > > >>>>>>>>> @@ -363,28 +363,18 @@ test-soraka:
> > > >>>>>>>>>       script:
> > > >>>>>>>>>         - submit .gitlab-ci/lava/soraka-camera-test.yml
> > > >>>>>>>>>
> > > >>>>>>>>> -# Run the unit tests in a virtual machine. Enable only the options exercised by
> > > >>>>>>>>> -# the unit tests.
> > > >>>>>>>>> -test-unit:
> > > >>>>>>>>> +# Enable only the options exercised by the unit tests.
> > > >>>>>>>>> +build-test:debug:
> > > >>>>>>>>
> > > >>>>>>>> I'd call this build-package:amd64, as we have build-package:arm64 and
> > > >>>>>>>> build-package:cros. I think it would also make sense to use the same
> > > >>>>>>>> build options for the amd64 and arm64 packages (beside possibly the
> > > >>>>>>>> selected pipeline handlers, although the 'auto' option may work for
> > > >>>>>>>> both).
> > > >>>>>>>>
> > > >>>>>>>>>       extends:
> > > >>>>>>>>>         - .fdo.distribution-image@debian
> > > >>>>>>>>>         - .libcamera-ci.debian:12
> > > >>>>>>>>>         - .libcamera-ci.scripts
> > > >>>>>>>>> -  stage: test
> > > >>>>>>>>> +  stage: build
> > > >>>>>>>>>       needs:
> > > >>>>>>>>>         - job: container-debian:12
> > > >>>>>>>>>           artifacts: false
> > > >>>>>>>>> -  tags:
> > > >>>>>>>>> -    - kvm
> > > >>>>>>>>>       script:
> > > >>>>>>>>>         - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
> > > >>>>>>>>> -    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
> > > >>>>>>>>> -  artifacts:
> > > >>>>>>>>> -    name: libcamera-unit-tests-${CI_COMMIT_SHA}
> > > >>>>>>>>> -    when: always
> > > >>>>>>>>> -    expire_in: 1 week
> > > >>>>>>>>> -    paths:
> > > >>>>>>>>> -      - build/meson-logs/
> > > >>>>>>>>>       variables:
> > > >>>>>>>>>         BUILD_TYPE: debug
> > > >>>>>>>>>         MESON_OPTIONS: >-
> > > >>>>>>>>> @@ -399,6 +389,30 @@ test-unit:
> > > >>>>>>>>>           -D qcam=disabled
> > > >>>>>>>>>           -D test=true
> > > >>>>>>>>>           -D v4l2=true
> > > >>>>>>>>> +  artifacts:
> > > >>>>>>>>> +    paths:
> > > >>>>>>>>> +      - build/
> > > >>>>>>>>
> > > >>>>>>>> The whole build directory can be very large. Can't we do the same as
> > > >>>>>>>> build-package:arm64 and run package-libcamera.sh to only package what we
> > > >>>>>>>> need ? We'll need probably need an unpackage script for the test-unit
> > > >>>>>>>> job.
> > > >>>>>>>
> > > >>>>>>> But of course the unit test binaries don't get installed... Can we fix
> > > >>>>>>> that and install them ? You can specify "install_tag : 'tests'" in
> > > >>>>>>> meson.build so they won't be installed by default (an appropriate
> > > >>>>>>> install_dir is also needed). This in turn requires bumping the minimum
> > > >>>>>>> meson version from 0.63.0 to 0.64.0, which shouldn't be an issue.
> > > >>>>>>
> > > >>>>>> I've been told on IRC that the motivation for the "tests" install tag in
> > > >>>>>> meson is https://gitlab.gnome.org/GNOME/gnome-desktop-testing. I don't
> > > >>>>>> think we should switch to a separate runner for unit tests (the pain is
> > > >>>>>> not worth the gain at this point in my opinion), but it could be useful
> > > >>>>>> to tag lc-compliance with install_tag = 'tests'.
> > > >>>>>>
> > > >>>>>>> And now that I've said this, I realize we wouldn't be able to run "meson
> > > >>>>>>> test" to run the tests :-/ I'm not sure there's an appropriate solution
> > > >>>>>>> for this. If not, given the size of the build directory, and to avoid
> > > >>>>>>> transferring a large amount of data between runners, we may need to keep
> > > >>>>>>> building libcamera within the test-unit job :-(
> > > >>>>>>>
> > > >>>>>>> A separate build-package target for lc-compliance would still make
> > > >>>>>>> sense.
> > > >>>>>
> > > >>>>> I think it would be unfortunate to give up the usage `meson test` as you
> > > >>>>> mentioned.
> > > >>>>
> > > >>>> We could work on a replacement, but it would require a significant
> > > >>>> amount of work and I think there are better things to do.
> > > >>>>
> > > >>>>> I have not noticed that these build artifacts would put any
> > > >>>>> appreciable strain on the infrastructure. The compressed build directory
> > > >>>>> comes out to around 167 MiB; I am not sure if I would consider that a
> > > >>>>> large amount of data. It is definitely cheaper, in terms of time, than
> > > >>>>> building libcamera twice. Clearing the object files could be another
> > > >>>>> option. With `artifacts:exclude: build/**/*.o` we can seemingly
> > > >>>>> remove more than half of the uncompressed size, and about 1/4 of
> > > >>>>> the compressed size. Does this look acceptable?
> > > >>>>
> > > >>>> Possibly. We should probably ask the fdo sysadmins about what is
> > > >>>> acceptable.
> > > >>>>
> > > >>>> I gave it a try locally though, and deleting all *.o files in the build
> > > >>>> directory results in "meson test" rebuilding everything.
> > > >>>
> > > >>> As far as I can see that does not happen with the `--no-rebuild` option,
> > > >>> which is already used in `test-libcamera-qemu.sh`.
> > > > 
> > > > Ah, good point, I missed that.
> > > > 
> > > >>>> For other uses of the artifacts (in particular deployment on real
> > > >>>> devices), I would still prefer minimizing the bandwidth, creating a
> > > >>>> package similarly to what build-package:arm64 does. How about keeping
> > > >>>> test-unit as-is, at the cost of a recompilation, and creating a
> > > >>>> build-package:amd64 that will be used by the lc-compliance test job ? We
> > > >>>> can try to improve on top when/if needed.
> > > >>>
> > > >>> Couple observations:
> > > >>>
> > > >>> 1. The virtual pipeline handler configuration is not installed, so
> > > >>>      that needs to be addressed. (Was this omitted intentionally?)
> > > > 
> > > > I don't know. We have to be careful here, we don't want to virtual
> > > > pipeline handler to end up being enabled with a default valid
> > > > configuration in distributions, otherwise people will all of a sudden
> > > > see virtual cameras poluting their devices list.
> > > 
> > > I am not sure what should be done. Maybe a separate meson `install_tag`.
> > > Or I suppose the configuration file could be created right before
> > > running it?
> > 
> > Hmmmm... If this is a sample configuration file that we don't want to
> > install by default in the location where it will be lookup up, how about
> > installing it as an example in doc_install_dir (e.g.
> > /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
> > to be a fairly common pattern.
> 
> That sounds fairly reasonable.
> 
> > You can move the doc_install_dir definition from
> > Documentation/meson.build to src/meson.build and rename it to
> > libcamera_docdir.
> 
> I'm fine with that. It would only get installed there if the virtual
> pipeline handler is enabled, and I suspect distro's won't include that ?

Distros may, but as long as the example file is installed as an example
in the documentation directory, and not where the virtual pipeline
handler looks for it, it won't disturb users.

> Will the CI define it's own 'virtual camera config' file to support it's
> own virtual requirements?

I'm increasingly thinking that should be the way, in which case we don't
need to install the example. We still could though.

> > > >>> 2. I am not a fan of the extra `tar` and `ldconfig`  calls that
> > > >>>      need to be sprinkled in. I think this would be much better
> > > >>>      if the package was not a mere tar archive but a proper deb/etc.
> > > >>>      package. I imagine that is a prerequisite of deploying on real
> > > >>>      hardware in any case, correct?
> > > >>
> > > >> Having the server build a 'real' deb would be a real benefit IMO, and
> > > >> indeed could help with installation/set up on real targets for testing
> > > >> in defined environments.
> > > >>
> > > >> I've wanted to tackle that for a while but never got time.
> > > > 
> > > > Same, I think it's probably worth it, but it will require some effort
> > > > and I didn't have enough time when I implemented build-package:arm64.
Laurent Pinchart Dec. 18, 2024, 1:10 a.m. UTC | #15
On Tue, Dec 17, 2024 at 05:39:28PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-12-17 17:32:25)
> > On Tue, Dec 17, 2024 at 07:25:00PM +0200, Laurent Pinchart wrote:
> > > > >>> Couple observations:
> > > > >>>
> > > > >>> 1. The virtual pipeline handler configuration is not installed, so
> > > > >>>      that needs to be addressed. (Was this omitted intentionally?)
> > > > > 
> > > > > I don't know. We have to be careful here, we don't want to virtual
> > > > > pipeline handler to end up being enabled with a default valid
> > > > > configuration in distributions, otherwise people will all of a sudden
> > > > > see virtual cameras poluting their devices list.
> > > > 
> > > > I am not sure what should be done. Maybe a separate meson `install_tag`.
> > > > Or I suppose the configuration file could be created right before
> > > > running it?
> > > 
> > > Hmmmm... If this is a sample configuration file that we don't want to
> > > install by default in the location where it will be lookup up, how about
> > > installing it as an example in doc_install_dir (e.g.
> > > /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
> > > to be a fairly common pattern.
> > 
> > And then in CI we would take that file and copy it to the expected
> > location, renaming it to virtual.yaml.
> > 
> > There's also the question of what install_tag to use. Other data files
> > use 'runtime', but those are installed to their runtime location. 'doc'
> > could be an option, we can use that even if we disable documentation
> > build in CI, but it may feel a bit weird, and possibly later install
> > other types of documentation that we way not want (although I suppose
> > installation of future "real" documentation could be conditioned by the
> > documentation meson option).
> > 
> > Another option is to create it from scratch in CI (possibly with the
> > file just added to the CI repository and copied in the test job).
> 
> Ultimately that's perhaps the 'easiest' option. But it would be nice to
> keep the CI aligned with any updates to the example virtual
> configuration too.

Yes, but if we consider that the virtual pipeline handler configuration
we use in CI has to be written for the needs of CI, then it should be
part of the libcamera-ci repository, and updated as the virtual pipeline
handler evolves.

On the other hand, the pipeline handler should pass lc-compliance with
any valid configuration, so it makes sense to test the example
configuration. Installing it to the documentation directory and then
copying it from there in the CI lc-compliance job sounds like a good
option (which contradicts what I wrote in a separate e-mail 2 minutes
ago :-)). Barnabás, what do you think ?
Barnabás Pőcze Dec. 18, 2024, 9:24 p.m. UTC | #16
2024. 12. 18. 2:10 keltezéssel, Laurent Pinchart írta:
> On Tue, Dec 17, 2024 at 05:39:28PM +0000, Kieran Bingham wrote:
>> Quoting Laurent Pinchart (2024-12-17 17:32:25)
>>> On Tue, Dec 17, 2024 at 07:25:00PM +0200, Laurent Pinchart wrote:
>>>>>>>> Couple observations:
>>>>>>>>
>>>>>>>> 1. The virtual pipeline handler configuration is not installed, so
>>>>>>>>       that needs to be addressed. (Was this omitted intentionally?)
>>>>>>
>>>>>> I don't know. We have to be careful here, we don't want to virtual
>>>>>> pipeline handler to end up being enabled with a default valid
>>>>>> configuration in distributions, otherwise people will all of a sudden
>>>>>> see virtual cameras poluting their devices list.
>>>>>
>>>>> I am not sure what should be done. Maybe a separate meson `install_tag`.
>>>>> Or I suppose the configuration file could be created right before
>>>>> running it?
>>>>
>>>> Hmmmm... If this is a sample configuration file that we don't want to
>>>> install by default in the location where it will be lookup up, how about
>>>> installing it as an example in doc_install_dir (e.g.
>>>> /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
>>>> to be a fairly common pattern.
>>>
>>> And then in CI we would take that file and copy it to the expected
>>> location, renaming it to virtual.yaml.
>>>
>>> There's also the question of what install_tag to use. Other data files
>>> use 'runtime', but those are installed to their runtime location. 'doc'
>>> could be an option, we can use that even if we disable documentation
>>> build in CI, but it may feel a bit weird, and possibly later install
>>> other types of documentation that we way not want (although I suppose
>>> installation of future "real" documentation could be conditioned by the
>>> documentation meson option).
>>>
>>> Another option is to create it from scratch in CI (possibly with the
>>> file just added to the CI repository and copied in the test job).
>>
>> Ultimately that's perhaps the 'easiest' option. But it would be nice to
>> keep the CI aligned with any updates to the example virtual
>> configuration too.
> 
> Yes, but if we consider that the virtual pipeline handler configuration
> we use in CI has to be written for the needs of CI, then it should be
> part of the libcamera-ci repository, and updated as the virtual pipeline
> handler evolves.
> 
> On the other hand, the pipeline handler should pass lc-compliance with
> any valid configuration, so it makes sense to test the example
> configuration. Installing it to the documentation directory and then
> copying it from there in the CI lc-compliance job sounds like a good
> option (which contradicts what I wrote in a separate e-mail 2 minutes
> ago :-)). Barnabás, what do you think ?
> 

Not sure. To be honest I am not a fan of either solution, but I don't
really ave an alternative idea. I suppose we could have a "special"
camera(s) named something like "Virtual-CI" in the example configuration,
which would be the one used in the CI test. Between these two options,
keeping the configuration in the main repository seems better to me.

Regards,
Barnabás Pőcze
Laurent Pinchart Dec. 18, 2024, 11:27 p.m. UTC | #17
Hi Barnabás,

On Wed, Dec 18, 2024 at 10:24:50PM +0100, Barnabás Pőcze wrote:
> 2024. 12. 18. 2:10 keltezéssel, Laurent Pinchart írta:
> > On Tue, Dec 17, 2024 at 05:39:28PM +0000, Kieran Bingham wrote:
> >> Quoting Laurent Pinchart (2024-12-17 17:32:25)
> >>> On Tue, Dec 17, 2024 at 07:25:00PM +0200, Laurent Pinchart wrote:
> >>>>>>>> Couple observations:
> >>>>>>>>
> >>>>>>>> 1. The virtual pipeline handler configuration is not installed, so
> >>>>>>>>       that needs to be addressed. (Was this omitted intentionally?)
> >>>>>>
> >>>>>> I don't know. We have to be careful here, we don't want to virtual
> >>>>>> pipeline handler to end up being enabled with a default valid
> >>>>>> configuration in distributions, otherwise people will all of a sudden
> >>>>>> see virtual cameras poluting their devices list.
> >>>>>
> >>>>> I am not sure what should be done. Maybe a separate meson `install_tag`.
> >>>>> Or I suppose the configuration file could be created right before
> >>>>> running it?
> >>>>
> >>>> Hmmmm... If this is a sample configuration file that we don't want to
> >>>> install by default in the location where it will be lookup up, how about
> >>>> installing it as an example in doc_install_dir (e.g.
> >>>> /usr/share/doc/libcamera/) and name it virtual.yaml.example ? That seems
> >>>> to be a fairly common pattern.
> >>>
> >>> And then in CI we would take that file and copy it to the expected
> >>> location, renaming it to virtual.yaml.
> >>>
> >>> There's also the question of what install_tag to use. Other data files
> >>> use 'runtime', but those are installed to their runtime location. 'doc'
> >>> could be an option, we can use that even if we disable documentation
> >>> build in CI, but it may feel a bit weird, and possibly later install
> >>> other types of documentation that we way not want (although I suppose
> >>> installation of future "real" documentation could be conditioned by the
> >>> documentation meson option).
> >>>
> >>> Another option is to create it from scratch in CI (possibly with the
> >>> file just added to the CI repository and copied in the test job).
> >>
> >> Ultimately that's perhaps the 'easiest' option. But it would be nice to
> >> keep the CI aligned with any updates to the example virtual
> >> configuration too.
> > 
> > Yes, but if we consider that the virtual pipeline handler configuration
> > we use in CI has to be written for the needs of CI, then it should be
> > part of the libcamera-ci repository, and updated as the virtual pipeline
> > handler evolves.
> > 
> > On the other hand, the pipeline handler should pass lc-compliance with
> > any valid configuration, so it makes sense to test the example
> > configuration. Installing it to the documentation directory and then
> > copying it from there in the CI lc-compliance job sounds like a good
> > option (which contradicts what I wrote in a separate e-mail 2 minutes
> > ago :-)). Barnabás, what do you think ?
> 
> Not sure. To be honest I am not a fan of either solution, but I don't
> really ave an alternative idea. I suppose we could have a "special"
> camera(s) named something like "Virtual-CI" in the example configuration,
> which would be the one used in the CI test. Between these two options,
> keeping the configuration in the main repository seems better to me.

Agreed. Let's go for that, installing the configuration file as an
example, and copying it as part of the CI job.

Patch
diff mbox series

diff --git a/.gitlab-ci/setup-container.sh b/.gitlab-ci/setup-container.sh
index d2909c7..0658368 100755
--- a/.gitlab-ci/setup-container.sh
+++ b/.gitlab-ci/setup-container.sh
@@ -103,6 +103,9 @@  case $FDO_DISTRIBUTION_VERSION in
 'bookworm')
 	# libclang-rt-dev for the clang ASan runtime.
 	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libclang-rt-dev )
+	# For cam and lc-compliance
+	# libevent-dev cannot be used here, see build-libcamera-common.sh
+	PKGS_LIBCAMERA_RUNTIME_MULTIARCH+=( libevent-2.1-7 libevent-pthreads-2.1-7 )
 	;;
 'trixie')
 	# gcc 13 to expand compilation testing coverage.
diff --git a/gitlab-ci.yml b/gitlab-ci.yml
index 8bc8bc2..c7448b8 100644
--- a/gitlab-ci.yml
+++ b/gitlab-ci.yml
@@ -64,7 +64,7 @@  include:
 .libcamera-ci.debian:12:
   variables:
     FDO_DISTRIBUTION_VERSION: 'bookworm'
-    FDO_DISTRIBUTION_TAG: '2024-12-12.1'
+    FDO_DISTRIBUTION_TAG: '2024-12-12.2'

 .libcamera-ci.debian:13:
   variables:
@@ -363,28 +363,18 @@  test-soraka:
   script:
     - submit .gitlab-ci/lava/soraka-camera-test.yml

-# Run the unit tests in a virtual machine. Enable only the options exercised by
-# the unit tests.
-test-unit:
+# Enable only the options exercised by the unit tests.
+build-test:debug:
   extends:
     - .fdo.distribution-image@debian
     - .libcamera-ci.debian:12
     - .libcamera-ci.scripts
-  stage: test
+  stage: build
   needs:
     - job: container-debian:12
       artifacts: false
-  tags:
-    - kvm
   script:
     - $CI_PROJECT_DIR/.gitlab-ci/build-libcamera.sh
-    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
-  artifacts:
-    name: libcamera-unit-tests-${CI_COMMIT_SHA}
-    when: always
-    expire_in: 1 week
-    paths:
-      - build/meson-logs/
   variables:
     BUILD_TYPE: debug
     MESON_OPTIONS: >-
@@ -399,6 +389,30 @@  test-unit:
       -D qcam=disabled
       -D test=true
       -D v4l2=true
+  artifacts:
+    paths:
+      - build/
+    expire_in: 1 day
+
+# Run the unit tests in a virtual machine.
+test-unit:
+  extends:
+    - .fdo.distribution-image@debian
+    - .libcamera-ci.debian:12
+    - .libcamera-ci.scripts
+  stage: test
+  needs:
+    - job: build-test:debug
+  tags:
+    - kvm
+  script:
+    - $CI_PROJECT_DIR/.gitlab-ci/test-libcamera-qemu.sh
+  artifacts:
+    name: libcamera-unit-tests-${CI_COMMIT_SHA}
+    when: always
+    expire_in: 1 week
+    paths:
+      - build/meson-logs/

   # meson prior to 1.2.0 doesn't correctly escape non-printable characters
   # when generating the testlog XML. This results in an unparseable file.