| Message ID | 560471F5.9080800@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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.
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
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
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.
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
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.
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 --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
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.