clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 2838 bytes --]

Hi,

Since clar unit testing framework was imported by commit 9b7caa2809cb (t:
import the clar unit testing framework, 2024-09-04), Git FTBFS on uclibc
systems built by Buildroot:

```
    CC t/unit-tests/unit-test.o
t/unit-tests/clar/clar.c: In function 'clar__assert_equal':
t/unit-tests/clar/clar.c:767:23: error: unknown type name 'wchar_t'
  767 |                 const wchar_t *wcs1 = va_arg(args, const wchar_t *);
      |                       ^~~~~~~
In file included from t/unit-tests/clar/clar.c:13:
t/unit-tests/clar/clar.c:767:58: error: unknown type name 'wchar_t'
  767 |                 const wchar_t *wcs1 = va_arg(args, const wchar_t *);
      |                                                          ^~~~~~~
t/unit-tests/clar/clar.c:768:23: error: unknown type name 'wchar_t'
  768 |                 const wchar_t *wcs2 = va_arg(args, const wchar_t *);
      |                       ^~~~~~~
t/unit-tests/clar/clar.c:768:58: error: unknown type name 'wchar_t'
  768 |                 const wchar_t *wcs2 = va_arg(args, const wchar_t *);
      |                                                          ^~~~~~~
t/unit-tests/clar/clar.c:769:65: warning: implicit declaration of function 'wcscmp' [-Wimplicit-function-declaration]
  769 |                 is_equal = (!wcs1 || !wcs2) ? (wcs1 == wcs2) : !wcscmp(wcs1, wcs2);
      |                                                                 ^~~~~~
t/unit-tests/clar/clar.c:784:23: error: unknown type name 'wchar_t'
  784 |                 const wchar_t *wcs1 = va_arg(args, const wchar_t *);
      |                       ^~~~~~~
t/unit-tests/clar/clar.c:784:58: error: unknown type name 'wchar_t'
  784 |                 const wchar_t *wcs1 = va_arg(args, const wchar_t *);
      |                                                          ^~~~~~~
t/unit-tests/clar/clar.c:785:23: error: unknown type name 'wchar_t'
  785 |                 const wchar_t *wcs2 = va_arg(args, const wchar_t *);
      |                       ^~~~~~~
t/unit-tests/clar/clar.c:785:58: error: unknown type name 'wchar_t'
  785 |                 const wchar_t *wcs2 = va_arg(args, const wchar_t *);
      |                                                          ^~~~~~~
t/unit-tests/clar/clar.c:787:65: warning: implicit declaration of function 'wcsncmp' [-Wimplicit-function-declaration]
  787 |                 is_equal = (!wcs1 || !wcs2) ? (wcs1 == wcs2) : !wcsncmp(wcs1, wcs2, len);
      |                                                                 ^~~~~~~
make[1]: *** [Makefile:2795: t/unit-tests/clar/clar.o] Error 1
```

See [1] for the full build log.

Thanks.

[1]: https://autobuild.buildroot.org/results/8cc9795dc18277926dd386eb1cb9f8c9b65b0042/build-end.log

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Patrick Steinhardt]

On Thu, Oct 17, 2024 at 10:51:05AM +0700, Bagas Sanjaya wrote:
> Hi,
> 
> Since clar unit testing framework was imported by commit 9b7caa2809cb (t:
> import the clar unit testing framework, 2024-09-04), Git FTBFS on uclibc
> systems built by Buildroot:

Wait a second, that doesn't sound right to me. `wchar_t` is part of ISO
C90, so any system not supporting it would basically be unsupported by
us from my point of view. And indeed, uclibc _does_ support that type
alright. I guess the issue is rather that we're relying on some kind of
platform-specific behaviour and thus don't include the correct header.

I'll have a look, thanks for the report!

Patrick

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Patrick Steinhardt]

On Thu, Oct 17, 2024 at 03:33:51PM +0200, Patrick Steinhardt wrote:
> On Thu, Oct 17, 2024 at 10:51:05AM +0700, Bagas Sanjaya wrote:
> > Hi,
> > 
> > Since clar unit testing framework was imported by commit 9b7caa2809cb (t:
> > import the clar unit testing framework, 2024-09-04), Git FTBFS on uclibc
> > systems built by Buildroot:
> 
> Wait a second, that doesn't sound right to me. `wchar_t` is part of ISO
> C90, so any system not supporting it would basically be unsupported by
> us from my point of view. And indeed, uclibc _does_ support that type
> alright. I guess the issue is rather that we're relying on some kind of
> platform-specific behaviour and thus don't include the correct header.
> 
> I'll have a look, thanks for the report!

