[PATCH] ipu3: Use posix basename

[Laurent Pinchart]

On Tue, Apr 16, 2024 at 05:30:51PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-04-16 16:05:51)
> > On Tue, Apr 16, 2024 at 03:58:53PM +0100, Kieran Bingham wrote:
> > > Quoting Khem Raj (2024-03-24 18:33:07)
> > > > musl does not implement GNU basename extention and with latest musl
> > > > the prototype from string.h is also removed [1] which now results in
> > > > compile errors e.g.
> > > > 
> > > > ../git/utils/ipu3/ipu3-pack.c:21:47: error: call to undeclared function 'basename'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> > > > 
> > > > These utilities are using this function in usage() which is used just
> > > > before program exit. Always use the basename APIs from libgen.h which is
> > > > posix implementation
> > > > 
> > > > [1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7
> > > 
> > > This passes all of our compile and lint tests presently. We don't have a
> > > Musl target in CI yet. Perhaps we should add one.
> > > 
> > > https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1138070
> > > 
> > > No particularly strong opinion on the patch, but it fixes an issue
> > > for you, and is only in one of the utils, and doesn't break CI ...
> > > 
> > > 
> > > It's a shame that this removes a const on a variable that otherwise
> > > shouldn't be modified but ... I think we can cope with that here.
> > > 
> > > In fact we have our own implementation of basename() in our base library
> > > utils probably because of this reason. But I don't think linking against
> > > libcamera-base is really required or desired/necessary for the
> > > ipu3-pack/unpack utils so ...
> > 
> > Is there a real need to compile ipu3-pack and ipu3-unpack on musl-based
> > systems ? Those are development tools that nobody has used for ages.
> 
> Maybe not, but regardless they are built as part of the whole libcamera
> build, so ensureing we continue to compile is probably a requirement.
> 
> Or we drop the utils... but I think that would be unfortunate for anyone
> who then later jumps on to the ipu3 and wants to play around with this.
> 
> But that's exactly why dropping a const here seems ... quite minor.
> 
> Do you prefer to nak this patch? Or maybe disable the build? (but let it
> bit rot?)

We could gate the utilities behind a meson option, but I think that's
overkill. Gating them behind automatic detection of a compatible version
of basename() would be a bit better, but likely overkill too. I'm OK
with the patch.

> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > 
> > > > Signed-off-by: Khem Raj <raj.khem at gmail.com>
> > > > ---
> > > >  utils/ipu3/ipu3-pack.c   | 4 ++--
> > > >  utils/ipu3/ipu3-unpack.c | 3 ++-
> > > >  2 files changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c
> > > > index decbfc6c..23d2db8b 100644
> > > > --- a/utils/ipu3/ipu3-pack.c
> > > > +++ b/utils/ipu3/ipu3-pack.c
> > > > @@ -8,6 +8,7 @@
> > > >  
> > > >  #include <errno.h>
> > > >  #include <fcntl.h>
> > > > +#include <libgen.h>
> > > >  #include <stdint.h>
> > > >  #include <stdio.h>
> > > >  #include <string.h>
> > > > @@ -15,9 +16,8 @@
> > > >  #include <sys/types.h>
> > > >  #include <unistd.h>
> > > >  
> > > > -static void usage(const char *argv0)
> > > > +static void usage(char *argv0)
> > > >  {
> > > > -
> > > >         printf("Usage: %s input-file output-file\n", basename(argv0));
> > > >         printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n");
> > > >         printf("If the output-file '-', output data will be written to standard output\n");
> > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c
> > > > index 9d2c1200..1505a970 100644
> > > > --- a/utils/ipu3/ipu3-unpack.c
> > > > +++ b/utils/ipu3/ipu3-unpack.c
> > > > @@ -8,6 +8,7 @@
> > > >  
> > > >  #include <errno.h>
> > > >  #include <fcntl.h>
> > > > +#include <libgen.h>
> > > >  #include <stdint.h>
> > > >  #include <stdio.h>
> > > >  #include <string.h>
> > > > @@ -15,7 +16,7 @@
> > > >  #include <sys/types.h>
> > > >  #include <unistd.h>
> > > >  
> > > > -static void usage(const char *argv0)
> > > > +static void usage(char *argv0)
> > > >  {
> > > >         printf("Usage: %s input-file output-file\n", basename(argv0));
> > > >         printf("Unpack the IPU3 raw Bayer format to 16-bit Bayer\n");

-- 
Regards,

Laurent Pinchart