Re: [yocto] [yocto-autobuilder-helper][PATCH v2] AUH: Add Openembedded auto-update-helper with list of layer to test

[Richard Purdie <richard.purdie@linuxfoundation.org>]

Hi David,

Comments below. I did try and sync up with Alex Kanavin so we have a
consistent direction.

On Tue, 2023-10-24 at 10:16 +0200, David PIERRET wrote:
> On Fri, Oct 20, 2023 at 4:57 PM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > Hi David,
> > 
> > Thanks for revising this, it is heading in the right direction.
> > 
> > On Fri, 2023-10-20 at 16:05 +0200, David Pierret wrote:
> > > - Add setup and run script openembedded specific
> > > - Add job config
> > > 
> > > Signed-off-by: David Pierret <david.pierret@smile.fr>
> > > ---
> > >  config.json          |  6 ++++++
> > >  scripts/run-auh-oe   | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > >  scripts/setup-auh-oe | 34 +++++++++++++++++++++++++++++++++
> > >  3 files changed, 85 insertions(+)
> > >  create mode 100755 scripts/run-auh-oe
> > >  create mode 100755 scripts/setup-auh-oe
> > > 
> > > diff --git a/config.json b/config.json
> > > index 3acb710..bbd0aaf 100644
> > > --- a/config.json
> > > +++ b/config.json
> > > @@ -1420,6 +1420,12 @@
> > >                  "${SCRIPTSDIR}/setup-auh ${HELPERBUILDDIR}; ${SCRIPTSDIR}/run-auh ${HELPERBUILDDIR} ${WEBPUBLISH_DIR}/pub/auh/"
> > >              ]
> > >          },
> > > +        "auh-openembedded" : {
> > 
> > This needs to be "auh-meta-oe" since "auh" is for openembedded-core and
> > this is otherwise going to be confusing.
> > 
> > > +            "NEEDREPOS" : ["poky", "meta-openembedded"],
> > > +            "EXTRAPLAINCMDS" : [
> > > +                "${SCRIPTSDIR}/setup-auh-oe ${HELPERBUILDDIR}; ${SCRIPTSDIR}/run-auh-oe ${HELPERBUILDDIR} ${WEBPUBLISH_DIR}/pub/auh/ ${HELPERBUILDDIR}/meta-openembedded ${HELPERBUILDDIR}/meta-openembedded/meta-oe ${HELPERBUILDDIR}/meta-openembedded/meta-python ${HELPERBUILDDIR}/meta-openembedded/meta-perl ${HELPERBUILDDIR}/meta-openembedded/meta-networking ${HELPERBUILDDIR}/meta-openembedded/meta-multimedia ${HELPERBUILDDIR}/meta-openembedded/meta-gnome ${HELPERBUILDDIR}/meta-openembedded/meta-xfce ${HELPERBUILDDIR}/meta-openembedded/meta-filesystems ${HELPERBUILDDIR}/meta-openembedded/meta-initramfs ${HELPERBUILDDIR}/meta-openembedded/meta-webserver "
> > > +            ]
> > > +        },
> > >          "a-quick" : {
> > >              "TEMPLATE" : "trigger-build"
> > >          },
> > 
> > Would it be better to have one setup-auh/run-auh script and add
> > ${HELPERTARGET}  as a parameter?
> > 
> > This should translate to "auh" or "auh-meta-oe" and the script can then
> > have conditionals in to handle both cases rather than duplicating the
> > script?
> 
> The setup-auh script is cloning its own repository instead of using
> the one initialized by CI.
> Any reason to not use them ? We could use the NEEDREPOS instead of the
> setup script,
> and add the auto-upgrade-helper repository in the config.

I think it does it's own clone for historical reasons from when it was
a standalone tool. I'm fine with letting the autobuilder handle this.

