From 4b538727e2a0e5eae228650c1c145c90471aa521 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 23 Mar 2018 13:45:37 -0400 Subject: [PATCH] Fix make rules that generate multiple output files. For years, our makefiles have correctly observed that "there is no correct way to write a rule that generates two files". However, what we did is to provide empty rules that "generate" the secondary output files from the primary one, and that's not right either. Depending on the details of the creating process, the primary file might end up timestamped later than one or more secondary files, causing subsequent make runs to consider the secondary file(s) out of date. That's harmless in a plain build, since make will just re-execute the empty rule and nothing happens. But it's fatal in a VPATH build, since make will expect the secondary file to be rebuilt in the build directory. This would manifest as "file not found" failures during VPATH builds from tarballs, if we were ever unlucky enough to ship a tarball with apparently out-of-date secondary files. (It's not clear whether that has ever actually happened, but it definitely could.) To ensure that secondary output files have timestamps >= their primary's, change our makefile convention to be that we provide a "touch $@" action not an empty rule. Also, make sure that this rule actually gets invoked during a distprep run, else the hazard remains. It's been like this a long time, so back-patch to all supported branches. In HEAD, I skipped the changes in src/backend/catalog/Makefile, because those rules are due to get replaced soon in the bootstrap data format patch, and there seems no need to create a merge issue for that patch. If for some reason we fail to land that patch in v11, we'll need to back-fill the changes in that one makefile from v10. Discussion: https://postgr.es/m/18556.1521668179@sss.pgh.pa.us --- src/Makefile.shlib | 20 +++++------------ src/backend/Makefile | 33 +++++++++++++++++----------- src/backend/parser/Makefile | 17 +++++++++----- src/backend/storage/lmgr/Makefile | 5 +++-- src/backend/utils/Makefile | 9 +++++--- src/bin/psql/Makefile | 7 ++++-- src/interfaces/ecpg/preproc/Makefile | 5 ++++- src/pl/plpgsql/src/Makefile | 4 +++- src/test/isolation/Makefile | 9 -------- 9 files changed, 58 insertions(+), 51 deletions(-) diff --git a/src/Makefile.shlib b/src/Makefile.shlib index 0839f37e0f..74d48c56f2 100644 --- a/src/Makefile.shlib +++ b/src/Makefile.shlib @@ -315,13 +315,9 @@ else # PORTNAME == aix # AIX case -# There is no correct way to write a rule that generates two files. -# Rules with two targets don't have that meaning, they are merely -# shorthand for two otherwise separate rules. To be safe for parallel -# make, we must chain the dependencies like this. The semicolon is -# important, otherwise make will choose some built-in rule. - -$(stlib): $(shlib) ; +# See notes in src/backend/parser/Makefile about the following two rules +$(stlib): $(shlib) + touch $@ $(shlib): $(OBJS) | $(SHLIB_PREREQS) rm -f $(stlib) @@ -352,13 +348,9 @@ else # Win32 case -# There is no correct way to write a rule that generates two files. -# Rules with two targets don't have that meaning, they are merely -# shorthand for two otherwise separate rules. To be safe for parallel -# make, we must chain the dependencies like this. The semicolon is -# important, otherwise make will choose some built-in rule. - -$(stlib): $(shlib) ; +# See notes in src/backend/parser/Makefile about the following two rules +$(stlib): $(shlib) + touch $@ # XXX A backend that loads a module linked with libgcc_s_dw2-1.dll will exit # uncleanly, hence -static-libgcc. (Last verified with MinGW-w64 compilers diff --git a/src/backend/Makefile b/src/backend/Makefile index 6450c5a959..ca230de2f3 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -70,13 +70,10 @@ ifeq ($(PORTNAME), cygwin) postgres: $(OBJS) $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_EX) $(export_dynamic) -Wl,--stack,$(WIN32_STACK_RLIMIT) -Wl,--export-all-symbols -Wl,--out-implib=libpostgres.a $(call expand_subsys,$^) $(LIBS) -o $@ -# There is no correct way to write a rule that generates two files. -# Rules with two targets don't have that meaning, they are merely -# shorthand for two otherwise separate rules. To be safe for parallel -# make, we must chain the dependencies like this. The semicolon is -# important, otherwise make will choose some built-in rule. - -libpostgres.a: postgres ; +# libpostgres.a is actually built in the preceding rule, but we need this to +# ensure it's newer than postgres; see notes in src/backend/parser/Makefile +libpostgres.a: postgres + touch $@ endif # cygwin @@ -86,7 +83,10 @@ LIBS += -lsecur32 postgres: $(OBJS) $(WIN32RES) $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_EX) -Wl,--stack=$(WIN32_STACK_RLIMIT) -Wl,--export-all-symbols -Wl,--out-implib=libpostgres.a $(call expand_subsys,$(OBJS)) $(WIN32RES) $(LIBS) -o $@$(X) -libpostgres.a: postgres ; +# libpostgres.a is actually built in the preceding rule, but we need this to +# ensure it's newer than postgres; see notes in src/backend/parser/Makefile +libpostgres.a: postgres + touch $@ endif # win32 @@ -130,21 +130,28 @@ postgres.o: $(OBJS) # The following targets are specified in make commands that appear in # the make files in our subdirectories. Note that it's important we # match the dependencies shown in the subdirectory makefiles! +# Also, in cases where a subdirectory makefile generates two files in +# what's really one step, such as bison producing both gram.h and gram.c, +# we must request making the one that is shown as the secondary (dependent) +# output, else the timestamp on it might be wrong. By project convention, +# the .h file is the dependent one for bison output, so we need only request +# that; but in other cases, request both for safety. parser/gram.h: parser/gram.y $(MAKE) -C parser gram.h storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lwlocknames.txt - $(MAKE) -C storage/lmgr lwlocknames.h + $(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c utils/errcodes.h: utils/generate-errcodes.pl utils/errcodes.txt $(MAKE) -C utils errcodes.h -# see explanation in parser/Makefile -utils/fmgrprotos.h: utils/fmgroids.h ; +# see notes in src/backend/parser/Makefile +utils/fmgrprotos.h: utils/fmgroids.h + touch $@ utils/fmgroids.h: utils/Gen_fmgrtab.pl catalog/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.h - $(MAKE) -C utils $(notdir $@) + $(MAKE) -C utils fmgroids.h fmgrprotos.h utils/probes.h: utils/probes.d $(MAKE) -C utils probes.h @@ -219,7 +226,7 @@ distprep: $(MAKE) -C bootstrap bootparse.c bootscanner.c $(MAKE) -C catalog schemapg.h postgres.bki postgres.description postgres.shdescription $(MAKE) -C replication repl_gram.c repl_scanner.c syncrep_gram.c syncrep_scanner.c - $(MAKE) -C storage/lmgr lwlocknames.h + $(MAKE) -C storage/lmgr lwlocknames.h lwlocknames.c $(MAKE) -C utils fmgrtab.c fmgroids.h fmgrprotos.h errcodes.h $(MAKE) -C utils/misc guc-file.c $(MAKE) -C utils/sort qsort_tuple.c diff --git a/src/backend/parser/Makefile b/src/backend/parser/Makefile index 4b97f83803..f14febdbda 100644 --- a/src/backend/parser/Makefile +++ b/src/backend/parser/Makefile @@ -23,12 +23,17 @@ include $(top_srcdir)/src/backend/common.mk # There is no correct way to write a rule that generates two files. # Rules with two targets don't have that meaning, they are merely -# shorthand for two otherwise separate rules. To be safe for parallel -# make, we must chain the dependencies like this. The semicolon is -# important, otherwise make will choose the built-in rule for -# gram.y=>gram.c. - -gram.h: gram.c ; +# shorthand for two otherwise separate rules. If we have an action +# that in fact generates two or more files, we must choose one of them +# as primary and show it as the action's output, then make all of the +# other output files dependent on the primary, like this. Furthermore, +# the "touch" action is essential, because it ensures that gram.h is +# marked as newer than (or at least no older than) gram.c. Without that, +# make is likely to try to rebuild gram.h in subsequent runs, which causes +# failures in VPATH builds from tarballs. + +gram.h: gram.c + touch $@ gram.c: BISONFLAGS += -d gram.c: BISON_CHECK_CMD = $(PERL) $(srcdir)/check_keywords.pl $< $(top_srcdir)/src/include/parser/kwlist.h diff --git a/src/backend/storage/lmgr/Makefile b/src/backend/storage/lmgr/Makefile index e1b787e838..8179f6dceb 100644 --- a/src/backend/storage/lmgr/Makefile +++ b/src/backend/storage/lmgr/Makefile @@ -25,8 +25,9 @@ s_lock_test: s_lock.c $(top_builddir)/src/port/libpgport.a $(CC) $(CPPFLAGS) $(CFLAGS) -DS_LOCK_TEST=1 $(srcdir)/s_lock.c \ $(TASPATH) -L $(top_builddir)/src/port -lpgport -o s_lock_test -# see explanation in ../../parser/Makefile -lwlocknames.c: lwlocknames.h ; +# see notes in src/backend/parser/Makefile +lwlocknames.c: lwlocknames.h + touch $@ lwlocknames.h: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl $(PERL) $(srcdir)/generate-lwlocknames.pl $< diff --git a/src/backend/utils/Makefile b/src/backend/utils/Makefile index efb8b53f49..e296e6ce4d 100644 --- a/src/backend/utils/Makefile +++ b/src/backend/utils/Makefile @@ -20,9 +20,12 @@ all: errcodes.h fmgroids.h fmgrprotos.h probes.h $(SUBDIRS:%=%-recursive): fmgroids.h fmgrprotos.h -# see explanation in ../parser/Makefile -fmgrprotos.h: fmgroids.h ; -fmgroids.h: fmgrtab.c ; +# see notes in src/backend/parser/Makefile +fmgrprotos.h: fmgroids.h + touch $@ + +fmgroids.h: fmgrtab.c + touch $@ fmgrtab.c: Gen_fmgrtab.pl $(catalogdir)/Catalog.pm $(top_srcdir)/src/include/catalog/pg_proc.h $(PERL) -I $(catalogdir) $< -I $(top_srcdir)/src/include/ $(top_srcdir)/src/include/catalog/pg_proc.h diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index b3166ecd15..2ad73d03a1 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -35,7 +35,10 @@ psql: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils help.o: sql_help.h -sql_help.c: sql_help.h ; +# See notes in src/backend/parser/Makefile about the following two rules +sql_help.c: sql_help.h + touch $@ + sql_help.h: create_help.pl $(wildcard $(REFDOCDIR)/*.sgml) $(PERL) $< $(REFDOCDIR) $* @@ -43,7 +46,7 @@ psqlscanslash.c: FLEXFLAGS = -Cfe -p -p psqlscanslash.c: FLEX_NO_BACKUP=yes psqlscanslash.c: FLEX_FIX_WARNING=yes -distprep: sql_help.h psqlscanslash.c +distprep: sql_help.h sql_help.c psqlscanslash.c install: all installdirs $(INSTALL_PROGRAM) psql$(X) '$(DESTDIR)$(bindir)/psql$(X)' diff --git a/src/interfaces/ecpg/preproc/Makefile b/src/interfaces/ecpg/preproc/Makefile index 0a00893b43..b0c404e41e 100644 --- a/src/interfaces/ecpg/preproc/Makefile +++ b/src/interfaces/ecpg/preproc/Makefile @@ -39,7 +39,10 @@ ecpg: $(OBJS) | submake-libpgport ../ecpglib/typename.o: ../ecpglib/typename.c $(MAKE) -C $(dir $@) $(notdir $@) -preproc.h: preproc.c ; +# See notes in src/backend/parser/Makefile about the following two rules +preproc.h: preproc.c + touch $@ + preproc.c: BISONFLAGS += -d preproc.y: ../../../backend/parser/gram.y parse.pl ecpg.addons ecpg.header ecpg.tokens ecpg.trailer ecpg.type diff --git a/src/pl/plpgsql/src/Makefile b/src/pl/plpgsql/src/Makefile index fc60618618..dd17092fd5 100644 --- a/src/pl/plpgsql/src/Makefile +++ b/src/pl/plpgsql/src/Makefile @@ -63,7 +63,9 @@ uninstall-headers: pl_gram.o pl_handler.o pl_comp.o pl_exec.o pl_funcs.o pl_scanner.o: plpgsql.h pl_gram.h plerrcodes.h # See notes in src/backend/parser/Makefile about the following two rules -pl_gram.h: pl_gram.c ; +pl_gram.h: pl_gram.c + touch $@ + pl_gram.c: BISONFLAGS += -d # generate plerrcodes.h from src/backend/utils/errcodes.txt diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile index efbdc40e1d..c3c8280ea2 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -36,15 +36,6 @@ isolationtester$(X): $(OBJS) | submake-libpq submake-libpgport distprep: specparse.c specscanner.c -# There is no correct way to write a rule that generates two files. -# Rules with two targets don't have that meaning, they are merely -# shorthand for two otherwise separate rules. To be safe for parallel -# make, we must chain the dependencies like this. The semicolon is -# important, otherwise make will choose the built-in rule for -# gram.y=>gram.c. - -specparse.h: specparse.c ; - # specscanner is compiled as part of specparse specparse.o: specscanner.c -- 2.40.0