RE: [PATCH v3 05/10] automation: Add Arm containers to containerize script

[Jiamei Xie <Jiamei.Xie@xxxxxxx>]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> Sent: Thursday, October 20, 2022 3:52 PM
> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>
> Subject: Re: [PATCH v3 05/10] automation: Add Arm containers to
> containerize script
> 
> Hi Jiamei,
> 
> On 20/10/2022 09:13, Jiamei Xie wrote:
> >
> >
> > Hi Michal,
> >
> >> -----Original Message-----
> >> From: Michal Orzel <michal.orzel@xxxxxxx>
> >> Sent: Thursday, October 20, 2022 2:59 PM
> >> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>; Stefano Stabellini
> >> <sstabellini@xxxxxxxxxx>
> >> Subject: Re: [PATCH v3 05/10] automation: Add Arm containers to
> >> containerize script
> >>
> >> Hi Jiamei,
> >>
> >> On 20/10/2022 05:00, Jiamei Xie wrote:
> >>>
> >>>
> >>> Hi Michal,
> >>>
> >>>> -----Original Message-----
> >>>> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
> Of
> >>>> Michal Orzel
> >>>> Sent: Tuesday, September 27, 2022 5:47 PM
> >>>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >>>> Cc: Michal Orzel <michal.orzel@xxxxxxx>; Doug Goldstein
> >>>> <cardoe@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >>>> Subject: [PATCH v3 05/10] automation: Add Arm containers to
> >> containerize
> >>>> script
> >>>>
> >>>> Script automation/scripts/containerize makes it easy to build Xen
> within
> >>>> predefined containers from gitlab container registry. This script is
> >>>> currently missing the helpers to select Arm containers, so populate the
> >>>> necessary entries.
> >>>>
> >>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> >>>> Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >>>> ---
> >>
> >>>
> >>> [Jiamei Xie]
> >>> I wonder if an default container for arm can be added. For example,  if
> >>>  "CONTAINER=arm64 automation/scripts/containerize bash",
> >>>  set the default CONTAINER as "registry.gitlab.com/xen-
> >> project/xen/alpine:3.12-arm64v8"
> >>>
> >>
> >> It can be added doing the following:
> >>
> >> diff --git a/automation/scripts/containerize
> >> b/automation/scripts/containerize
> >> index 0f4645c4cccb..b395bd359ecf 100755
> >> --- a/automation/scripts/containerize
> >> +++ b/automation/scripts/containerize
> >> @@ -25,7 +25,7 @@ die() {
> >>  BASE="registry.gitlab.com/xen-project/xen"
> >>  case "_${CONTAINER}" in
> >>      _alpine) CONTAINER="${BASE}/alpine:3.12" ;;
> >> -    _alpine-arm64v8) CONTAINER="${BASE}/alpine:3.12-arm64v8" ;;
> >> +    _alpine-arm64v8|_arm64) CONTAINER="${BASE}/alpine:3.12-
> arm64v8" ;;
> >>      _archlinux|_arch) CONTAINER="${BASE}/archlinux:current" ;;
> >>      _riscv64) CONTAINER="${BASE}/archlinux:riscv64" ;;
> >>      _centos7) CONTAINER="${BASE}/centos:7" ;;
> >>
> >> The question is whether it would be beneficial. After all you would still
> need
> >> to
> >> type CONTAINER=arm64, whereas at the moment, you need to type
> >> CONTAINER=alpine-arm64v8.
> >> TBH I'm not sure it is improving anything (?).
> >>
> >> ~Michal
> > [Jiamei Xie]
> > I am not sure about this either.  I added something like below f to run it 
> > on
> arm64 machine.   But it  didn't take "running container for a different
> architecture" into consideration.
> >
> So your question is not about adding default container when selecting
> CONTAINER=arm64, but adding
> a default one when running on arm64 platform. Right now, the default one
> is debian:stretch
> (if you don't type CONTAINER= at all). Do I understand it right that you
> would like the same
> behavior when running on arm64 platform (currently, it would also select
> debian:stretch)?
> So that when executing:
> ./automation/scripts/containerize ...
> it would automatically select alpine-arm64v8?
> 
Yes, this is what I mean.
> 
> > --- a/automation/scripts/containerize
> > +++ b/automation/scripts/containerize
> > @@ -18,6 +18,12 @@ die() {
> >      exit 1
> >  }
> >
> > +# There are two containers that can run on aarch64, unstable and alpine.
> > +# Set the default container to alpine for aarch64
> > +if [[ $(uname -m) = "aarch64" && -z ${CONTAINER} ]]; then
> The output from `uname -m` for arm64 can be aarch64 and arm64.
> 
> > +    CONTAINER="alpine"
> > +fi
> > +
> >  #
> >  # The caller is expected to override the CONTAINER environment
> >  # variable with the container they wish to launch.
> > @@ -41,6 +47,11 @@ case "_${CONTAINER}" in
> >      _opensuse-tumbleweed|_tumbleweed)
> CONTAINER="${BASE}/suse:opensuse-tumbleweed" ;;
> >  esac
> >
> > +# Containers for aarch64 have a sufix "-arm64v8"
> > +if [[ $(uname -m) = "aarch64" ]]; then
> > +    CONTAINER="${CONTAINER}-arm64v8"
> > +fi
> This is not needed. CONTAINER can be selected on the first check and let
> case/esac block
> to determine the full path to container.
> 
> > +
> >  # Use this variable to control whether root should be used
> >  case "_${CONTAINER_UID0}" in
> >      _1)   userarg= ;;
> >
> >
> > Best wishes
> > Jiamei Xie
> >
> >
> 
> What you are asking for can be done in a simpler way. The following is
> enough:
> 
> diff --git a/automation/scripts/containerize
> b/automation/scripts/containerize
> index 0f4645c4cccb..4e7e8bb48e3a 100755
> --- a/automation/scripts/containerize
> +++ b/automation/scripts/containerize
> @@ -18,6 +18,11 @@ die() {
>      exit 1
>  }
> 
> +# Select default container when running on arm64 machine.
> +if [ -z "${CONTAINER}" ] && uname -m | grep -qE 'aarch64|arm64'; then
> +    CONTAINER="alpine-arm64v8"
> +fi
> +
>  #
>  # The caller is expected to override the CONTAINER environment
>  # variable with the container they wish to launch.
> 
> ~Michal
Yeah, I agree with this  implementation. 

Best wishes
Jiamei Xie