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

[Michal Orzel <michal.orzel@xxxxxxx>]

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?


> --- 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