Okay, uclibc indeed has _optional_ support for `wchar_t`. But what
really throws me off: "include/wchar.h" from uclibc has the following
snippet right at the top:

    #ifndef __UCLIBC_HAS_WCHAR__
    #error Attempted to include wchar.h when uClibc built without wide char support.
    #endif

We unconditionally include <wchar.h>, and your system does not seem to
have support for it built in. So why doesn't the `#error` trigger? It's
also not like this is a recent error, it has been added with 581deed72
(The obligatory forgotten files..., 2002-05-06).

We can do something like the below patch in clar, but I'd first like to
understand why your platform seems to be broken in such a way.

Patrick

diff --git a/clar.c b/clar.c
index 64879cf..06fe3d1 100644
--- a/clar.c
+++ b/clar.c
@@ -9,6 +9,11 @@
 #define _DARWIN_C_SOURCE
 #define _DEFAULT_SOURCE
 
+#if defined(__UCLIBC__) && ! defined(__UCLIBC_HAS_WCHAR__)
+#else
+#	define HAVE_WCHAR
+#endif
+
 #include <errno.h>
 #include <setjmp.h>
 #include <stdlib.h>
@@ -16,7 +21,9 @@
 #include <string.h>
 #include <math.h>
 #include <stdarg.h>
+#ifdef HAVE_WCHAR
 #include <wchar.h>
+#endif
 #include <time.h>
 #include <inttypes.h>
 
@@ -766,6 +773,7 @@ void clar__assert_equal(
 			}
 		}
 	}
+#ifdef HAVE_WCHAR
 	else if (!strcmp("%ls", fmt)) {
 		const wchar_t *wcs1 = va_arg(args, const wchar_t *);
 		const wchar_t *wcs2 = va_arg(args, const wchar_t *);
@@ -801,6 +809,7 @@ void clar__assert_equal(
 			}
 		}
 	}
+#endif // HAVE_WCHAR
 	else if (!strcmp("%"PRIuMAX, fmt) || !strcmp("%"PRIxMAX, fmt)) {
 		uintmax_t sz1 = va_arg(args, uintmax_t), sz2 = va_arg(args, uintmax_t);
 		is_equal = (sz1 == sz2);

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Taylor Blau]

On Thu, Oct 17, 2024 at 03:54:28PM +0200, Patrick Steinhardt wrote:
> We can do something like the below patch in clar, but I'd first like to
> understand why your platform seems to be broken in such a way.

Thanks, both, for the report and investigation.

Thanks,
Taylor

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

On 2024-10-17 at 13:54:28, Patrick Steinhardt wrote:
> On Thu, Oct 17, 2024 at 03:33:51PM +0200, Patrick Steinhardt wrote:
> > On Thu, Oct 17, 2024 at 10:51:05AM +0700, Bagas Sanjaya wrote:
> > > Hi,
> > > 
> > > Since clar unit testing framework was imported by commit 9b7caa2809cb (t:
> > > import the clar unit testing framework, 2024-09-04), Git FTBFS on uclibc
> > > systems built by Buildroot:
> > 
> > Wait a second, that doesn't sound right to me. `wchar_t` is part of ISO
> > C90, so any system not supporting it would basically be unsupported by
> > us from my point of view. And indeed, uclibc _does_ support that type
> > alright. I guess the issue is rather that we're relying on some kind of
> > platform-specific behaviour and thus don't include the correct header.
> > 
> > I'll have a look, thanks for the report!
> 
> Okay, uclibc indeed has _optional_ support for `wchar_t`. But what
> really throws me off: "include/wchar.h" from uclibc has the following
> snippet right at the top:
> 
>     #ifndef __UCLIBC_HAS_WCHAR__
>     #error Attempted to include wchar.h when uClibc built without wide char support.
>     #endif
> 
> We unconditionally include <wchar.h>, and your system does not seem to
> have support for it built in. So why doesn't the `#error` trigger? It's
> also not like this is a recent error, it has been added with 581deed72
> (The obligatory forgotten files..., 2002-05-06).
> 
> We can do something like the below patch in clar, but I'd first like to
> understand why your platform seems to be broken in such a way.

Yeah, this is definitely broken.  We require ISO C99, and according to
the draft preceding the ratification[0], `wchar.h` and its contents are not
optional.  The similar draft for C11 also doesn't appear to make these
optional.

I think users of uclibc will need to compile it with full ISO C99
support.  I expect that a wide variety of other software will be
similarly broken without that.

[0] Chosen because it is available for at no charge and the standard is not.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Jeff King]

