This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[COMMITTED PATCH] Fix fallout from Joseph's untested Makeconfig change.


As others have noticed, Joseph's change was not properly tested and it
broke the build.  Reviewers should have caught the issue, but moreover they
should have pushed back on any structural change to the build system
without clear testimony of definitely adequate testing.  It's clear that
Joseph never tested a clean build from scratch.  Frankly, that's the bare
minimum of testing for a change like this.

I was such a reviewer after the fact and I too am guilty of failing to
catch this.  But I think it's important to point out that Joseph committed
this change before I responded, when, in fact, nobody who credibly should
know all the ins and outs of the makefiles had replied.  I think we have a
general problem if the notion of "review by consensus" is being interpreted
to mean that a positive reply from any contributor whatsoever constitutes
adequate review.  Perhaps Ondrej did not mean to assert that his review was
wholly sufficient for this change, but Joseph seems to have taken it that
way.  Ondrej has made many fine contributions, but to me it is quite clear
that he is still not sufficiently conversant in and aligned with all the
principles and details of libc maintenance to qualify as sole reviewer for
nontrivial changes.  If the system is that one person posts a change,
another person or two who isn't really competent to review that particular
change says it looks good, and nobody explicitly objects within a few days,
then it gets committed, we are going to have a lot of bad code go in over
time.  I don't want to be a gate-keeper for lots of little things, and I
don't want to be obliged to respond very quickly to very many things.
But we need more adult supervision than this.

This covers the cases that broke the build and the similar ones that stood
out to me just looking quickly.  Now the onus is on Joseph to do a thorough
audit of the uses of variables in Makeconfig to make sure that all of them
are compatible with the new inclusion order.  If Joseph doesn't testify
that he has done this audit (and submit changes for any more fallout)
within a week, then I'll revert the Makeconfig change.  (Of course, if
anybody else wants to take up the slack for Joseph, that is great.  It's
certainly not the case that Joseph doesn't do enough around here!)


Thanks,
Roland


	* csu/Makefile (generated, before-compile): Use += rather than =.
	* catgets/Makefile (generated, generated-dirs): Likewise.
	* debug/Makefile (generated): Likewise.
	* dlfcn/Makefile (generated): Likewise.
	* elf/Makefile (before-compile, generated, generated-dirs): Likewise.
	* iconvdata/Makefile (before-compile, generated): Likewise.
	* intl/Makefile (before-compile, generated, generated-dirs): Likewise.
	* libio/Makefile (generated): Likewise.
	* malloc/Makefile (generated): Likewise.
	* manual/Makefile (generated, generated-dirs): Likewise.
	* misc/Makefile (generated): Likewise.
	* posix/Makefile (generated): Likewise.
	* resolv/Makefile (generated): Likewise.
	* sunrpc/Makefile (generated, generated-dirs): Likewise.
	* timezone/Makefile (generated, generated-dirs): Likewise.

localedata/
2014-02-28  Roland McGrath  <roland@hack.frob.com>

	* Makefile (generated, generated-dirs): Use += rather than =.

nptl/
2014-02-28  Roland McGrath  <roland@hack.frob.com>

	* Makefile (generated-dirs): Use += rather than =.

--- a/catgets/Makefile
+++ b/catgets/Makefile
@@ -46,9 +46,9 @@ catgets-CPPFLAGS := -DNLSPATH='"$(msgcatdir)/%L/%N:$(msgcatdir)/%L/LC_MESSAGES/%
 
 CPPFLAGS-gencat = -DNOT_IN_libc
 
-generated = de.msg test1.cat test1.h test2.cat test2.h sample.SJIS.cat \
-	    test-gencat.h
-generated-dirs = de
+generated += de.msg test1.cat test1.h test2.cat test2.h sample.SJIS.cat \
+	     test-gencat.h
+generated-dirs += de
 
 tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de
 
--- a/csu/Makefile
+++ b/csu/Makefile
@@ -39,8 +39,8 @@ omit-deps = $(patsubst %.o,%,$(start-installed-name) g$(start-installed-name) \
 			     b$(start-installed-name) $(csu-dummies) \
 			     S$(start-installed-name))
 install-lib = $(start-installed-name) g$(start-installed-name) $(csu-dummies)
-generated = version-info.h
-before-compile = $(objpfx)version-info.h
+generated += version-info.h
+before-compile += $(objpfx)version-info.h
 
 tests := tst-empty tst-atomic tst-atomic-long
 tests-static := tst-empty
--- a/debug/Makefile
+++ b/debug/Makefile
@@ -166,7 +166,7 @@ install-bin-script = xtrace
 ifeq ($(build-shared),yes)
 install-bin-script += catchsegv
 endif
-generated = catchsegv xtrace
+generated += catchsegv xtrace
 
 include ../Rules
 