> > > diff --git a/scripts/run-auh-oe b/scripts/run-auh-oe
> > > new file mode 100755
> > > index 0000000..588755a
> > > --- /dev/null
> > > +++ b/scripts/run-auh-oe
> > 
> > Similarly, this should be run-auh-meta-oe.
> > 
> > > @@ -0,0 +1,45 @@
> > > +#!/bin/bash
> > > +#
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +#
> > > +# Run Auto Upgrade Helper in a directory set up by setup_auh.
> > > +#
> > > +# Called with $1 - the directory where the setup was created
> > > +
> > > +if [ -z $1 ]; then
> > > +  echo "Use: $0 [auh_setup_dir] [publish_dir] [meta_dir] [meta_list]"
> > > +  exit 1
> > > +fi
> > > +
> > > +full_dir=$(readlink -e $1)
> > > +
> > > +auh_dir=$full_dir/auto-upgrade-helper
> > > +poky_dir=$full_dir/poky
> > > +
> > > +build_dir=$full_dir/build
> > > +sstate_dir=$full_dir/build/sstate-cache
> > > +meta_dir=$3
> > > +meta_list=${@:4}
> > > +machine_list="qemux86 qemux86-64 qemuarm qemumips qemuppc qemux86_musl"
> > 
> > Do we need to vary the machine_list per layer? At present it only seems
> > to support one list anyway so I'm not sure this does anything useful?
> 
> The list is the same as the one used for poky.

It matches the default in the scripts though so I was wondering why
this was specified?

FWIW I agree we should perhaps limit testing to qemux86-64, qemuarm and
qemux86_musl initially just to make things a little easier.

> > > +pushd $meta_dir || exit 1
> > > +
> > > +# Base the upgrades on meta_openembedded master
> > > +git fetch origin
> > > +git checkout -B tmp-auh-upgrades origin/main
> > > +
> > > +source $poky_dir/oe-init-build-env $build_dir
> > > +
> > > +# build the layer_names variable to me used in the command line
> > > +layer_names=""
> > > +for d in $meta_list; do
> > > +  layer_names+=" $(basename ${d})"
> > > +done
> > > +
> > > +$auh_dir/upgrade-helper.py -e --layer-dir ${meta_dir} --layer-names ${layer_names} --layer-machines ${machine_list} -- all
> > 
> > Would it be simpler just to iterate, calling upgrade-helper once per
> > sub-layer or meta-openembedded?
> 
> Is it prefered to set 1 step per layer ?
> or keep the call to the script but iterate over a list of layers ?

I think a step per layer will make things clearer in the autobuilder
output.

> > You may have considered that in which case I'd just like to understand
> > why you ended up with this as the preferred way of doing things?
> > 
> > In some ways a separate report/run may be useful for the way meta-oe
> > maintainers might handle this?
> 
> The iteration over a list was my first approach, but aborted following
> review on patch v1.
> The V1 review asked to allow the AUH to accept dynamic layers as parameters.
> We maybe misunderstood each other.

I think this is perhaps a misunderstanding. I think we do want AUH to
learn the concept of testing just a layer but we do want to keep the
iteration as autobuilder steps to keep the output and logs separate.

There are also different maintainers for each layer in meta-oe so we
may want to copy different people on different bits of the run, we
certainly need that capability.

> > > +  echo "Use: $0 target_dir"
> > > +  exit 1
> > > +fi
> > > +
> > > +mkdir -p $1
> > > +pushd $1
> > > +
> > > +git clone git://git.yoctoproject.org/poky
> > > +pushd poky
> > > +git config user.email auh@yoctoproject.org
> > > +git config user.name "Auto Upgrade Helper"
> > > +popd
> > > +git clone git://git.openembedded.org/meta-openembedded
> > > +pushd meta-openembedded
> > > +git config user.email auh@yoctoproject.org
> > > +git config user.name "Auto Upgrade Helper"
> > > +popd
> > > +git clone git://git.yoctoproject.org/auto-upgrade-helper
> > > +source poky/oe-init-build-env build
> > > +mkdir -p upgrade-helper
> > > +popd
> > > +
> > > +cp $CONFIG_DIR/upgrade-helper.conf $1/build/upgrade-helper
> > > +cat $CONFIG_DIR/local.conf.append >> $1/build/conf/local.conf
> > 
> > Would a upgrade-helper.conf per target help and simplfy the script
> > differences?
> 
> Do you mean 1 upgrade-helper.conf per layer ?

That would allow us to copy different maintainers on different parts of
the run for example so might be useful?

Also, at least initially can we configure this to send the output to
test-list@lists.yoctoproject.org? That way we can test it is working,
then make it "live" without spamming the main list.

Cheers,

Richard