| Message ID | 20240324183307.3833076-1-raj.khem@gmail.com |
|---|---|
| State | Accepted |
| Commit | f4d416db9141e7a7802aac00a87500a524c11e37 |
| Headers | show |
| Series |
|
| Related | show |
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 ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Khem Raj <raj.khem@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"); > -- > 2.44.0 >
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. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Khem Raj <raj.khem@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");
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?) -- Kieran > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Khem Raj <raj.khem@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
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@ideasonboard.com> > > > > > > > Signed-off-by: Khem Raj <raj.khem@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");
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");
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 Signed-off-by: Khem Raj <raj.khem@gmail.com> --- utils/ipu3/ipu3-pack.c | 4 ++-- utils/ipu3/ipu3-unpack.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-)