Multiple-output-file make rules: We're Doing It Wrong - Mailing list pgsql-hackers

In https://postgr.es/m/32497.1519858483@sss.pgh.pa.us
I griped about a weird make failure I was having with VPATH builds.
I did not push the patch I suggested at the time, because I didn't
understand why it seemed to resolve the issue, and because neither
I nor anyone else could reproduce the issue on other machines.

Well, I've had an epiphany after fooling with John Naylor's bootstrap
patch, and I now understand what must have been happening and how
to reproduce it.  Specifically, I can reproduce the issue as stated
if I force sql_help.h to be newer than sql_help.c.  Typically they'd
have the same file mod times, to within the filesystem's resolution;
but create_help.pl does close the .c file first, so with a bit of
bad luck the .h file might be recorded as 1 second newer.  (It's
probably easier to reproduce the problem on newer filesystems with
sub-second timestamp resolution.)

Given that state of affairs, make thinks it needs to rebuild sql_help.c,
which it does by executing the stated rule:

sql_help.c: sql_help.h ;

and of course nothing happens.  So in a plain build, we've just wasted
a few cycles in make.  But in a VPATH build, make believes that the
execution of this rule should have produced a sql_help.c file *in the
build directory*, and then when it goes to compile that file, we get
the sql_help.c-doesn't-exist failure I observed.

My first thought about fixing this was to switch the order of the file
close steps in create_help.pl.  But that's just a hack, which I'd
already considered and rejected for genbki.pl in the case of the
bootstrap data patch.  We cannot hope to control the order in which
bison writes gram.c and gram.h, for instance, so we need some other
solution if we want to prevent the same sort of thing from sometimes
happening in VPATH builds from distribution tarballs.

I propose that the right solution is that these dummy rules should
look like

sql_help.c: sql_help.h
    touch $@

Although the dependent file is actually made by the same process that
made the referenced file, the "touch" makes certain that its file
timestamp is >= the referenced file's timestamp, so that we won't
think we need to re-make it again later, and in particular a VPATH
build will consider that sql_help.c is up to date.

Now, to make this work, we need to make sure that distprep steps
request building of sql_help.c not only sql_help.h, or else the
tarball build run won't execute the touch step and we're back to
a state of uncertainty about the file timestamps.  In the attached
proposed patch, I made sure that any command asking to make the
depended-on file also asked for the dependent file.  This is kind
of annoying, but it avoids assuming anything about which way the
dependency runs, which after all is a basically-arbitrary choice
in the responsible Makefile.

I looked for rules with this bug by looking for comments that
mention parser/Makefile.  There may well be some more, but I have
no good idea how to find them --- any thoughts?  (Note that I
intentionally didn't touch the instance in backend/catalog/Makefile,
as that will be fixed by the bootstrap data patch, and there's no
need to create a merge failure in advance of that.)

Anyway, I think we need to apply and back-patch the attached,
or something much like it.

            regards, tom lane

diff --git a/src/backend/Makefile b/src/backend/Makefile
index 4a28267..d0f99a6 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -131,19 +131,20 @@ postgres.o: $(OBJS)
 # match the dependencies shown in the subdirectory makefiles!

 parser/gram.h: parser/gram.y
-    $(MAKE) -C parser gram.h
+    $(MAKE) -C parser gram.h gram.c

 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 ;
+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
@@ -218,7 +219,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 4b97f83..304f90e 100644
--- a/src/backend/parser/Makefile
+++ b/src/backend/parser/Makefile
@@ -24,11 +24,14 @@ 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 ;
+# make, we must chain the dependencies like this.  Furthermore, the
+# "touch" step is essential, because it ensures that gram.h is seen as
+# newer than (or at least no older than) gram.c.  Without that, make is
+# likely to try to rebuild both files at inopportune times, causing
+# havoc in parallel or VPATH builds.
+
+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 e1b787e..d97f672 100644
--- a/src/backend/storage/lmgr/Makefile
+++ b/src/backend/storage/lmgr/Makefile
@@ -26,7 +26,8 @@ s_lock_test: s_lock.c $(top_builddir)/src/port/libpgport.a
         $(TASPATH) -L $(top_builddir)/src/port -lpgport -o s_lock_test

 # see explanation in ../../parser/Makefile
-lwlocknames.c: lwlocknames.h ;
+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 efb8b53..0809cd8 100644
--- a/src/backend/utils/Makefile
+++ b/src/backend/utils/Makefile
@@ -21,8 +21,11 @@ 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 ;
+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 c8eb2f9..58f7aa8 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -35,7 +35,11 @@ psql: $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils

 help.o: sql_help.h

-sql_help.c: sql_help.h ;
+# sql_help.c is built by create_help.pl, but make sure it's newer than
+# sql_help.h (see comments in src/backend/parser/Makefile)
+sql_help.c: sql_help.h
+    touch $@
+
 sql_help.h: create_help.pl $(wildcard $(REFDOCDIR)/*.sgml)
     $(PERL) $< $(REFDOCDIR) $*

@@ -43,7 +47,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/pl/plpgsql/src/Makefile b/src/pl/plpgsql/src/Makefile
index fc60618..dd17092 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