| Message ID | 20221202132706.15675-1-jacopo@jmondi.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hello, Thank you for the patch On 12/2/22 9:27 PM, Jacopo Mondi via libcamera-devel wrote: > The "basename" function from the standard C library is defined in the > libgen.h header. > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > utils/ipu3/ipu3-pack.c | 1 + > utils/ipu3/ipu3-unpack.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > index decbfc6c397a..27af44068956 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> > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > index 9d2c1200d932..aa67e0b0ecf5 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>
Hi Jacopo, Thank you for the patch. On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote: > The "basename" function from the standard C library is defined in the > libgen.h header. > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > utils/ipu3/ipu3-pack.c | 1 + > utils/ipu3/ipu3-unpack.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > index decbfc6c397a..27af44068956 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> > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > index 9d2c1200d932..aa67e0b0ecf5 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>
Not really...
I get with clang
../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
21 | printf("Usage: %s input-file output-file\n", basename(argv0));
Sorry, I should have tested before, but it seemed trivial.
I now wonder what 'basename' we were calling, or better, to which
function prototype we were referring to, which didn't trigger such
error...
On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > The "basename" function from the standard C library is defined in the
> > libgen.h header.
> >
> > Include it to avoid errors triggered by -Wimplicit-function-declaration
> >
> > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function
> > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> > utils/ipu3/ipu3-pack.c | 1 +
> > utils/ipu3/ipu3-unpack.c | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c
> > index decbfc6c397a..27af44068956 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>
> > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c
> > index 9d2c1200d932..aa67e0b0ecf5 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>
>
> --
> Regards,
>
> Laurent Pinchart
On Fri, Dec 02, 2022 at 02:48:27PM +0100, Jacopo Mondi wrote: > Not really... > > I get with clang > > ../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] > 21 | printf("Usage: %s input-file output-file\n", basename(argv0)); > > Sorry, I should have tested before, but it seemed trivial. > > I now wonder what 'basename' we were calling, or better, to which > function prototype we were referring to, which didn't trigger such > error... From libgen.h: /* Return final component of PATH. This is the weird XPG version of this function. It sometimes will modify its argument. Therefore we normally use the GNU version (in <string.h>) and only if this header is included make the XPG version available under the real name. */ extern char *__xpg_basename (char *__path) __THROW; #define basename __xpg_basename So we're using the string.h version. The basename manpage mentions NOTES There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after #define _GNU_SOURCE /* See feature_test_macros(7) */ #include <string.h> The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/". There is no GNU version of dirname(). With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise. We have utils::basename() to support different libc versions, but we can't use it in utils/. Have you encountered this when compiling for Android ? Maybe we can disable compilation of those utilities instead ? > On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote: > > > The "basename" function from the standard C library is defined in the > > > libgen.h header. > > > > > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > > > > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > > > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > utils/ipu3/ipu3-pack.c | 1 + > > > utils/ipu3/ipu3-unpack.c | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > > > index decbfc6c397a..27af44068956 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> > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > > > index 9d2c1200d932..aa67e0b0ecf5 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>
Hi Laurent On Fri, Dec 02, 2022 at 03:59:07PM +0200, Laurent Pinchart wrote: > On Fri, Dec 02, 2022 at 02:48:27PM +0100, Jacopo Mondi wrote: > > Not really... > > > > I get with clang > > > > ../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] > > 21 | printf("Usage: %s input-file output-file\n", basename(argv0)); > > > > Sorry, I should have tested before, but it seemed trivial. > > > > I now wonder what 'basename' we were calling, or better, to which > > function prototype we were referring to, which didn't trigger such > > error... > > From libgen.h: > > /* Return final component of PATH. > > This is the weird XPG version of this function. It sometimes will > modify its argument. Therefore we normally use the GNU version (in > <string.h>) and only if this header is included make the XPG > version available under the real name. */ > extern char *__xpg_basename (char *__path) __THROW; > #define basename __xpg_basename > > So we're using the string.h version. The basename manpage mentions Spot on, I read the header file after sending the email.... > > NOTES > There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after > > #define _GNU_SOURCE /* See feature_test_macros(7) */ > #include <string.h> > > The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/". There is no GNU version of > dirname(). > > With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise. > > We have utils::basename() to support different libc versions, but we > can't use it in utils/. Have you encountered this when compiling for > Android ? Maybe we can disable compilation of those utilities instead ? > Yes, I am trying compile with the NDK. I think we can safely remove those scripts from the build (I wonder how this would be decided), but I anticipate already that we'll have a few hickups as in example: ../src/libcamera/media_device.cpp:167:2: error: use of undeclared identifier 'lockf' lockf(fd_.get(), F_ULOCK, 0); As Android has instead the BSD-conformant flock(). So expect more wrappers in utils:: (hopefully not many) > > On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote: > > > > The "basename" function from the standard C library is defined in the > > > > libgen.h header. > > > > > > > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > > > > > > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > > > > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > utils/ipu3/ipu3-pack.c | 1 + > > > > utils/ipu3/ipu3-unpack.c | 1 + > > > > 2 files changed, 2 insertions(+) > > > > > > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > > > > index decbfc6c397a..27af44068956 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> > > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > > > > index 9d2c1200d932..aa67e0b0ecf5 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> > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Dec 02, 2022 at 03:15:21PM +0100, Jacopo Mondi wrote: > On Fri, Dec 02, 2022 at 03:59:07PM +0200, Laurent Pinchart wrote: > > On Fri, Dec 02, 2022 at 02:48:27PM +0100, Jacopo Mondi wrote: > > > Not really... > > > > > > I get with clang > > > > > > ../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] > > > 21 | printf("Usage: %s input-file output-file\n", basename(argv0)); > > > > > > Sorry, I should have tested before, but it seemed trivial. > > > > > > I now wonder what 'basename' we were calling, or better, to which > > > function prototype we were referring to, which didn't trigger such > > > error... > > > > From libgen.h: > > > > /* Return final component of PATH. > > > > This is the weird XPG version of this function. It sometimes will > > modify its argument. Therefore we normally use the GNU version (in > > <string.h>) and only if this header is included make the XPG > > version available under the real name. */ > > extern char *__xpg_basename (char *__path) __THROW; > > #define basename __xpg_basename > > > > So we're using the string.h version. The basename manpage mentions > > Spot on, I read the header file after sending the email.... > > > > > NOTES > > There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after > > > > #define _GNU_SOURCE /* See feature_test_macros(7) */ > > #include <string.h> > > > > The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/". There is no GNU version of > > dirname(). > > > > With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise. > > > > We have utils::basename() to support different libc versions, but we > > can't use it in utils/. Have you encountered this when compiling for > > Android ? Maybe we can disable compilation of those utilities instead ? > > > > Yes, I am trying compile with the NDK. > > I think we can safely remove those scripts from the build (I wonder > how this would be decided), Maybe a meson option ? Or doing so automatically based on a test at configuration time to see if there's a compliant basename() ? Or even a check on the platform type, if that's available. > but I anticipate already that we'll have a > few hickups as in example: > > ../src/libcamera/media_device.cpp:167:2: error: use of undeclared identifier 'lockf' > lockf(fd_.get(), F_ULOCK, 0); > > As Android has instead the BSD-conformant flock(). So expect more > wrappers in utils:: (hopefully not many) We could skip the whole locking on Android, as there will never be multiple processes trying to use the same camera. However, the lockf() issue seems weird, it seems to be available in bionic. > > > On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote: > > > > Hi Jacopo, > > > > > > > > Thank you for the patch. > > > > > > > > On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote: > > > > > The "basename" function from the standard C library is defined in the > > > > > libgen.h header. > > > > > > > > > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > > > > > > > > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > > > > > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > --- > > > > > utils/ipu3/ipu3-pack.c | 1 + > > > > > utils/ipu3/ipu3-unpack.c | 1 + > > > > > 2 files changed, 2 insertions(+) > > > > > > > > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > > > > > index decbfc6c397a..27af44068956 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> > > > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > > > > > index 9d2c1200d932..aa67e0b0ecf5 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>
Hi Laurent On Fri, Dec 02, 2022 at 05:34:15PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Dec 02, 2022 at 03:15:21PM +0100, Jacopo Mondi wrote: > > On Fri, Dec 02, 2022 at 03:59:07PM +0200, Laurent Pinchart wrote: > > > On Fri, Dec 02, 2022 at 02:48:27PM +0100, Jacopo Mondi wrote: > > > > Not really... > > > > > > > > I get with clang > > > > > > > > ../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] > > > > 21 | printf("Usage: %s input-file output-file\n", basename(argv0)); > > > > > > > > Sorry, I should have tested before, but it seemed trivial. > > > > > > > > I now wonder what 'basename' we were calling, or better, to which > > > > function prototype we were referring to, which didn't trigger such > > > > error... > > > > > > From libgen.h: > > > > > > /* Return final component of PATH. > > > > > > This is the weird XPG version of this function. It sometimes will > > > modify its argument. Therefore we normally use the GNU version (in > > > <string.h>) and only if this header is included make the XPG > > > version available under the real name. */ > > > extern char *__xpg_basename (char *__path) __THROW; > > > #define basename __xpg_basename > > > > > > So we're using the string.h version. The basename manpage mentions > > > > Spot on, I read the header file after sending the email.... > > > > > > > > NOTES > > > There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after > > > > > > #define _GNU_SOURCE /* See feature_test_macros(7) */ > > > #include <string.h> > > > > > > The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/". There is no GNU version of > > > dirname(). > > > > > > With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise. > > > > > > We have utils::basename() to support different libc versions, but we > > > can't use it in utils/. Have you encountered this when compiling for > > > Android ? Maybe we can disable compilation of those utilities instead ? > > > > > > > Yes, I am trying compile with the NDK. > > > > I think we can safely remove those scripts from the build (I wonder > > how this would be decided), > > Maybe a meson option ? Or doing so automatically based on a test at > configuration time to see if there's a compliant basename() ? Or even a > check on the platform type, if that's available. > We could indeed have a build option to compile the several platform specific utilities. I can list utils/ipu3 and utils/rkisp1 while utils/raspberry which contains CTT should probably be built unconditionally ? > > but I anticipate already that we'll have a > > few hickups as in example: > > > > ../src/libcamera/media_device.cpp:167:2: error: use of undeclared identifier 'lockf' > > lockf(fd_.get(), F_ULOCK, 0); > > > > As Android has instead the BSD-conformant flock(). So expect more > > wrappers in utils:: (hopefully not many) > > We could skip the whole locking on Android, as there will never be > multiple processes trying to use the same camera. However, the lockf() > issue seems weird, it seems to be available in bionic. > Only after API version 24 as far as I can tell, at least in NDK #if __ANDROID_API__ >= 24 int lockf(int __fd, int __cmd, off_t __length) __RENAME_IF_FILE_OFFSET64(lockf64) __INTRODUCED_IN(24); /** * Like lockf() but allows using a 64-bit length * even from a 32-bit process without `__FILE_OFFSET_BITS=64`. */ int lockf64(int __fd, int __cmd, off64_t __length) __INTRODUCED_IN(24); #endif /* __ANDROID_API__ >= 24 */ I'll shortly send patches... > > > > On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote: > > > > > Hi Jacopo, > > > > > > > > > > Thank you for the patch. > > > > > > > > > > On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote: > > > > > > The "basename" function from the standard C library is defined in the > > > > > > libgen.h header. > > > > > > > > > > > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > > > > > > > > > > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > > > > > > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > > --- > > > > > > utils/ipu3/ipu3-pack.c | 1 + > > > > > > utils/ipu3/ipu3-unpack.c | 1 + > > > > > > 2 files changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > > > > > > index decbfc6c397a..27af44068956 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> > > > > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > > > > > > index 9d2c1200d932..aa67e0b0ecf5 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> > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Dec 02, 2022 at 04:59:14PM +0100, Jacopo Mondi wrote: > On Fri, Dec 02, 2022 at 05:34:15PM +0200, Laurent Pinchart wrote: > > On Fri, Dec 02, 2022 at 03:15:21PM +0100, Jacopo Mondi wrote: > > > On Fri, Dec 02, 2022 at 03:59:07PM +0200, Laurent Pinchart wrote: > > > > On Fri, Dec 02, 2022 at 02:48:27PM +0100, Jacopo Mondi wrote: > > > > > Not really... > > > > > > > > > > I get with clang > > > > > > > > > > ../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] > > > > > 21 | printf("Usage: %s input-file output-file\n", basename(argv0)); > > > > > > > > > > Sorry, I should have tested before, but it seemed trivial. > > > > > > > > > > I now wonder what 'basename' we were calling, or better, to which > > > > > function prototype we were referring to, which didn't trigger such > > > > > error... > > > > > > > > From libgen.h: > > > > > > > > /* Return final component of PATH. > > > > > > > > This is the weird XPG version of this function. It sometimes will > > > > modify its argument. Therefore we normally use the GNU version (in > > > > <string.h>) and only if this header is included make the XPG > > > > version available under the real name. */ > > > > extern char *__xpg_basename (char *__path) __THROW; > > > > #define basename __xpg_basename > > > > > > > > So we're using the string.h version. The basename manpage mentions > > > > > > Spot on, I read the header file after sending the email.... > > > > > > > > > > > NOTES > > > > There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after > > > > > > > > #define _GNU_SOURCE /* See feature_test_macros(7) */ > > > > #include <string.h> > > > > > > > > The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/". There is no GNU version of > > > > dirname(). > > > > > > > > With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise. > > > > > > > > We have utils::basename() to support different libc versions, but we > > > > can't use it in utils/. Have you encountered this when compiling for > > > > Android ? Maybe we can disable compilation of those utilities instead ? > > > > > > > > > > Yes, I am trying compile with the NDK. > > > > > > I think we can safely remove those scripts from the build (I wonder > > > how this would be decided), > > > > Maybe a meson option ? Or doing so automatically based on a test at > > configuration time to see if there's a compliant basename() ? Or even a > > check on the platform type, if that's available. > > We could indeed have a build option to compile the several platform > specific utilities. I can list utils/ipu3 and utils/rkisp1 while > utils/raspberry which contains CTT should probably be built > unconditionally ? I'd skip any C/C++ tools from utils completely on Android to be honest, I don't think any of them will be run on an Android device. > > > but I anticipate already that we'll have a > > > few hickups as in example: > > > > > > ../src/libcamera/media_device.cpp:167:2: error: use of undeclared identifier 'lockf' > > > lockf(fd_.get(), F_ULOCK, 0); > > > > > > As Android has instead the BSD-conformant flock(). So expect more > > > wrappers in utils:: (hopefully not many) > > > > We could skip the whole locking on Android, as there will never be > > multiple processes trying to use the same camera. However, the lockf() > > issue seems weird, it seems to be available in bionic. > > Only after API version 24 as far as I can tell, at least in NDK > > #if __ANDROID_API__ >= 24 > int lockf(int __fd, int __cmd, off_t __length) __RENAME_IF_FILE_OFFSET64(lockf64) __INTRODUCED_IN(24); > > /** > * Like lockf() but allows using a 64-bit length > * even from a 32-bit process without `__FILE_OFFSET_BITS=64`. > */ > int lockf64(int __fd, int __cmd, off64_t __length) __INTRODUCED_IN(24); > #endif /* __ANDROID_API__ >= 24 */ > > I'll shortly send patches... > > > > > > On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote: > > > > > > Hi Jacopo, > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote: > > > > > > > The "basename" function from the standard C library is defined in the > > > > > > > libgen.h header. > > > > > > > > > > > > > > Include it to avoid errors triggered by -Wimplicit-function-declaration > > > > > > > > > > > > > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function > > > > > > > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > > > > --- > > > > > > > utils/ipu3/ipu3-pack.c | 1 + > > > > > > > utils/ipu3/ipu3-unpack.c | 1 + > > > > > > > 2 files changed, 2 insertions(+) > > > > > > > > > > > > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c > > > > > > > index decbfc6c397a..27af44068956 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> > > > > > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c > > > > > > > index 9d2c1200d932..aa67e0b0ecf5 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>
diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c index decbfc6c397a..27af44068956 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> diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c index 9d2c1200d932..aa67e0b0ecf5 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>
The "basename" function from the standard C library is defined in the libgen.h header. Include it to avoid errors triggered by -Wimplicit-function-declaration ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration] Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- utils/ipu3/ipu3-pack.c | 1 + utils/ipu3/ipu3-unpack.c | 1 + 2 files changed, 2 insertions(+)