Re: [PATCH 1/9] automation: Use custom build jobs when extra config options are needed

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Sep 2022, Michal Orzel wrote:
> Hi Stefano,
> 
> On 22/09/2022 23:40, Stefano Stabellini wrote:
> > 
> > 
> > On Thu, 22 Sep 2022, Michal Orzel wrote:
> >> Currently, all the arm64 defconfig build jobs, regardless of the
> >> container used, end up building Xen with the extra config options
> >> specified in the main build script (e.g. CONFIG_EXPERT,
> >> CONFIG_STATIC_MEMORY). Because these options are only needed for
> >> specific test jobs, the current behavior of the CI is incorrect
> >> as we add the extra options to all the defconfig builds. This means
> >> that on arm64 there is not a single job performing proper defconfig build.
> >>
> >> To fix this issue, add custom build jobs each time there is a need for
> >> building Xen with additional config options. Introduce EXTRA_XEN_CONFIG
> >> variable to be used by these jobs to store the required options. This
> >> variable will be then read by the main build script to modify the .config
> >> file. This will also help users to understand what is needed to run 
> >> specific
> >> test.
> >>
> >> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> >> ---
> >> This patch could actually be consider to be taken for 4.17 release.
> >> The reason why is because currently our CI for arm64 does not even
> >> peform clean defconfig build which is quite crucial target to be tested.
> >> Performing builds always with EXPERT and UNSUPPORTED is not something so
> >> beneficial for release tests. This is up to the release manager.
> > 
> > + Henry
> > 
> > I agree this should go in 4.17, so that gitlab-ci can test non-DEBUG
> > builds on ARM.
> > 
> Do you mean the whole series should go in?
> I'm ok with that, even though I only marked this patch as the one that should 
> go in
> as it can be seen as a fix. But I can also see the benefits of merging the 
> whole series.

I think only this patch should go in. I like this series but it is best
to stay on the safe side and push to staging the rest of the series
later after the release.

 
> >> ---
> >>  automation/gitlab-ci/build.yaml | 15 +++++++++++++++
> >>  automation/gitlab-ci/test.yaml  |  4 ++--
> >>  automation/scripts/build        |  8 ++------
> >>  3 files changed, 19 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/automation/gitlab-ci/build.yaml 
> >> b/automation/gitlab-ci/build.yaml
> >> index 720ce6e07ba0..a39ed72aac6d 100644
> >> --- a/automation/gitlab-ci/build.yaml
> >> +++ b/automation/gitlab-ci/build.yaml
> >> @@ -566,6 +566,21 @@ alpine-3.12-gcc-debug-arm64:
> >>    variables:
> >>      CONTAINER: alpine:3.12-arm64v8
> >>
> >> +alpine-3.12-gcc-arm64-staticmem:
> >> +  extends: .gcc-arm64-build
> >> +  variables:
> >> +    CONTAINER: alpine:3.12-arm64v8
> >> +    EXTRA_XEN_CONFIG: |
> > 
> > Why the "|" ?
> > 
> > I was trying to look for its documentation in the gitlab yaml docs but
> > couldn't find it.
> > 
> By default gitlab variables are one liners so that they can store one 
> key:value pair.
> If you want to define a variable storing multiple values (in this case we 
> want to
> store multi-line string because of .config format) you need to use |. You can 
> check [1].

OK


> > 
> >> +      CONFIG_EXPERT=y
> >> +      CONFIG_UNSUPPORTED=y
> >> +      CONFIG_STATIC_MEMORY=y
> >> +
> >> +alpine-3.12-gcc-arm64-boot-cpupools:
> >> +  extends: .gcc-arm64-build
> >> +  variables:
> >> +    CONTAINER: alpine:3.12-arm64v8
> >> +    EXTRA_XEN_CONFIG: |
> >> +      CONFIG_BOOT_TIME_CPUPOOLS=y
> >>
> >>  ## Test artifacts common
> >>
> >> diff --git a/automation/gitlab-ci/test.yaml 
> >> b/automation/gitlab-ci/test.yaml
> >> index d899b3bdbf7a..4f96e6e322de 100644
> >> --- a/automation/gitlab-ci/test.yaml
> >> +++ b/automation/gitlab-ci/test.yaml
> >> @@ -88,7 +88,7 @@ qemu-smoke-arm64-gcc-staticmem:
> >>    script:
> >>      - ./automation/scripts/qemu-smoke-arm64.sh static-mem 2>&1 | tee 
> >> qemu-smoke-arm64.log
> >>    needs:
> >> -    - alpine-3.12-gcc-arm64
> >> +    - alpine-3.12-gcc-arm64-staticmem
> >>      - alpine-3.12-arm64-rootfs-export
> >>      - kernel-5.19-arm64-export
> >>      - qemu-system-aarch64-6.0.0-arm64-export
> >> @@ -107,7 +107,7 @@ qemu-smoke-arm64-gcc-boot-cpupools:
> >>    script:
> >>      - ./automation/scripts/qemu-smoke-arm64.sh boot-cpupools 2>&1 | tee 
> >> qemu-smoke-arm64.log
> >>    needs:
> >> -    - alpine-3.12-gcc-arm64
> >> +    - alpine-3.12-gcc-arm64-boot-cpupools
> >>      - alpine-3.12-arm64-rootfs-export
> >>      - kernel-5.19-arm64-export
> >>      - qemu-system-aarch64-6.0.0-arm64-export
> >> diff --git a/automation/scripts/build b/automation/scripts/build
> >> index 2f15ab3198e6..7d441cedb4ae 100755
> >> --- a/automation/scripts/build
> >> +++ b/automation/scripts/build
> >> @@ -15,12 +15,8 @@ if [[ "${RANDCONFIG}" == "y" ]]; then
> >>      make -j$(nproc) -C xen 
> >> KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config randconfig
> >>      hypervisor_only="y"
> >>  else
> >> -    if [[ "${XEN_TARGET_ARCH}" = "arm64" ]]; then
> >> -        echo "
> >> -CONFIG_EXPERT=y
> >> -CONFIG_UNSUPPORTED=y
> >> -CONFIG_STATIC_MEMORY=y
> >> -CONFIG_BOOT_TIME_CPUPOOLS=y" > xen/.config
> >> +    if [ -n "${EXTRA_XEN_CONFIG}" ]; then
> > 
> > NIT: for uniformity with rest of the file use
> > 
> >   if [[ -n "${EXTRA_XEN_CONFIG}" ]]; then
> > 
> Ok, will do in v2.
> 
> > 
> >> +        echo "${EXTRA_XEN_CONFIG}" > xen/.config
> >>          make -j$(nproc) -C xen olddefconfig
> >>      else
> >>          make -j$(nproc) -C xen defconfig
> >> --
> >> 2.25.1
> >>
> 
> ~Michal
> 
> [1] 
> https://docs.gitlab.com/ee/ci/variables/#store-multiple-values-in-one-variable
>