On Thu, Oct 17, 2024 at 10:21:43PM +0000, brian m. carlson wrote:

> > We unconditionally include <wchar.h>, and your system does not seem to
> > have support for it built in. So why doesn't the `#error` trigger? It's
> > also not like this is a recent error, it has been added with 581deed72
> > (The obligatory forgotten files..., 2002-05-06).
> > 
> > We can do something like the below patch in clar, but I'd first like to
> > understand why your platform seems to be broken in such a way.
> 
> Yeah, this is definitely broken.  We require ISO C99, and according to
> the draft preceding the ratification[0], `wchar.h` and its contents are not
> optional.  The similar draft for C11 also doesn't appear to make these
> optional.
> 
> I think users of uclibc will need to compile it with full ISO C99
> support.  I expect that a wide variety of other software will be
> similarly broken without that.

Perhaps, but...don't the current releases of Git work just fine on such
a wchar-less uclibc system now? We don't use wchar or include wchar.h
ourselves, except on Windows or via compat/regex (though it is even
conditional there). This is a new portability problem introduced by the
clar test harness. And even there I doubt it is something we care about
(it looks like it's for allowing "%ls" in assertions).

Our approach to portability has traditionally been a cost/benefit for
individual features. Standards are a nice guideline, but the real world
does not always follow them. Sometimes accommodating platforms that
don't strictly follow the standard is cheap enough that it's worth
doing.

I think more recent discussions have trended to looking at standards in
a bit stronger way: giving minimum requirements and sticking to them.
Certainly I'm sympathetic to that viewpoint, as it can reduce noise.

But IMHO this is a good example of where the flexibility of the first
approach shines. We could accommodate this platform without any real
cost (and indeed, we should be able to _drop_ some clar code).

-Peff

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 12:51:55AM -0400, Jeff King wrote:
> On Thu, Oct 17, 2024 at 10:21:43PM +0000, brian m. carlson wrote:
> 
> > > We unconditionally include <wchar.h>, and your system does not seem to
> > > have support for it built in. So why doesn't the `#error` trigger? It's
> > > also not like this is a recent error, it has been added with 581deed72
> > > (The obligatory forgotten files..., 2002-05-06).
> > > 
> > > We can do something like the below patch in clar, but I'd first like to
> > > understand why your platform seems to be broken in such a way.
> > 
> > Yeah, this is definitely broken.  We require ISO C99, and according to
> > the draft preceding the ratification[0], `wchar.h` and its contents are not
> > optional.  The similar draft for C11 also doesn't appear to make these
> > optional.
> > 
> > I think users of uclibc will need to compile it with full ISO C99
> > support.  I expect that a wide variety of other software will be
> > similarly broken without that.
> 
> Perhaps, but...don't the current releases of Git work just fine on such
> a wchar-less uclibc system now? We don't use wchar or include wchar.h
> ourselves, except on Windows or via compat/regex (though it is even
> conditional there). This is a new portability problem introduced by the
> clar test harness. And even there I doubt it is something we care about
> (it looks like it's for allowing "%ls" in assertions).
> 
> Our approach to portability has traditionally been a cost/benefit for
> individual features. Standards are a nice guideline, but the real world
> does not always follow them. Sometimes accommodating platforms that
> don't strictly follow the standard is cheap enough that it's worth
> doing.
> 
> I think more recent discussions have trended to looking at standards in
> a bit stronger way: giving minimum requirements and sticking to them.
> Certainly I'm sympathetic to that viewpoint, as it can reduce noise.
> 
> But IMHO this is a good example of where the flexibility of the first
> approach shines. We could accommodate this platform without any real
> cost (and indeed, we should be able to _drop_ some clar code).

Well, dropping doesn't work as it breaks other projects that depend on
the clar-features that depend on `wchar_t`. But other than that I agree
and would like to fix this issue, also because it potentially benefits
other users of the clar.

The only problem is that the platform seems to be severely broken. As
mentioned elsewhere, we have this snippet in uclibc's "wchar.h":

    #ifndef __UCLIBC_HAS_WCHAR__
    #error Attempted to include wchar.h when uClibc built without wide char support.
    #endif

So if __UCLIBC_HAS_WCHAR__ is not defined, we should see the error. But
the report didn't show the error, which means that the define has to be
set. And consequently we have no way to patch around this, because the
macro that we're supposed to use is broken.

Might be I'm missing something with how uclibc is intended to work. But
if I'm right then this is just a broken platform, and I don't think
working around it would be sensible. Otherwise I'd be happy to make the
wchar-related code conditional as shown in the preliminary patch posted
in [1].

Patrick

[1]: <ZxEXFI80i4Q_4NJT@pks.im>

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Jeff King]

On Fri, Oct 18, 2024 at 06:59:17AM +0200, Patrick Steinhardt wrote:

> > But IMHO this is a good example of where the flexibility of the first
> > approach shines. We could accommodate this platform without any real
> > cost (and indeed, we should be able to _drop_ some clar code).
> 
> Well, dropping doesn't work as it breaks other projects that depend on
> the clar-features that depend on `wchar_t`. But other than that I agree
> and would like to fix this issue, also because it potentially benefits
> other users of the clar.

So that's a rabbit hole I didn't go down in my other message. ;)