--- a/dlfcn/Makefile
+++ b/dlfcn/Makefile
@@ -60,7 +60,7 @@ tststatic5-ENV = $(tststatic-ENV)
 endif
 
 extra-test-objs += $(modules-names:=.os)
-generated := $(modules-names:=.so)
+generated += $(modules-names:=.so)
 
 include ../Rules
 
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -55,9 +55,9 @@ routines += unwind-dw2-fde-glibc
 shared-only-routines += unwind-dw2-fde-glibc
 endif
 
-before-compile  = $(objpfx)trusted-dirs.h
-generated	:= trusted-dirs.h trusted-dirs.st for-renamed/renamed.so
-generated-dirs	:= for-renamed
+before-compile  += $(objpfx)trusted-dirs.h
+generated	+= trusted-dirs.h trusted-dirs.st for-renamed/renamed.so
+generated-dirs	+= for-renamed
 
 ifeq ($(build-shared),yes)
 ld-map		= $(common-objpfx)ld.map
--- a/iconvdata/Makefile
+++ b/iconvdata/Makefile
@@ -177,9 +177,9 @@ gen-special-modules := iso8859-7jp
 generated-modules := $(gen-8bit-modules) $(gen-8bit-gap-modules) \
 		     $(gen-special-modules)
 
-generated = $(generated-modules:=.h) $(generated-modules:=.stmp) \
-	    iconv-test.out iconv-rules tst-loading.mtrace	 \
-	    mtrace-tst-loading tst-tables.out iconv-test.xxx
+generated += $(generated-modules:=.h) $(generated-modules:=.stmp) \
+	     iconv-test.out iconv-rules tst-loading.mtrace	 \
+	     mmtrace-tst-loading tst-tables.out iconv-test.xxx
 ifdef objpfx
 generated += gconv-modules
 endif
@@ -202,7 +202,7 @@ touch $@
 endef
 
 # The headers must be generated before the compilation.
-before-compile = $(addprefix $(objpfx),$(generated-modules:=.h))
+before-compile += $(addprefix $(objpfx),$(generated-modules:=.h))
 
 ifndef avoid-generated
 $(objpfx)iconv-rules: Makefile
--- a/intl/Makefile
+++ b/intl/Makefile
@@ -34,12 +34,12 @@ test-srcs += $(multithread-test-srcs)
 endif
 tests = tst-ngettext
 
-before-compile = $(objpfx)msgs.h
+before-compile += $(objpfx)msgs.h
 
 install-others = $(inst_msgcatdir)/locale.alias
 
-generated = msgs.h mtrace-tst-gettext tst-gettext.mtrace
-generated-dirs := domaindir localedir
+generated += msgs.h mtrace-tst-gettext tst-gettext.mtrace
+generated-dirs += domaindir localedir
 
 ifneq (no,$(BISON))
 plural.c: plural.y
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -159,7 +159,7 @@ bug-ftell-ENV = LOCPATH=$(common-objpfx)localedata
 tst-fgetwc-ENV = LOCPATH=$(common-objpfx)localedata
 tst-fseek-ENV = LOCPATH=$(common-objpfx)localedata
 
-generated = tst-fopenloc.mtrace tst-fopenloc.check
+generated += tst-fopenloc.mtrace tst-fopenloc.check
 
 aux	:= fileops genops stdfiles stdio strops
 
