Test failures in --disable-multibyte

Test failures in --disable-multibyte

[Daniel Shahaf]

While looking into workers/45164 (${(S)%%*}) I found that igetmatch() had two
implementations, depending on whether multibyte was enabled.  I wanted to test
both codepaths, so I built clean master with --disable-multibyte and ran the
test suite and got several failures (see below).  Some of the failures are
probably bugs in the tests, not in the C core, but even so, given that so many
tests fail — and failed in 5.7.1 too — I wonder whether anyone actually builds
zsh with --disable-multibyte.

The --enable-multibyte flag was added to configure in 2005, and hasn't changed
much since.  The functions that configure checks for when deciding whether to
--enable-multibyte are all in POSIX.1-2001, and all but one of them (wcwidth())
are also in C99.

In the circumstances, I wonder if we should just make those functions a hard
dependency and remove the --disable-multibyte (#ifndef MULTIBYTE_SUPPORT)
codepaths entirely.

Thoughts?

Cheers,

Daniel

[[[
./Test/A03quoting.ztst: starting.
--- /tmp/zsh.ztst.15381/ztst.out	2019-12-31 18:07:13.215429795 +0000
+++ /tmp/zsh.ztst.15381/ztst.tout	2019-12-31 18:07:13.215429795 +0000
@@ -1 +1,3 @@
-$'one\\two\n\'buckle\'\tmy\\shoe\n'
+'one\two
+'\''buckle'\''	my\shoe
+'
Test ./Test/A03quoting.ztst failed: output differs from expected as shown above for:
 foo=$'one\\two\n\'buckle\'\tmy\\shoe\n'
 print -r ${(q+)foo}
Was testing: Extended minimal quoting of quotes and backslashes

./Test/B02typeset.ztst: starting.
--- /tmp/zsh.ztst.22994/ztst.out	2019-12-31 18:07:24.467350785 +0000
+++ /tmp/zsh.ztst.22994/ztst.tout	2019-12-31 18:07:24.467350785 +0000
@@ -1,7 +1,7 @@
 l o c a
 typeset -UT SCALAR array=( l o c a ) ''
 typeset -aT SCALAR array=( l o c a ) ''
-SCALAR=$'l\C-@o\C-@c\C-@a'
+SCALAR=loca
 array=( l o c a )
 local unique tied array SCALAR
 array local tied SCALAR array
Test ./Test/B02typeset.ztst failed: output differs from expected as shown above for:
 typeset -T SCALAR=$'l\000o\000c\000a\000l' array $'\000'
 typeset -U SCALAR
 print $array
 typeset -p SCALAR array
 typeset -m SCALAR array
 typeset +m SCALAR array
 [[ $SCALAR == $'l\000o\000c\000a' ]]
Was testing: Tied parameters and uniquified arrays with NUL-character as separator

./Test/B03print.ztst: starting.
--- /tmp/zsh.ztst.23078/ztst.out	2019-12-31 18:07:25.019346909 +0000
+++ /tmp/zsh.ztst.23078/ztst.tout	2019-12-31 18:07:25.023346880 +0000
@@ -1,2 +1,2 @@
 typeset -g foo='once more'
-typeset -g foo=$'into\C-@the-breach\C-@-'
+typeset -g foo=intothe-breach-
Test ./Test/B03print.ztst failed: output differs from expected as shown above for:
 unset foo
 print -v foo once more
 typeset -p foo
 printf -v foo "%s\0%s-" into the breach
 typeset -p foo
Was testing: print and printf into a variable

./Test/D01prompt.ztst: starting.
--- /tmp/zsh.ztst.24994/ztst.out	2019-12-31 18:07:42.087227059 +0000
+++ /tmp/zsh.ztst.24994/ztst.tout	2019-12-31 18:07:42.091227031 +0000
@@ -1 +1 @@
-
+V
Test ./Test/D01prompt.ztst failed: output differs from expected as shown above for:
  print ${(%U)Y-%(v}
Was testing: Regression test for test on empty psvar

./Test/D04parameter.ztst: starting.
--- /tmp/zsh.ztst.25437/ztst.out        2019-12-31 18:07:43.699215739 +0000                                                                                                                                        
+++ /tmp/zsh.ztst.25437/ztst.tout	2019-12-31 18:07:43.703215711 +0000
@@ -1 +1 @@
-^?^@
+\C-?\C-@
Test ./Test/D04parameter.ztst failed: output differs from expected as shown above for:
  foo=$'\x7f\x00'
  print -r -- ${(V)foo}
Was testing: ${(V)...}

./Test/V09datetime.ztst: starting.
--- /tmp/zsh.ztst.27420/ztst.out	2019-12-31 18:07:49.207177063 +0000
+++ /tmp/zsh.ztst.27420/ztst.tout	2019-12-31 18:07:49.207177063 +0000
@@ -1 +1 @@
-1973^@03^@03
+1973\C-@03\C-@03
Test ./Test/V09datetime.ztst failed: output differs from expected as shown above for:
  print -r -- ${(V)"$(strftime $'%Y\0%m\0%d' 100000000)"}
Was testing: Embedded nulls

./Test/V10private.ztst: starting.
Test ./Test/V10private.ztst failed: bad status 1, expected 0 from:
 if (( UID )); then
   ZTST_verbose=0 $ZTST_exe +Z -f $ZTST_srcdir/ztst.zsh private.TMP/B02
 else
   ZTST_skip="cannot re-run typeset tests when tests run as superuser"
 fi
Was testing: typeset still works with zsh/param/private module loaded

**************************************
43 successful test scripts, 7 failures, 3 skipped
**************************************
]]]

Re: Test failures in --disable-multibyte

[Peter Stephenson]

On Tue, 2019-12-31 at 18:39 +0000, Daniel Shahaf wrote:
> In the circumstances, I wonder if we should just make those functions a hard
> dependency and remove the --disable-multibyte (#ifndef MULTIBYTE_SUPPORT)
> codepaths entirely.

It's not generally expected you'll want to disable multibyte explicitly,
but it tests the case you get if you have a system which doesn't have
the required multibyte support.  It's not clear if zsh gets compiled on
such systems now --- they won't be very common but there might be some
deliberately reduced systems for low-level use.  These only need testing
in a similarly limited fashion.

pws

Re: Test failures in --disable-multibyte

[dana]

On 31 Dec 2019, at 12:39, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> given that so many
> tests fail — and failed in 5.7.1 too — I wonder whether anyone actually builds
> zsh with --disable-multibyte.

See workers/44272. Some people apparently do test with it at least.

(I'd intended to improve things from the test-harness side, and i got half-way
there, but it ended up being kind of ugly and weird, and as usual i got
overwhelmed trying to think about how to make it nicer and eventually set it
aside.)

Re: systems without wide-character support, i'm not very familiar, but the zsh
in Optware (package system for embedded devices) seems to use the defaults,
fwiw

dana

Re: Test failures in --disable-multibyte

[Daniel Shahaf]

dana wrote on Tue, Dec 31, 2019 at 14:23:47 -0600:
> On 31 Dec 2019, at 12:39, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > given that so many
> > tests fail — and failed in 5.7.1 too — I wonder whether anyone actually builds
> > zsh with --disable-multibyte.
> 
> See workers/44272. Some people apparently do test with it at least.

Thanks; I'd forgotten about it.

In Gentoo, USE flags can be set globally (like /etc/make.conf in FreeBSD), so
it's not even clear that someone was specifically interested in a Unicode-less
zsh; for all I know, someone was just doing a QA run, building all packages
under various configurations.

> Re: systems without wide-character support, i'm not very familiar, but the zsh
> in Optware (package system for embedded devices) seems to use the defaults,
> fwiw

Thanks, that's a useful data point.

Peter Stephenson wrote on Tue, Dec 31, 2019 at 19:46:23 +0000:
> On Tue, 2019-12-31 at 18:39 +0000, Daniel Shahaf wrote:
> > In the circumstances, I wonder if we should just make those functions a hard
> > dependency and remove the --disable-multibyte (#ifndef MULTIBYTE_SUPPORT)
> > codepaths entirely.
> 
> It's not generally expected you'll want to disable multibyte explicitly,
> but it tests the case you get if you have a system which doesn't have
> the required multibyte support.  It's not clear if zsh gets compiled on
> such systems now --- they won't be very common but there might be some
> deliberately reduced systems for low-level use.  These only need testing
> in a similarly limited fashion.

This state of affairs doesn't sound quite right.  If those codepaths need
testing, the test suite should be made to pass in that configuration, so that
bugfixes would be able to be tested for unintended side effects.  With no
working test suite and no known users, it sounds like --disable-multibyte is
_de facto_ unsupported, but we are not ready to officially pull the plug on it
due to the possibility of it being used on a C99-less embedded system
somewhere.  So, how about having NEWS in 5.8 say that unless someone asks us to
continue supporting --disable-multibyte, it will become unsupported and may be
removed?

(For what it's worth, even on embedded systems it should be trivial to implement
the requisite standard library functions¹ for the ASCII-only subset of inputs
in terms of the C89 standard library, which we already require.)

Cheers,

Daniel

¹ configure.ac requires the following functions to be available:

  iswalnum iswcntrl iswdigit iswgraph iswlower iswprint iswpunct iswspace iswupper iswxdigit
  mbrlen
  mbrtowc wcrtomb
  towupper towlower
  wcschr wcscpy wcslen wcsncmp wcsncpy
  wcwidth
  wmemchr wmemcmp wmemcpy wmemmove wmemset

Re: Test failures in --disable-multibyte

[Peter Stephenson]

On Wed, 2020-01-01 at 13:44 +0000, Daniel Shahaf wrote:
> > It's not generally expected you'll want to disable multibyte explicitly,
> > but it tests the case you get if you have a system which doesn't have
> > the required multibyte support.  It's not clear if zsh gets compiled on
> > such systems now --- they won't be very common but there might be some
> > deliberately reduced systems for low-level use.  These only need testing
> > in a similarly limited fashion.
> 
> This state of affairs doesn't sound quite right.  If those codepaths
> need testing, the test suite should be made to pass in that
> configuration, so that bugfixes would be able to be tested for
> unintended side effects.  With no working test suite and no known
> users, it sounds like --disable-multibyte is _de facto_ unsupported,
> but we are not ready to officially pull the plug on it due to the
> possibility of it being used on a C99-less embedded system somewhere.
> So, how about having NEWS in 5.8 say that unless someone asks us to
> continue supporting --disable-multibyte, it will become unsupported
> and may be removed?

You're correct, it's de facto unsupported.  The difference is largely
whether anyone gets around to fixing it, which is hard to predict --- I
don't think the problems are likely to be hard.  So certainly it would
be useful to get interest.  Alas, experience suggests most people don't
take much notice until the point where it actually stops working for
them.  So as long as we point out there are problems, I think whatever
takes the least number of hostages to fortune is probably the right
thing to say.

With the changes being largely minor and even cosmetic (but they're
certainly not all cosmetic) it probably doesn't make sense to remove
it any time soon.  But it could rot further, so possibly a warning is
sensible.

Of course, the best solution would be someone relying on simple
character handling taking it over themselves.

> (For what it's worth, even on embedded systems it should be trivial to
> implement the requisite standard library functions¹ for the ASCII-only
> subset of inputs in terms of the C89 standard library, which we
> already require.)

This is pure guesswork, so if you have a solid estimate of how much work
that is you're ahead of me, but I'd be surprised if this was easier in
practice than fixing up the simple mode directly, although it is more
future-proof.

pws

Re: Test failures in --disable-multibyte

[Daniel Shahaf]

Peter Stephenson wrote on Wed, 01 Jan 2020 18:01 +00:00:
> On Wed, 2020-01-01 at 13:44 +0000, Daniel Shahaf wrote:
> > > It's not generally expected you'll want to disable multibyte explicitly,
> > > but it tests the case you get if you have a system which doesn't have
> > > the required multibyte support.  It's not clear if zsh gets compiled on
> > > such systems now --- they won't be very common but there might be some
> > > deliberately reduced systems for low-level use.  These only need testing
> > > in a similarly limited fashion.
> > 
> > This state of affairs doesn't sound quite right.  If those codepaths
> > need testing, the test suite should be made to pass in that
> > configuration, so that bugfixes would be able to be tested for
> > unintended side effects.  With no working test suite and no known
> > users, it sounds like --disable-multibyte is _de facto_ unsupported,
> > but we are not ready to officially pull the plug on it due to the
> > possibility of it being used on a C99-less embedded system somewhere.
> > So, how about having NEWS in 5.8 say that unless someone asks us to
> > continue supporting --disable-multibyte, it will become unsupported
> > and may be removed?
> 
> You're correct, it's de facto unsupported.  The difference is largely
> whether anyone gets around to fixing it, which is hard to predict --- I
> don't think the problems are likely to be hard.  So certainly it would
> be useful to get interest.  Alas, experience suggests most people don't
> take much notice until the point where it actually stops working for
> them.  So as long as we point out there are problems, I think whatever
> takes the least number of hostages to fortune is probably the right
> thing to say.
> 
> With the changes being largely minor and even cosmetic (but they're
> certainly not all cosmetic) it probably doesn't make sense to remove
> it any time soon.  But it could rot further, so possibly a warning is
> sensible.

A build-time warning, then?  I.e., have configure warn if --disable-multibyte
is selected (manually or automatically), or perhaps use a #warning at
the C level.  It could say something to the effect of "Compiling without
multibyte support is known not to pass 'make test'.  We recommend to
rebuild with --enable-multibyte.  Alternatively, fix the test suite and
send us the patches".

This will at least make the issue known to anyone who tries to disable
multibyte support for whatever reason.

Re: Test failures in --disable-multibyte

[Peter Stephenson]

On Thu, 2020-01-02 at 11:30 +0000, Daniel Shahaf wrote:
> A build-time warning, then?  I.e., have configure warn if --disable-multibyte
> is selected (manually or automatically), or perhaps use a #warning at
> the C level.  It could say something to the effect of "Compiling without
> multibyte support is known not to pass 'make test'.  We recommend to
> rebuild with --enable-multibyte.  Alternatively, fix the test suite and
> send us the patches".
> 
> This will at least make the issue known to anyone who tries to disable
> multibyte support for whatever reason.

It sounds like a pretty sensible idea to mention this at the end of the
configure output, where there are a couple of other messages for issues
(typically linking but anything configuration related would seem to be
appropriate).

My gut feel would be warnings in C would be more appropriate if you knew
some actual bit of code it was compiling needed fixing, but other people
may have different attitudes to this.

pws

[PATCH] Make --disable-multibyte warn, since the test suite fails in that configuration. (was: Re: Test failures in --disable-multibyte)

[Daniel Shahaf]

---
Peter Stephenson wrote on Fri, Jan 03, 2020 at 10:15:51 +0000:
> On Thu, 2020-01-02 at 11:30 +0000, Daniel Shahaf wrote:
> > A build-time warning, then?  I.e., have configure warn if --disable-multibyte
> > is selected (manually or automatically), or perhaps use a #warning at
> > the C level.  It could say something to the effect of "Compiling without
> > multibyte support is known not to pass 'make test'.  We recommend to
> > rebuild with --enable-multibyte.  Alternatively, fix the test suite and
> > send us the patches".
> > 
> > This will at least make the issue known to anyone who tries to disable
> > multibyte support for whatever reason.
> 
> It sounds like a pretty sensible idea to mention this at the end of the
> configure output, where there are a couple of other messages for issues
> (typically linking but anything configuration related would seem to be
> appropriate).

Here's a draft.  Probably needs wordsmithing.

Cheers,

Daniel
(This patch was written on top of the gdbm patch I just posted, but they
can be applied independently of each other.)


diff --git a/configure.ac b/configure.ac
index 256584538..cd42a789e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2553,6 +2553,7 @@ wmemcpy wmemmove wmemset; do
       AC_MSG_NOTICE([all functions found, multibyte support enabled])
       zsh_cv_c_unicode_support=yes
     else
+      # Warns at the end of configure
       AC_MSG_NOTICE([missing functions, multibyte support disabled])
       zsh_cv_c_unicode_support=no
     fi
@@ -3299,4 +3300,16 @@ fi
 echo "See config.modules for installed modules and functions.
 "
 
+if test x$zsh_cv_c_unicode_support != xyes; then
+  if test "x$zfuncs_absent" = x; then
+    # The user opted out.
+    AC_MSG_WARN([You have chosen to build without multibyte support.])
+    AC_MSG_WARN([This configuration may not be suitable for production use. It is known to cause errors in 'make test'. We strongly recommend to re-run configure with --enable-multibyte.])
+  else
+    # Some requisite functions are missing.
+    AC_MSG_WARN([Multibyte support disabled due to missing functions: $zfuncs_absent])
+    AC_MSG_WARN([Building without multibyte support is strongly discouraged and may cause errors in 'make test'.])
+  fi
+fi
+
 exit 0

Re: [PATCH] Make --disable-multibyte warn, since the test suite fails in that configuration. (was: Re: Test failures in --disable-multibyte)

[Peter Stephenson]

On Fri, 2020-01-03 at 20:11 +0000, Daniel Shahaf wrote:
> Here's a draft.  Probably needs wordsmithing.

Sorry to be slow --- Definitely in the target area; I'm wondering about

> +    # Some requisite functions are missing.
> +    AC_MSG_WARN([Multibyte support disabled due to missing functions: $zfuncs_absent])
> +    AC_MSG_WARN([Building without multibyte support is strongly discouraged and may cause errors in 'make test'.])

where the user doesn't have a lot of choice about what they're building,
so discouraging it is a bit pointless.  I'd be tempted to omit that bit
here.

pws

Re: [PATCH] Make --disable-multibyte warn, since the test suite fails in that configuration. (was: Re: Test failures in --disable-multibyte)

[Daniel Shahaf]

Peter Stephenson wrote on Sun, Jan 05, 2020 at 18:20:30 +0000:
> On Fri, 2020-01-03 at 20:11 +0000, Daniel Shahaf wrote:
> > Here's a draft.  Probably needs wordsmithing.
> 
> Sorry to be slow --- Definitely in the target area; I'm wondering about

No worries; I wasn't going to commit this until after the weekend anyway.

> > +    # Some requisite functions are missing.
> > +    AC_MSG_WARN([Multibyte support disabled due to missing functions: $zfuncs_absent])
> > +    AC_MSG_WARN([Building without multibyte support is strongly discouraged and may cause errors in 'make test'.])
> 
> where the user doesn't have a lot of choice about what they're building,
> so discouraging it is a bit pointless.  I'd be tempted to omit that bit
> here.

Fair point.  How about:

diff --git a/configure.ac b/configure.ac
index 256584538..cd42a789e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2553,6 +2553,7 @@ wmemcpy wmemmove wmemset; do
       AC_MSG_NOTICE([all functions found, multibyte support enabled])
       zsh_cv_c_unicode_support=yes
     else
+      # Warns at the end of configure
       AC_MSG_NOTICE([missing functions, multibyte support disabled])
       zsh_cv_c_unicode_support=no
     fi
@@ -3299,4 +3300,18 @@ fi
 echo "See config.modules for installed modules and functions.
 "
 
+if test x$zsh_cv_c_unicode_support != xyes; then
+  if test "x$zfuncs_absent" = x; then
+    # The user opted out.
+    AC_MSG_WARN([You have chosen to build without multibyte support.])
+    AC_MSG_WARN([This configuration may not be suitable for production use. It is known to cause errors in 'make test'. We strongly recommend to re-run configure with --enable-multibyte.])
+  else
+    # Some requisite functions are missing.
+    AC_MSG_WARN([Multibyte support cannot be enabled: some standard library functions are missing: $zfuncs_absent])
+    AC_MSG_WARN([This configuration may not be suitable for production use. It is known to cause errors in 'make test'. If your system provides those functions, we recommend to re-run configure appropriately.])
+    # If your system doesn't have those functions, consider patching the
+    # test suite and sending the patch to zsh-workers@ for inclusion.
+  fi
+fi
+
 exit 0

(The "if" branch is unchanged.)

Cheers,

Daniel

Re: [PATCH] Make --disable-multibyte warn, since the test suite fails in that configuration. (was: Re: Test failures in --disable-multibyte)

[Peter Stephenson]

On Sun, 2020-01-05 at 19:13 +0000, Daniel Shahaf wrote:
> Peter Stephenson wrote on Sun, Jan 05, 2020 at 18:20:30 +0000:
> Fair point.  How about:

That's probably as good as we need, thanks.

pws