But another traditional philosophy the Git project has had is to be very
conservative in our dependencies. And now we have this new dependency,
and already it is causing a portability problem.

I don't think that means we should throw away the dependency. But if we
are inheriting portability problems from imported code, I think we
should consider to what degree we can lightly tweak that code to match
our project. I don't care what clar does upstream. If _we_ don't need
wchar support, we can drop it or #ifdef it out.

Overall, I'm a little sad to see all of the #includes in clar.c. We have
spent 20 years building up git-compat-util.h to meet our needs for
portability, and there are lots of subtle bits in there about what is
included and when, along with various wrappers. And now we have a new
subsystem which doesn't use _any_ of that, and has its own set of
includes and wrappers. It seems inevitable that we are going to run into
cases where a platform we support isn't handled by clar, or that we'll
have to duplicate our solution in both places. I wish it were just using
git-compat-util.h. I know that means essentially forking, but I think I
may prefer that to inheriting some other project's portability problems.

> The only problem is that the platform seems to be severely broken. As
> mentioned elsewhere, we have this snippet in uclibc's "wchar.h":
> 
>     #ifndef __UCLIBC_HAS_WCHAR__
>     #error Attempted to include wchar.h when uClibc built without wide char support.
>     #endif

Yeah, I have no clue what's going on there. Certainly I have no problem
if you want to dig further to get confidence in the direction we choose.

-Peff

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 01:24:48AM -0400, Jeff King wrote:
> On Fri, Oct 18, 2024 at 06:59:17AM +0200, Patrick Steinhardt wrote:
> 
> > > But IMHO this is a good example of where the flexibility of the first
> > > approach shines. We could accommodate this platform without any real
> > > cost (and indeed, we should be able to _drop_ some clar code).
> > 
> > Well, dropping doesn't work as it breaks other projects that depend on
> > the clar-features that depend on `wchar_t`. But other than that I agree
> > and would like to fix this issue, also because it potentially benefits
> > other users of the clar.
> 
> So that's a rabbit hole I didn't go down in my other message. ;)
> 
> But another traditional philosophy the Git project has had is to be very
> conservative in our dependencies. And now we have this new dependency,
> and already it is causing a portability problem.
> 
> I don't think that means we should throw away the dependency. But if we
> are inheriting portability problems from imported code, I think we
> should consider to what degree we can lightly tweak that code to match
> our project. I don't care what clar does upstream. If _we_ don't need
> wchar support, we can drop it or #ifdef it out.
> 
> Overall, I'm a little sad to see all of the #includes in clar.c. We have
> spent 20 years building up git-compat-util.h to meet our needs for
> portability, and there are lots of subtle bits in there about what is
> included and when, along with various wrappers. And now we have a new
> subsystem which doesn't use _any_ of that, and has its own set of
> includes and wrappers. It seems inevitable that we are going to run into
> cases where a platform we support isn't handled by clar, or that we'll
> have to duplicate our solution in both places. I wish it were just using
> git-compat-util.h. I know that means essentially forking, but I think I
> may prefer that to inheriting some other project's portability problems.

Well, I'm of a different mind here. It sure is more work for now, and I
have been chipping away at the issues. But in the end, it's not only us
who benefit, but the overall ecosystem because others can use clar on
more or less esoteric platforms, too. It's part of the reason why I have
been advocating for clar in the first place: we have a good relationship
to its maintainers, so it is easy to upstream changes.

So yes, right now we feel a bit of pain there. But that's going to go
away, and from thereon everyone benefits.

> > The only problem is that the platform seems to be severely broken. As
> > mentioned elsewhere, we have this snippet in uclibc's "wchar.h":
> > 
> >     #ifndef __UCLIBC_HAS_WCHAR__
> >     #error Attempted to include wchar.h when uClibc built without wide char support.
> >     #endif
> 
> Yeah, I have no clue what's going on there. Certainly I have no problem
> if you want to dig further to get confidence in the direction we choose.