--- a/localedata/Makefile
+++ b/localedata/Makefile
@@ -51,9 +51,9 @@ ld-test-srcs := $(addprefix tests/,$(addsuffix .cm,$(ld-test-names)) \
 fmon-tests = n01y12 n02n40 n10y31 n11y41 n12y11 n20n32 n30y20 n41n00 \
 	     y01y10 y02n22 y22n42 y30y21 y32n31 y40y00 y42n21
 
-generated := $(test-input) $(test-output) sort-test.out tst-locale.out \
+generated += $(test-input) $(test-output) sort-test.out tst-locale.out \
 	     tst-mbswcs.out tst-leaks.mtrace mtrace-tst-leaks
-generated-dirs := $(ld-test-names) tt_TT de_DE.437			\
+generated-dirs += $(ld-test-names) tt_TT de_DE.437			\
 		  $(addprefix tstfmon_,$(fmon-tests))			\
 
 ifeq ($(run-built-tests),yes)
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -53,7 +53,7 @@ CPPFLAGS-memusagestat = -DNOT_IN_libc
 # The Perl script to analyze the output of the mtrace functions.
 ifneq ($(PERL),no)
 install-bin-script = mtrace
-generated = mtrace
+generated += mtrace
 
 # The Perl script will print addresses and to do this nicely we must know
 # whether we are on a 32 or 64 bit machine.
--- a/manual/Makefile
+++ b/manual/Makefile
@@ -162,14 +162,14 @@ minimal-dist = summary.awk texis.awk tsort.awk libc-texinfo.sh libc.texinfo \
 	       $(patsubst %.c.texi,examples/%.c, $(examples))
 
 indices = cp fn pg tp vr ky
-generated-dirs := libc
-generated = libc.dvi libc.pdf libc.tmp libc.info*			    \
-	stubs								    \
-	texis summary.texi stamp-summary *.c.texi			    \
-	$(foreach index,$(indices),libc.$(index) libc.$(index)s)	    \
-	libc.log libc.aux libc.toc					    \
-	$(libc-texi-generated)						    \
-	stamp-libm-err stamp-version
+generated-dirs += libc
+generated += libc.dvi libc.pdf libc.tmp libc.info*			      \
+	     stubs							      \
+	     texis summary.texi stamp-summary *.c.texi			      \
+	     $(foreach index,$(indices),libc.$(index) libc.$(index)s)	      \
+	     libc.log libc.aux libc.toc					      \
+	     $(libc-texi-generated)					      \
+	     stamp-libm-err stamp-version
 
 include ../Rules
 
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -69,7 +69,7 @@ routines := brk sbrk sstk ioctl \
 	    listxattr lgetxattr llistxattr lremovexattr lsetxattr \
 	    removexattr setxattr getauxval ifunc-impl-list
 
-generated := tst-error1.mtrace tst-error1-mem
+generated += tst-error1.mtrace tst-error1-mem
 
 aux := init-misc
 install-lib := libg.a
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -338,7 +338,7 @@ $(objpfx)multidir.mk: $(common-objpfx)config.make
 crti-objs := crti.o
 crtn-objs := crtn.o
 ifneq (,$(patsubst .,,$(multidir)))
-generated-dirs := $(firstword $(subst /, , $(multidir)))
+generated-dirs += $(firstword $(subst /, , $(multidir)))
 crti-objs += $(multidir)/crti.o
 crtn-objs += $(multidir)/crtn.o
 $(objpfx)$(multidir):
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -101,7 +101,7 @@ install-others-programs	:= $(inst_libexecdir)/getconf
 before-compile	:= testcases.h ptestcases.h
 
 # So they get cleaned up.
-generated := $(addprefix wordexp-test-result, 1 2 3 4 5 6 7 8 9 10) \
+generated += $(addprefix wordexp-test-result, 1 2 3 4 5 6 7 8 9 10) \
 	     annexc annexc.out wordexp-tst.out bug-regex2-mem \
 	     bug-regex2.mtrace bug-regex14-mem bug-regex14.mtrace \
 	     bug-regex21-mem bug-regex21.mtrace \
--- a/resolv/Makefile
+++ b/resolv/Makefile
@@ -64,7 +64,7 @@ ifeq (yesyes,$(build-shared)$(have-thread-library))
 tests: $(objpfx)ga_test
 endif
 
-generated := mtrace-tst-leaks tst-leaks.mtrace \
+generated += mtrace-tst-leaks tst-leaks.mtrace \
 	     mtrace-tst-leaks2 tst-leaks2.mtrace
 
 include ../Rules
--- a/sunrpc/Makefile
+++ b/sunrpc/Makefile
@@ -56,9 +56,9 @@ headers-not-in-tirpc = $(addprefix rpc/,key_prot.h rpc_des.h) \
 		       $(rpcsvc:%=rpcsvc/%) rpcsvc/bootparam.h
 headers = rpc/netdb.h
 install-others = $(inst_sysconfdir)/rpc
-generated = $(rpcsvc:%.x=rpcsvc/%.h) $(rpcsvc:%.x=x%.c) $(rpcsvc:%.x=x%.stmp) \
-	    $(rpcsvc:%.x=rpcsvc/%.stmp) rpcgen
-generated-dirs := rpcsvc
+generated += $(rpcsvc:%.x=rpcsvc/%.h) $(rpcsvc:%.x=x%.c) $(rpcsvc:%.x=x%.stmp) \
+	     $(rpcsvc:%.x=rpcsvc/%.stmp) rpcgen
+generated-dirs += rpcsvc
 
 ifeq ($(link-obsolete-rpc),yes)
 headers += $(headers-in-tirpc) $(headers-not-in-tirpc)
--- a/timezone/Makefile
+++ b/timezone/Makefile
@@ -35,10 +35,10 @@ tzbases := africa antarctica asia australasia europe northamerica \
 tzlinks := backward systemv
 tzfiles := $(tzbases) $(tzlinks)
 
-generated := $(addprefix z.,$(tzfiles))
+generated += $(addprefix z.,$(tzfiles))
 install-sbin := zic zdump
 
-generated-dirs = testdata
+generated-dirs += testdata
 
 CPPFLAGS-zic = -DNOT_IN_libc
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]