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

> 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:

> 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!

> 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.

> > 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.

> 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).

> > 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

> 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.

> > 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.

> 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.

> > 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.

> 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.

> 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.

> 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.)

> 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);
>