Yup, I'll do that, but first need additional input from the reporter. I
don't have a uclibc platform, and couldn't really find obvious Docker
images to reproduce the issue with.

Patrick

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Taylor Blau]

On Fri, Oct 18, 2024 at 07:31:00AM +0200, Patrick Steinhardt wrote:
> > Overall, I'm a little sad to see all of the #includes in clar.c. We have
> > spent 20 years building up git-compat-util.h to meet our needs for
> > portability, and there are lots of subtle bits in there about what is
> > included and when, along with various wrappers. And now we have a new
> > subsystem which doesn't use _any_ of that, and has its own set of
> > includes and wrappers. It seems inevitable that we are going to run into
> > cases where a platform we support isn't handled by clar, or that we'll
> > have to duplicate our solution in both places. I wish it were just using
> > git-compat-util.h. I know that means essentially forking, but I think I
> > may prefer that to inheriting some other project's portability problems.
>
> Well, I'm of a different mind here. It sure is more work for now, and I
> have been chipping away at the issues. But in the end, it's not only us
> who benefit, but the overall ecosystem because others can use clar on
> more or less esoteric platforms, too. It's part of the reason why I have
> been advocating for clar in the first place: we have a good relationship
> to its maintainers, so it is easy to upstream changes.
>
> So yes, right now we feel a bit of pain there. But that's going to go
> away, and from thereon everyone benefits.

I'm not sure I agree wholly here. In particular, I think saying "a bit"
of pain may not be capturing the full picture.

