diff mbox

18969 - multiple string test failures due to missing locale dependencies

Message ID 560471F5.9080800@gmail.com
State New
Headers show

Commit Message

Martin Sebor Sept. 24, 2015, 9:58 p.m. UTC
After glibc has been built but before 'make check' has been invoked,
running 'make check subdirs=string' causes four string tests to fail
that otherwise succeed.  The failures can also be reproduced by
removing the contents of the localedata directory and invoking make
check as shown below.  The cause of the failures is a missing
dependency of the string makefile on the locales used by string tests.

In addition, while three of the four tests produce output that makes
the reason for their failures clear by printing the names of the
locales they couldn't set, string/tst-strxfrm2 prints the less helpful
"setlocale failed."

The patch attached to the bug and copied below fixes both problems
by a) adding the missing dependency on gen-locales.mk to
string/Makefile and setting the LOCALES variable to the names of
the prerequisite locales, and b) printing the name of the failed
locale in tst-strxfrm2.

The patch was tested on powerpc64le.

Martin

$ rm -rf localedata/* string/test-*.out; nice make subdirs=string -k check
...
FAIL: string/test-strcasecmp
FAIL: string/test-strncasecmp
FAIL: string/tst-strxfrm
FAIL: string/tst-strxfrm2
Summary of test results:
       4 FAIL
      55 PASS
make[1]: *** [tests] Error 1
make[1]: Target `check' not remade because of errors.


2015-09-15  Martin Sebor  <msebor@redhat.com>

	* string/Makefile (LOCALES): Define.
	(gen-locales.mk): Include.
	(test-strcasecmp.out, test-strncasecmp.out, tst-strxfrm.out)
	(tst-strxfrm2.out): Add deppendency on $(gen-locales).
	* string/tst-strxfrm2.c (do_test): Print the name of the locale
	on setlocale failure.

Comments

Mike Frysinger Sept. 26, 2015, 6:24 p.m. UTC | #1
On 24 Sep 2015 15:58, Martin Sebor wrote:
> --- a/string/Makefile
> +++ b/string/Makefile
> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
>   $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
>          cmp $^ > $@; \
>          $(evaluate-test)
> +
> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9 
> tr_TR.UTF-8
> +include ../gen-locales.mk

should this be wrapped in ifeq ($(run-built-tests),yes) ?
-mike
Martin Sebor Sept. 26, 2015, 9 p.m. UTC | #2
On 09/26/2015 12:24 PM, Mike Frysinger wrote:
> On 24 Sep 2015 15:58, Martin Sebor wrote:
>> --- a/string/Makefile
>> +++ b/string/Makefile
>> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
>>    $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
>>           cmp $^ > $@; \
>>           $(evaluate-test)
>> +
>> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9
>> tr_TR.UTF-8
>> +include ../gen-locales.mk
>
> should this be wrapped in ifeq ($(run-built-tests),yes) ?
> -mike

Thanks for looking at the patch.

The patch does insert the whole hunk into an existing block that
is guarded by the conditional (see the line that starts with @@),
although what I suspect really needs to be wrapped is the use of
the gen-locales variable defined in gen-locales.mk. IIUC, whether
or not the include is wrapped shouldn't make a practical difference
since when gen-locales isn't used nothing should depend on the
locales.

FWIW, when I wrote the patch I looked for other makefiles that
include gen-locales.mk for examples. I found just two: one that
includes it unconditionally (benchtests/Makefile) and another
that does so only when run-built-tests is defined to yes
(localedata/Makefile). I went with the latter approach on
the assumption that it's more likely to be correct. But I'd
be happy to be corrected.

Martin
Mike Frysinger Sept. 26, 2015, 9:18 p.m. UTC | #3
On 26 Sep 2015 15:00, Martin Sebor wrote:
> On 09/26/2015 12:24 PM, Mike Frysinger wrote:
> > On 24 Sep 2015 15:58, Martin Sebor wrote:
> >> --- a/string/Makefile
> >> +++ b/string/Makefile
> >> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
> >>    $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
> >>           cmp $^ > $@; \
> >>           $(evaluate-test)
> >> +
> >> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9
> >> tr_TR.UTF-8
> >> +include ../gen-locales.mk
> >
> > should this be wrapped in ifeq ($(run-built-tests),yes) ?
> 
> The patch does insert the whole hunk into an existing block that
> is guarded by the conditional (see the line that starts with @@),

yeah, looks like just one more line of context was needed ;)

> although what I suspect really needs to be wrapped is the use of
> the gen-locales variable defined in gen-locales.mk. IIUC, whether
> or not the include is wrapped shouldn't make a practical difference
> since when gen-locales isn't used nothing should depend on the
> locales.

patch as-is looks ok then

> FWIW, when I wrote the patch I looked for other makefiles that
> include gen-locales.mk for examples. I found just two: one that
> includes it unconditionally (benchtests/Makefile) and another
> that does so only when run-built-tests is defined to yes
> (localedata/Makefile). I went with the latter approach on
> the assumption that it's more likely to be correct. But I'd
> be happy to be corrected.

the benchtests is meant to be run more directly.  i wouldn't
really consider it as an example here.
-mike
Carlos O'Donell Sept. 28, 2015, 2:02 p.m. UTC | #4
Martin,

OK to checkin with one nit fixed.

On 09/24/2015 05:58 PM, Martin Sebor wrote:
> 2015-09-15  Martin Sebor  <msebor@redhat.com>
> 
>     * string/Makefile (LOCALES): Define.
>     (gen-locales.mk): Include.
>     (test-strcasecmp.out, test-strncasecmp.out, tst-strxfrm.out)
>     (tst-strxfrm2.out): Add deppendency on $(gen-locales).
>     * string/tst-strxfrm2.c (do_test): Print the name of the locale
>     on setlocale failure.
> 
> diff --git a/string/Makefile b/string/Makefile
> index 8424a61..0d8df0b 100644
> --- a/string/Makefile
> +++ b/string/Makefile
> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
>  $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
>         cmp $^ > $@; \
>         $(evaluate-test)
> +
> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9 tr_TR.UTF-8

Please use ":=" to avoid any possible deferred expansion, which
we presently never want with LOCALES.

> +include ../gen-locales.mk
> +
> +$(objpfx)test-strcasecmp.out: $(gen-locales)
> +$(objpfx)test-strncasecmp.out: $(gen-locales)
> +$(objpfx)tst-strxfrm.out: $(gen-locales)
> +$(objpfx)tst-strxfrm2.out: $(gen-locales)

OK.

> +
>  endif
> diff --git a/string/tst-strxfrm2.c b/string/tst-strxfrm2.c
> index d5a1115..bea5aa2 100644
> --- a/string/tst-strxfrm2.c
> +++ b/string/tst-strxfrm2.c
> @@ -5,6 +5,8 @@
>  static int
>  do_test (void)
>  {
> +  static const char test_locale[] = "de_DE.UTF-8";

OK.

> +
>    int res = 0;
> 
>    char buf[20];
> @@ -38,9 +40,9 @@ do_test (void)
>        res = 1;
>      }
> 
> -  if (setlocale (LC_ALL, "de_DE.UTF-8") == NULL)
> +  if (setlocale (LC_ALL, test_locale) == NULL)
>      {
> -      puts ("setlocale failed");
> +      printf ("cannot set locale \"%s\"\n", test_locale);

OK. Thanks for fixing this.

>        res = 1;
>      }
>    else

OK, with the nit fixed.

Cheers,
Carlos.
Mike Frysinger Sept. 28, 2015, 9:32 p.m. UTC | #5
On 24 Sep 2015 15:58, Martin Sebor wrote:
> After glibc has been built but before 'make check' has been invoked,
> running 'make check subdirs=string' causes four string tests to fail
> that otherwise succeed.  The failures can also be reproduced by
> removing the contents of the localedata directory and invoking make
> check as shown below.  The cause of the failures is a missing
> dependency of the string makefile on the locales used by string tests.
> 
> In addition, while three of the four tests produce output that makes
> the reason for their failures clear by printing the names of the
> locales they couldn't set, string/tst-strxfrm2 prints the less helpful
> "setlocale failed."
> 
> The patch attached to the bug and copied below fixes both problems
> by a) adding the missing dependency on gen-locales.mk to
> string/Makefile and setting the LOCALES variable to the names of
> the prerequisite locales, and b) printing the name of the failed
> locale in tst-strxfrm2.
> 
> The patch was tested on powerpc64le.

hmm, i see this was pushed now, but with this commit message:

commit 60cf80f09d029257caedc0c8abe7e3e09c64e6c7
Author: Martin Sebor <msebor@gcc.gnu.org>
Date:   Mon Sep 28 16:55:57 2015 -0400

    Let 'make check subdirs=string' succeed even when it's invoked
    immediately after glibc has been built and before 'make check'
    (or after 'make clean').

the glibc git repo should generally follow more git-like flows.  so
in this case, the commit message should have been something like:
======
fix handling of locale data with string tests [BZ #18969]

After glibc has been built but before 'make check' has been invoked,
running 'make check subdirs=string' causes four string tests to fail
that otherwise succeed.  The failures can also be reproduced by
removing the contents of the localedata directory and invoking make
...<the rest of the e-mail you sent>...
===
-mike
Andreas Schwab Oct. 12, 2015, 9:35 a.m. UTC | #6
Martin Sebor <msebor@gmail.com> writes:

> diff --git a/string/Makefile b/string/Makefile
> index 8424a61..0d8df0b 100644
> --- a/string/Makefile
> +++ b/string/Makefile
> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
>  $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
>         cmp $^ > $@; \
>         $(evaluate-test)
> +
> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9 
> tr_TR.UTF-8
> +include ../gen-locales.mk
> +
> +$(objpfx)test-strcasecmp.out: $(gen-locales)
> +$(objpfx)test-strncasecmp.out: $(gen-locales)
> +$(objpfx)tst-strxfrm.out: $(gen-locales)
> +$(objpfx)tst-strxfrm2.out: $(gen-locales)

What happens if there are multiple calls to gen-locales.sh in parallel
from different directories?

Andreas.
Florian Weimer Oct. 12, 2015, 11 a.m. UTC | #7
On 10/12/2015 11:35 AM, Andreas Schwab wrote:
> Martin Sebor <msebor@gmail.com> writes:
> 
>> diff --git a/string/Makefile b/string/Makefile
>> index 8424a61..0d8df0b 100644
>> --- a/string/Makefile
>> +++ b/string/Makefile
>> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
>>  $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
>>         cmp $^ > $@; \
>>         $(evaluate-test)
>> +
>> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9 
>> tr_TR.UTF-8
>> +include ../gen-locales.mk
>> +
>> +$(objpfx)test-strcasecmp.out: $(gen-locales)
>> +$(objpfx)test-strncasecmp.out: $(gen-locales)
>> +$(objpfx)tst-strxfrm.out: $(gen-locales)
>> +$(objpfx)tst-strxfrm2.out: $(gen-locales)
> 
> What happens if there are multiple calls to gen-locales.sh in parallel
> from different directories?

Are you concerned that make will not serialize them properly because the
build system relies on recursive make invocations?

This could indeed be a problem.  I'm not sure what's the best way to
tackle that.

Florian
Mike Frysinger Oct. 12, 2015, 4:43 p.m. UTC | #8
On 12 Oct 2015 13:00, Florian Weimer wrote:
> On 10/12/2015 11:35 AM, Andreas Schwab wrote:
> > Martin Sebor <msebor@gmail.com> writes:
> > 
> >> diff --git a/string/Makefile b/string/Makefile
> >> index 8424a61..0d8df0b 100644
> >> --- a/string/Makefile
> >> +++ b/string/Makefile
> >> @@ -75,4 +75,13 @@ ifeq ($(run-built-tests),yes)
> >>  $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
> >>         cmp $^ > $@; \
> >>         $(evaluate-test)
> >> +
> >> +LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9 
> >> tr_TR.UTF-8
> >> +include ../gen-locales.mk
> >> +
> >> +$(objpfx)test-strcasecmp.out: $(gen-locales)
> >> +$(objpfx)test-strncasecmp.out: $(gen-locales)
> >> +$(objpfx)tst-strxfrm.out: $(gen-locales)
> >> +$(objpfx)tst-strxfrm2.out: $(gen-locales)
> > 
> > What happens if there are multiple calls to gen-locales.sh in parallel
> > from different directories?
> 
> Are you concerned that make will not serialize them properly because the
> build system relies on recursive make invocations?
> 
> This could indeed be a problem.  I'm not sure what's the best way to
> tackle that.

pretty sure that's not a problem.  $(gen-locales) expands into:
CTYPE_FILES = $(addsuffix /LC_CTYPE,$(LOCALES))
gen-locales := $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES))

so it creates one dep per locale.  the rule for processing these targets:
$(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES)): %: \
  ../localedata/gen-locale.sh \
  $(common-objpfx)locale/localedef \
  ../localedata/Makefile \
  $(addprefix ../localedata/charmaps/,$(CHARMAPS)) \
  $(addprefix ../localedata/locales/,$(LOCALE_SRCS))
    @$(SHELL) ../localedata/gen-locale.sh $(common-objpfx) \
          '$(built-program-cmd-before-env)' '$(run-program-env)' \
          '$(built-program-cmd-after-env)' $@; \
    $(evaluate-test)

so it generates one locale per invocation.  it looks correct to me.

then again, it's fairly easy to test: just go into one of these dirs,
delete the locale output, and run `make -j`.  see if any of the commands
are repeated.
-mike
Andreas Schwab Oct. 13, 2015, 7:14 a.m. UTC | #9
Mike Frysinger <vapier@gentoo.org> writes:

> pretty sure that's not a problem.  $(gen-locales) expands into:
> CTYPE_FILES = $(addsuffix /LC_CTYPE,$(LOCALES))
> gen-locales := $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES))
>
> so it creates one dep per locale.

Each subdir has its independent set of dependencies.

> $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES)): %: \
>   ../localedata/gen-locale.sh \
>   $(common-objpfx)locale/localedef \
>   ../localedata/Makefile \
>   $(addprefix ../localedata/charmaps/,$(CHARMAPS)) \
>   $(addprefix ../localedata/locales/,$(LOCALE_SRCS))
>     @$(SHELL) ../localedata/gen-locale.sh $(common-objpfx) \
>           '$(built-program-cmd-before-env)' '$(run-program-env)' \
>           '$(built-program-cmd-after-env)' $@; \
>     $(evaluate-test)
>
> so it generates one locale per invocation.  it looks correct to me.

Different subdirs can build the same locale in parallel.

Andreas.
Mike Frysinger Oct. 17, 2015, 3:16 a.m. UTC | #10
On 13 Oct 2015 09:14, Andreas Schwab wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> > pretty sure that's not a problem.  $(gen-locales) expands into:
> > CTYPE_FILES = $(addsuffix /LC_CTYPE,$(LOCALES))
> > gen-locales := $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES))
> >
> > so it creates one dep per locale.
> 
> Each subdir has its independent set of dependencies.
> 
> > $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES)): %: \
> >   ../localedata/gen-locale.sh \
> >   $(common-objpfx)locale/localedef \
> >   ../localedata/Makefile \
> >   $(addprefix ../localedata/charmaps/,$(CHARMAPS)) \
> >   $(addprefix ../localedata/locales/,$(LOCALE_SRCS))
> >     @$(SHELL) ../localedata/gen-locale.sh $(common-objpfx) \
> >           '$(built-program-cmd-before-env)' '$(run-program-env)' \
> >           '$(built-program-cmd-after-env)' $@; \
> >     $(evaluate-test)
> >
> > so it generates one locale per invocation.  it looks correct to me.
> 
> Different subdirs can build the same locale in parallel.

to be clear, i didn't say otherwise, and i'm aware of this behavior.
i was pointing out that each subdir builds each required locale in
parallel and does so with proper targets.
-mike
Andreas Schwab Oct. 17, 2015, 7:41 a.m. UTC | #11
Mike Frysinger <vapier@gentoo.org> writes:

> On 13 Oct 2015 09:14, Andreas Schwab wrote:
>> Mike Frysinger <vapier@gentoo.org> writes:
>> > pretty sure that's not a problem.  $(gen-locales) expands into:
>> > CTYPE_FILES = $(addsuffix /LC_CTYPE,$(LOCALES))
>> > gen-locales := $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES))
>> >
>> > so it creates one dep per locale.
>> 
>> Each subdir has its independent set of dependencies.
>> 
>> > $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES)): %: \
>> >   ../localedata/gen-locale.sh \
>> >   $(common-objpfx)locale/localedef \
>> >   ../localedata/Makefile \
>> >   $(addprefix ../localedata/charmaps/,$(CHARMAPS)) \
>> >   $(addprefix ../localedata/locales/,$(LOCALE_SRCS))
>> >     @$(SHELL) ../localedata/gen-locale.sh $(common-objpfx) \
>> >           '$(built-program-cmd-before-env)' '$(run-program-env)' \
>> >           '$(built-program-cmd-after-env)' $@; \
>> >     $(evaluate-test)
>> >
>> > so it generates one locale per invocation.  it looks correct to me.
>> 
>> Different subdirs can build the same locale in parallel.
>
> to be clear, i didn't say otherwise, and i'm aware of this behavior.
> i was pointing out that each subdir builds each required locale in
> parallel and does so with proper targets.

That has no influence on the global behaviour.

Andreas.
Florian Weimer Oct. 26, 2015, 12:36 p.m. UTC | #12
On 10/12/2015 06:43 PM, Mike Frysinger wrote:

> so it creates one dep per locale.  the rule for processing these targets:
> $(addprefix $(common-objpfx)localedata/,$(CTYPE_FILES)): %: \
>   ../localedata/gen-locale.sh \
>   $(common-objpfx)locale/localedef \
>   ../localedata/Makefile \
>   $(addprefix ../localedata/charmaps/,$(CHARMAPS)) \
>   $(addprefix ../localedata/locales/,$(LOCALE_SRCS))
>     @$(SHELL) ../localedata/gen-locale.sh $(common-objpfx) \
>           '$(built-program-cmd-before-env)' '$(run-program-env)' \
>           '$(built-program-cmd-after-env)' $@; \
>     $(evaluate-test)
> 
> so it generates one locale per invocation.  it looks correct to me.
> 
> then again, it's fairly easy to test: just go into one of these dirs,
> delete the locale output, and run `make -j`.  see if any of the commands
> are repeated.

I still see occasional crashes in the localedata tests.  Your “make -j”
check does not work because the action in the rule above is prefixed
with “@”.  Can we remove that, so that we may end up with more evidence
of unsynchronized, parallel execution?

Florian
diff mbox

Patch

diff --git a/string/Makefile b/string/Makefile
index 8424a61..0d8df0b 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -75,4 +75,13 @@  ifeq ($(run-built-tests),yes)
  $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
         cmp $^ > $@; \
         $(evaluate-test)
+
+LOCALES = de_DE.UTF-8 en_US.ISO-8859-1 en_US.UTF-8 tr_TR.ISO-8859-9 
tr_TR.UTF-8
+include ../gen-locales.mk
+
+$(objpfx)test-strcasecmp.out: $(gen-locales)
+$(objpfx)test-strncasecmp.out: $(gen-locales)
+$(objpfx)tst-strxfrm.out: $(gen-locales)
+$(objpfx)tst-strxfrm2.out: $(gen-locales)
+
  endif
diff --git a/string/tst-strxfrm2.c b/string/tst-strxfrm2.c
index d5a1115..bea5aa2 100644
--- a/string/tst-strxfrm2.c
+++ b/string/tst-strxfrm2.c
@@ -5,6 +5,8 @@ 
  static int
  do_test (void)
  {
+  static const char test_locale[] = "de_DE.UTF-8";
+
    int res = 0;

    char buf[20];
@@ -38,9 +40,9 @@  do_test (void)
        res = 1;
      }

-  if (setlocale (LC_ALL, "de_DE.UTF-8") == NULL)
+  if (setlocale (LC_ALL, test_locale) == NULL)
      {
-      puts ("setlocale failed");
+      printf ("cannot set locale \"%s\"\n", test_locale);
        res = 1;
      }
    else