Re: [ImageBuilder] Add zstd compression support

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 30 Dec 2024, Ayan Kumar Halder wrote:
> Hi Jason
> 
> On 17/12/2024 21:19, Jason Andryuk wrote:
> > uboot-script-gen fails to process a zstd-compressed initramdisk, exiting
> > with:
> > Wrong file type initrd.img. It should be cpio archive, exiting.
> > 
> > Extend the existing approach to also check zstd.
> > 
> > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> > ---
> >   scripts/uboot-script-gen | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> > index fc63702..db2c011 100755
> > --- a/scripts/uboot-script-gen
> > +++ b/scripts/uboot-script-gen
> > @@ -567,6 +567,7 @@ function check_compressed_file_type()
> >   {
> >       local filename=$1
> >       local type="$2"
> > +    local file_type
> >         if [ ! -f $filename ]
> >       then
> > @@ -574,13 +575,17 @@ function check_compressed_file_type()
> >           cleanup_and_return_err
> >       fi
> >   -    file -L $filename | grep "gzip compressed data" &> /dev/null
> > -    if test $? == 0
> > -    then
> > +    file_type=$( file -L $filename )
> > +    if echo "$file_type" | grep -q "gzip compressed data" ; then
> >           local tmp=`mktemp`
> >           tmp_files+=($tmp)
> >           cat $filename | gunzip > $tmp
> >           filename=$tmp
> > +    elif echo "$file_type" | grep -q "Zstandard compressed data" ; then
> > +        local tmp=`mktemp`
> > +        tmp_files+=($tmp)
> > +        zstdcat $filename > $tmp
> 
> I think you need to list zstd in |prog_req
> |
> 
> |See
> https://gitlab.com/xen-project/imagebuilder/-/blob/master/scripts/uboot-script-gen?ref_type=heads#L5|
> 
> |Also you need to include this as a part of the dockerfiles like|
> 
> |https://gitlab.com/xen-project/xen/-/blob/staging/automation/tests-artifacts/qemu-system-aarch64/6.0.0-arm64v8.dockerfile?ref_type=heads|
> 
> |https://gitlab.com/xen-project/xen/-/blob/staging/automation/tests-artifacts/alpine/3.18-arm64v8.dockerfile?ref_type=heads

Ayan makes a good point. Given that zstdcat is only used if images are
provided in zstd format, and given that we have been using ImageBuilder
without zstd for years now, I am tempted to consider zstd optional,
rather than required. We could leave it out of prog_req. If we add it to
prog_req, ImageBuilder will refuse to start if zstd is not installed,
and, like Ayan wrote, we would have to add zstd to the Dockerfiles.

So in conclusion I think it is best to go with this patch as is:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>