Will it take us another 20 years to resolve all of the portability
issues which Clar suffers from (but git-compat-util.h doesn't)?
Probably not 20 years, but I don't think that it's on the complete other
end of the spectrum, either.

TBH, I would not at all be sad to see us "fork" Clar into our own tree
or have a new repository upstream that we track with submodules, etc.

Thanks,
Taylor

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 04:50:52PM -0400, Taylor Blau wrote:
> On Fri, Oct 18, 2024 at 07:31:00AM +0200, Patrick Steinhardt wrote:
> > > Overall, I'm a little sad to see all of the #includes in clar.c. We have
> > > spent 20 years building up git-compat-util.h to meet our needs for
> > > portability, and there are lots of subtle bits in there about what is
> > > included and when, along with various wrappers. And now we have a new
> > > subsystem which doesn't use _any_ of that, and has its own set of
> > > includes and wrappers. It seems inevitable that we are going to run into
> > > cases where a platform we support isn't handled by clar, or that we'll
> > > have to duplicate our solution in both places. I wish it were just using
> > > git-compat-util.h. I know that means essentially forking, but I think I
> > > may prefer that to inheriting some other project's portability problems.
> >
> > Well, I'm of a different mind here. It sure is more work for now, and I
> > have been chipping away at the issues. But in the end, it's not only us
> > who benefit, but the overall ecosystem because others can use clar on
> > more or less esoteric platforms, too. It's part of the reason why I have
> > been advocating for clar in the first place: we have a good relationship
> > to its maintainers, so it is easy to upstream changes.
> >
> > So yes, right now we feel a bit of pain there. But that's going to go
> > away, and from thereon everyone benefits.
> 
> I'm not sure I agree wholly here. In particular, I think saying "a bit"
> of pain may not be capturing the full picture.
> 
> Will it take us another 20 years to resolve all of the portability
> issues which Clar suffers from (but git-compat-util.h doesn't)?
> Probably not 20 years, but I don't think that it's on the complete other
> end of the spectrum, either.

My assumption is that we'll iron out the issues in this release. Our
"git-compat-util.h" has grown _huge_, but that's mostly because it needs
to support a ton of different things. The Git codebase is orders of
magnitude bigger than the clar, so it is totally expected that it also
exercises way more edge cases in C. Conversely, I expect that the compat
headers in clar need to only be a fraction of what we have.

I don't really understand where the claim comes from that this is such a
huge pain. Sure, there's been a bit of back and forth now. But all of
the reports I received were easy to fix, and I've fixed them upstream in
a matter of days.

I'd really like us to take a step back here and take things a bit more
relaxed. If we see that this continues to be a major pain to maintain
then yes, I agree, we should likely rope in our own compat headers. But
from my point of view there isn't really indication that this is going
to be the case.

> TBH, I would not at all be sad to see us "fork" Clar into our own tree
> or have a new repository upstream that we track with submodules, etc.

I don't see any reason for this. If we eventually figure out that we
want to rope in our own compat headers we can wire up support for this
without forking clar.

In any case, I'll reroll my patch series that updates the clar today or
tomorrow to incorporate a fix for the reported issue.

Patrick

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Jeff King]

On Mon, Oct 21, 2024 at 08:44:01AM +0200, Patrick Steinhardt wrote:

> > Will it take us another 20 years to resolve all of the portability
> > issues which Clar suffers from (but git-compat-util.h doesn't)?
> > Probably not 20 years, but I don't think that it's on the complete other
> > end of the spectrum, either.
> 
> My assumption is that we'll iron out the issues in this release. Our
> "git-compat-util.h" has grown _huge_, but that's mostly because it needs
> to support a ton of different things. The Git codebase is orders of
> magnitude bigger than the clar, so it is totally expected that it also
> exercises way more edge cases in C. Conversely, I expect that the compat
> headers in clar need to only be a fraction of what we have.
> 
> I don't really understand where the claim comes from that this is such a
> huge pain. Sure, there's been a bit of back and forth now. But all of
> the reports I received were easy to fix, and I've fixed them upstream in
> a matter of days.
> 
> I'd really like us to take a step back here and take things a bit more
> relaxed. If we see that this continues to be a major pain to maintain
> then yes, I agree, we should likely rope in our own compat headers. But
> from my point of view there isn't really indication that this is going
> to be the case.

I'm OK with that direction. Just to be clear, I think you've done a
great job (as you always do) of responding to the issue promptly and
keeping things moving forward. And you're right that there is a good
chance that we iron out this wrinkle and never worry about it again. If
that doesn't turn out to be the case, we can iterate from there.

My thinking / response was mostly just: git-compat-util.h has many
subtle fixes we've accumulated through battle-testing. To the point that
I don't think we even know which ones are important and which are not
anymore. That's why our guidelines say that everything should include it
first, rather than trying to handle system headers themselves. Clar
violates that rule, and if it were original code within Git it probably
would have been flagged in review as such. But since it's imported,
there's some tension there between making the code as Git-like as
possible, and keeping it easy to track upstream.

I tend to err on the "fork and make it Git-like" side of that line, but
certainly there is an argument for the other way. Anyway, let's deal
with this wchar.h issue and then see if it ever even comes up again.

-Peff

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Taylor Blau]

On Mon, Oct 21, 2024 at 03:30:24PM -0400, Jeff King wrote:
> On Mon, Oct 21, 2024 at 08:44:01AM +0200, Patrick Steinhardt wrote:
>
> > > Will it take us another 20 years to resolve all of the portability
> > > issues which Clar suffers from (but git-compat-util.h doesn't)?
> > > Probably not 20 years, but I don't think that it's on the complete other
> > > end of the spectrum, either.
> >
> > My assumption is that we'll iron out the issues in this release. Our
> > "git-compat-util.h" has grown _huge_, but that's mostly because it needs
> > to support a ton of different things. The Git codebase is orders of
> > magnitude bigger than the clar, so it is totally expected that it also
> > exercises way more edge cases in C. Conversely, I expect that the compat
> > headers in clar need to only be a fraction of what we have.
> >
> > I don't really understand where the claim comes from that this is such a
> > huge pain. Sure, there's been a bit of back and forth now. But all of
> > the reports I received were easy to fix, and I've fixed them upstream in
> > a matter of days.
> >
> > I'd really like us to take a step back here and take things a bit more
> > relaxed. If we see that this continues to be a major pain to maintain
> > then yes, I agree, we should likely rope in our own compat headers. But
> > from my point of view there isn't really indication that this is going
> > to be the case.
>
> I'm OK with that direction. Just to be clear, I think you've done a
> great job (as you always do) of responding to the issue promptly and
> keeping things moving forward. And you're right that there is a good
> chance that we iron out this wrinkle and never worry about it again. If
> that doesn't turn out to be the case, we can iterate from there.

Yeah, to be clear on my own position, I agree with Peff here. I was
merely suggesting that there might be more work here than we estimate,
and that it would be nice to take advantage of the experience and years
of work that have gone into git-compat-util.h if that were the case.

Certainly you have done a great job at responding promptly to any
breakages, which is greatly appreciated by myself and the project.

Thanks,
Taylor

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3321 bytes --]

On 2024-10-18 at 04:51:55, Jeff King wrote:
> Perhaps, but...don't the current releases of Git work just fine on such
> a wchar-less uclibc system now? We don't use wchar or include wchar.h
> ourselves, except on Windows or via compat/regex (though it is even
> conditional there). This is a new portability problem introduced by the
> clar test harness. And even there I doubt it is something we care about
> (it looks like it's for allowing "%ls" in assertions).
> 
> Our approach to portability has traditionally been a cost/benefit for
> individual features. Standards are a nice guideline, but the real world
> does not always follow them. Sometimes accommodating platforms that
> don't strictly follow the standard is cheap enough that it's worth
> doing.
> 
> I think more recent discussions have trended to looking at standards in
> a bit stronger way: giving minimum requirements and sticking to them.
> Certainly I'm sympathetic to that viewpoint, as it can reduce noise.

I think there's a tradeoff here in what is reasonable to support.  For
example, we know FreeBSD and NetBSD return something than the
POSIX-mandated ELOOP for an open(2) on a symlink with O_NOFOLLOW.  This
is really minor to work around, and overall, both operating systems
overwhelmingly are easy to support and run almost all POSIX-compatible
software with ease and support a modern POSIX version.

And then we have uclibc, which has decided to optionally fail to
implement obligatory functionality that has been around for 35 years,
which is longer than some of my colleagues have been alive.  This isn't
even that it doesn't implement all of the POSIX functionality, but that
it doesn't even support what was standardized in C89—not C17, or C11, or
C99, but C89.

I should point out that the entirety of musl, a competing libc which
does ship with fully functional wide character support, is less than 800
KiB in shared object form, so there's really no defensible reason for
this option to even exist.  Nobody is shipping even embedded Linux
systems with so little storage or memory that this option matters
(because you need a decent amount of space for the kernel as well),
especially considering that flash can be cheaper than CAD 0.06 per GB.

The difference is one set of systems has a minor incompatibility that
requires little work to work around and has few practical effects, and
the other tried to exclude major functionality from a tiny, ancient
standard, the result of which is a wide variety of software that's
broken.  (For example, ncurses normally builds a wide character
version of the shared library in addition to the byte-based version.)

So I agree that we should allow minor variances for nonconformance,
because very few systems practically comply with all of the standards
(Linux, for example, does not).  That's a prudent and sensible thing to
do, and we should definitely continue to do that in the future.  But
given that this is major core functionality in the standard and there's
actually an option to include it which this distributor has just chosen
not to enable, I think it's fine for us to tell the distributor to just
use the appropriate compile-time option for their libc.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Jeff King]

On Fri, Oct 18, 2024 at 08:07:39PM +0000, brian m. carlson wrote:

> The difference is one set of systems has a minor incompatibility that
> requires little work to work around and has few practical effects, and
> the other tried to exclude major functionality from a tiny, ancient
> standard, the result of which is a wide variety of software that's
> broken.  (For example, ncurses normally builds a wide character
> version of the shared library in addition to the byte-based version.)

I think this is the crux of where we see things differently. I don't see
wchar as major functionality, since it's not something that Git has ever
used! It's an include that we pulled in via a dependency to implement a
minor feature that we aren't even making use of. We can just not compile
that otherwise dead code, and everything works as it did in the past. We
do not have to pass judgement on uclibc's feature completeness or make
life any harder for users on such systems.

(This is all assuming that we are dealing with a uclibc with disabled
features in the first place. I don't think we've seen an answer to
Patrick's questions about why the #error is not triggering).

-Peff

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Jeff King]

On Mon, Oct 21, 2024 at 03:19:34PM -0400, Jeff King wrote:

> (This is all assuming that we are dealing with a uclibc with disabled
> features in the first place. I don't think we've seen an answer to
> Patrick's questions about why the #error is not triggering).

Ah, nevermind, I just saw his response from this morning about the stub
header. So I think we do know that this is a tweaked uclibc build.

-Peff

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 3245 bytes --]

On Thu, Oct 17, 2024 at 03:54:28PM +0200, Patrick Steinhardt wrote:
> Okay, uclibc indeed has _optional_ support for `wchar_t`. But what
> really throws me off: "include/wchar.h" from uclibc has the following
> snippet right at the top:
> 
>     #ifndef __UCLIBC_HAS_WCHAR__
>     #error Attempted to include wchar.h when uClibc built without wide char support.
>     #endif
> 
> We unconditionally include <wchar.h>, and your system does not seem to
> have support for it built in. So why doesn't the `#error` trigger? It's
> also not like this is a recent error, it has been added with 581deed72
> (The obligatory forgotten files..., 2002-05-06).
> 
> We can do something like the below patch in clar, but I'd first like to
> understand why your platform seems to be broken in such a way.
> 
> Patrick
> 
> diff --git a/clar.c b/clar.c
> index 64879cf..06fe3d1 100644
> --- a/clar.c
> +++ b/clar.c
> @@ -9,6 +9,11 @@
>  #define _DARWIN_C_SOURCE
>  #define _DEFAULT_SOURCE
>  
> +#if defined(__UCLIBC__) && ! defined(__UCLIBC_HAS_WCHAR__)
> +#else
> +#	define HAVE_WCHAR
> +#endif
> +
>  #include <errno.h>
>  #include <setjmp.h>
>  #include <stdlib.h>
> @@ -16,7 +21,9 @@
>  #include <string.h>
>  #include <math.h>
>  #include <stdarg.h>
> +#ifdef HAVE_WCHAR
>  #include <wchar.h>
> +#endif
>  #include <time.h>
>  #include <inttypes.h>
>  
> @@ -766,6 +773,7 @@ void clar__assert_equal(
>  			}
>  		}
>  	}
> +#ifdef HAVE_WCHAR
>  	else if (!strcmp("%ls", fmt)) {
>  		const wchar_t *wcs1 = va_arg(args, const wchar_t *);
>  		const wchar_t *wcs2 = va_arg(args, const wchar_t *);
> @@ -801,6 +809,7 @@ void clar__assert_equal(
>  			}
>  		}
>  	}
> +#endif // HAVE_WCHAR
>  	else if (!strcmp("%"PRIuMAX, fmt) || !strcmp("%"PRIxMAX, fmt)) {
>  		uintmax_t sz1 = va_arg(args, uintmax_t), sz2 = va_arg(args, uintmax_t);
>  		is_equal = (sz1 == sz2);
> 

Hi,

On Buildroot site, Edgar Bonet (Cc:'ed) suggests to improve your patch by
wrapping strcmps [1]:

---- >8 ----
diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
index cef0f023c2..6de0b415b1 100644
--- a/t/unit-tests/clar/clar.c
+++ b/t/unit-tests/clar/clar.c
@@ -18,6 +18,13 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#if defined(__UCLIBC__) && ! defined(__UCLIBC_HAS_WCHAR__)
+   /* uClibc can be built without wchar support, in which case the
+      installed <wchar.h> is a stub that does not define wchar_t. */
+#else
+#  define HAVE_WCHAR
+#endif
+
 #ifdef _WIN32
 #	define WIN32_LEAN_AND_MEAN
 #	include <windows.h>
@@ -763,6 +770,7 @@ void clar__assert_equal(
 			}
 		}
 	}
+#ifdef HAVE_WCHAR
 	else if (!strcmp("%ls", fmt)) {
 		const wchar_t *wcs1 = va_arg(args, const wchar_t *);
 		const wchar_t *wcs2 = va_arg(args, const wchar_t *);
@@ -798,6 +806,7 @@ void clar__assert_equal(
 			}
 		}
 	}
+#endif // HAVE_WCHAR
 	else if (!strcmp("%"PRIuZ, fmt) || !strcmp("%"PRIxZ, fmt)) {
 		size_t sz1 = va_arg(args, size_t), sz2 = va_arg(args, size_t);
 		is_equal = (sz1 == sz2);

Thanks.

[1]: https://lore.kernel.org/buildroot/f517190c-6fcd-4101-afa6-f6ea521feb9e@grenoble.cnrs.fr/

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: clar unit testing framework FTBFS on uclibc systems (wchar_t unsupported)

[Patrick Steinhardt]

On Fri, Oct 18, 2024 at 08:49:49PM +0700, Bagas Sanjaya wrote:
> On Thu, Oct 17, 2024 at 03:54:28PM +0200, Patrick Steinhardt wrote:
[snip]
> On Buildroot site, Edgar Bonet (Cc:'ed) suggests to improve your patch by
> wrapping strcmps [1]:

Thanks. So with the proposed patch things work?

One thing I still don't understand: why don't you get an error from
includeng <wchar.h>? As shown above, it should contain the following
snippet:

> >     #ifndef __UCLIBC_HAS_WCHAR__
> >     #error Attempted to include wchar.h when uClibc built without wide char support.
> >     #endif

So I'd expect that to trigger and cause the build to abort. Am I missing
anything here? Let me have another look at uclibc...

Oh, yes! There's also a "wchar-stub.h" file that replaces "wchar.h" when
compiled without UCLIBC_HAS_WCHAR=Yes. And that file indeed does not
cause us to error out, but is a stub. It also explains why the logs do
not complain about the missing `wchar_t` type, as that type does get
defined by the stub header. Weird, but so be it.

I've picked this up upstream via [1].

[1]: https://github.com/clar-test/clar/pull/108

Patrick