Refactor installation of extension headers.

Commit be54b37 failed on gmake 3.80 due to a chained conditional,
which on closer examination could be removed entirely with some
refactoring elsewhere for a net simplification and more robustness
against empty expansions. Along the way, add some more comments.

Also make explicit in the documentation and comments that built
headers are not removed by 'make clean', since we don't typically want
that for headers generated by a separate ./configure step, and it's
much easier to add your own 'distclean' rule or use EXTRA_CLEAN than
to try and override a deletion rule in pgxs.mk.

Per buildfarm member prariedog and comments by Michael Paquier, though
all the actual changes are my fault.

Author: RhodiumToad

doc/src/sgml/extend.sgml

@@ -1227,8 +1227,13 @@ include $(PGXS)
       <term><varname>HEADERS_built</varname></term>
       <listitem>
        <para>
-        files to (optionally build and) install under
-        <literal><replaceable>prefix</replaceable>/include/server/$MODULEDIR/$MODULE_big</literal>
+        Files to (optionally build and) install under
+        <literal><replaceable>prefix</replaceable>/include/server/$MODULEDIR/$MODULE_big</literal>.
+       </para>
+       <para>
+        Unlike <literal>DATA_built</literal>, files in <literal>HEADERS_built</literal>
+        are not removed by the <literal>clean</literal> target; if you want them removed,
+        also add them to <literal>EXTRA_CLEAN</literal> or add your own rules to do it.
        </para>
       </listitem>
      </varlistentry>
@@ -1243,6 +1248,11 @@ include $(PGXS)
         where <literal>$MODULE</literal> must be a module name used
         in <literal>MODULES</literal> or <literal>MODULE_big</literal>.
        </para>
+       <para>
+        Unlike <literal>DATA_built</literal>, files in <literal>HEADERS_built_$MODULE</literal>
+        are not removed by the <literal>clean</literal> target; if you want them removed,
+        also add them to <literal>EXTRA_CLEAN</literal> or add your own rules to do it.
+       </para>
        <para>
         It is legal to use both variables for the same module, or any
         combination, unless you have two module names in the

src/makefiles/pgxs.mk

@@ -39,11 +39,11 @@
 #   SCRIPTS_built -- script files (not binaries) to install into $PREFIX/bin,
 #     which need to be built first
 #   HEADERS -- files to install into $(includedir_server)/$MODULEDIR/$MODULE_big
-#   HEADERS_built -- as above but built first
+#   HEADERS_built -- as above but built first (but NOT cleaned)
 #   HEADERS_$(MODULE) -- files to install into
 #     $(includedir_server)/$MODULEDIR/$MODULE; the value of $MODULE must be
 #     listed in MODULES or MODULE_big
-#   HEADERS_built_$(MODULE) -- as above but built first
+#   HEADERS_built_$(MODULE) -- as above but built first (also NOT cleaned)
 #   REGRESS -- list of regression test cases (without suffix)
 #   REGRESS_OPTS -- additional switches to pass to pg_regress
 #   NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if
@@ -117,6 +117,8 @@ ifdef PG_CPPFLAGS
 override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS)
 endif
 
+# logic for HEADERS_* stuff
+
 # get list of all names used with or without built_ prefix
 # note that use of HEADERS_built_foo will get both "foo" and "built_foo",
 # we cope with that later when filtering this list against MODULES.
@@ -128,44 +130,54 @@ HEADER_alldirs := $(patsubst HEADERS_%,%,$(filter HEADERS_%, $(.VARIABLES)))
 HEADER_alldirs += $(patsubst HEADERS_built_%,%,$(filter HEADERS_built_%, $(.VARIABLES)))
 
 # collect all names of built headers to use as a dependency
-HEADER_allbuilt =
+HEADER_allbuilt :=
 
-# HEADERS is an error in the absence of MODULE_big to provide a dir name
 ifdef MODULE_big
-ifdef HEADERS
+
+# we can unconditionally add $(MODULE_big) here, because we will strip it
+# back out below if it turns out not to actually define any headers.
 HEADER_dirs := $(MODULE_big)
 HEADER_unbuilt_$(MODULE_big) = $(HEADERS)
 HEADER_built_$(MODULE_big) = $(HEADERS_built)
 HEADER_allbuilt += $(HEADERS_built)
-else ifdef HEADERS_built
-HEADER_dirs := $(MODULE_big)
-HEADER_built_$(MODULE_big) = $(HEADERS_built)
-HEADER_allbuilt += $(HEADERS_built)
-else
-# file might have used HEADERS_foo or HEADERS_built_foo, so check for those
-HEADER_dirs := $(if $(filter $(MODULE_big) built_$(MODULE_big),$(HEADER_alldirs)),$(MODULE_big))
-HEADER_unbuilt_$(MODULE_big) = $(HEADERS_$(MODULE_big))
-HEADER_built_$(MODULE_big) = $(HEADERS_built_$(MODULE_big))
-HEADER_allbuilt += $(HEADERS_built_$(MODULE_big))
-endif
-else
+# treat "built" as an exclusion below as well as "built_foo"
+HEADER_xdirs := built built_$(MODULE_big)
+
+else # not MODULE_big, so check MODULES
+
+# HEADERS is an error in the absence of MODULE_big to provide a dir name
 ifdef HEADERS
 $(error HEADERS requires MODULE_big to be set)
 endif
 # make list of modules that have either HEADERS_foo or HEADERS_built_foo
 HEADER_dirs := $(foreach m,$(MODULES),$(if $(filter $(m) built_$(m),$(HEADER_alldirs)),$(m)))
+# make list of conflicting names to exclude
+HEADER_xdirs := $(addprefix built_,$(HEADER_dirs))
+
+endif # MODULE_big or MODULES
+
+# HEADERS_foo requires that "foo" is in MODULES as a sanity check
+ifneq (,$(filter-out $(HEADER_dirs) $(HEADER_xdirs),$(HEADER_alldirs)))
+$(error $(patsubst %,HEADERS_%,$(filter-out $(HEADER_dirs) $(HEADER_xdirs),$(HEADER_alldirs))) defined with no module)
+endif
+
 # assign HEADER_unbuilt_foo and HEADER_built_foo, but make sure
 # that "built" takes precedence in the case of conflict, by removing
 # conflicting module names when matching the unbuilt name
-$(foreach m,$(filter-out $(addprefix built_,$(MODULES)),$(MODULES)),$(eval HEADER_unbuilt_$(m) = $$(HEADERS_$(m))))
-$(foreach m,$(MODULES),$(eval HEADER_built_$(m) = $$(HEADERS_built_$(m))))
-$(foreach m,$(MODULES),$(eval HEADER_allbuilt += $$(HEADERS_built_$(m))))
-endif
+$(foreach m,$(filter-out $(HEADER_xdirs),$(HEADER_dirs)),$(eval HEADER_unbuilt_$(m) += $$(HEADERS_$(m))))
+$(foreach m,$(HEADER_dirs),$(eval HEADER_built_$(m) += $$(HEADERS_built_$(m))))
+$(foreach m,$(HEADER_dirs),$(eval HEADER_allbuilt += $$(HEADERS_built_$(m))))
 
-# HEADERS_foo requires that "foo" is in MODULES as a sanity check
-ifneq ($(filter-out $(HEADER_dirs) $(addprefix built_,$(HEADER_dirs)),$(HEADER_alldirs)),)
-$(error $(patsubst %,HEADERS_%,$(filter-out $(HEADER_dirs) $(addprefix built_,$(HEADER_dirs)),$(HEADER_alldirs))) defined with no module)
-endif
+# expand out the list of headers for each dir, attaching source prefixes
+header_file_list = $(HEADER_built_$(1)) $(addprefix $(srcdir)/,$(HEADER_unbuilt_$(1)))
+$(foreach m,$(HEADER_dirs),$(eval HEADER_files_$(m) := $$(call header_file_list,$$(m))))
+
+# note that the caller's HEADERS* vars have all been expanded now, and
+# later changes will have no effect.
+
+# remove entries in HEADER_dirs that produced an empty list of files,
+# to ensure we don't try and install them
+HEADER_dirs := $(foreach m,$(HEADER_dirs),$(if $(strip $(HEADER_files_$(m))),$(m)))
 
 # Functions for generating install/uninstall commands; the blank lines
 # before the "endef" are required, don't lose them
@@ -181,6 +193,8 @@ rm -f $(addprefix '$(DESTDIR)$(includedir_server)/$(incmoduledir)/$(1)'/, $(notd
 
 endef
 
+# end of HEADERS_* stuff
+
 
 all: $(PROGRAM) $(DATA_built) $(HEADER_allbuilt) $(SCRIPTS_built) $(addsuffix $(DLSUFFIX), $(MODULES)) $(addsuffix .control, $(EXTENSION))
 
@@ -228,9 +242,8 @@ endif # SCRIPTS
 ifdef SCRIPTS_built
 	$(INSTALL_SCRIPT) $(SCRIPTS_built) '$(DESTDIR)$(bindir)/'
 endif # SCRIPTS_built
-ifneq ($(strip $(HEADER_dirs)),)
-	$(foreach dir,$(HEADER_dirs),$(if $(HEADER_unbuilt_$(dir)),$(call install_headers,$(dir),$(addprefix $(srcdir)/, $(HEADER_unbuilt_$(dir))))))
-	$(foreach dir,$(HEADER_dirs),$(if $(HEADER_built_$(dir)),$(call install_headers,$(dir),$(HEADER_built_$(dir)))))
+ifneq (,$(strip $(HEADER_dirs)))
+	$(foreach dir,$(HEADER_dirs),$(call install_headers,$(dir),$(HEADER_files_$(dir))))
 endif # HEADERS
 ifdef MODULE_big
 ifeq ($(with_llvm), yes)
@@ -296,8 +309,8 @@ endif
 ifdef SCRIPTS_built
 	rm -f $(addprefix '$(DESTDIR)$(bindir)'/, $(SCRIPTS_built))
 endif
-ifneq ($(strip $(HEADER_dirs)),)
-	$(foreach dir,$(HEADER_dirs),$(call uninstall_headers,$(dir),$(HEADER_unbuilt_$(dir)) $(HEADER_built_$(dir))))
+ifneq (,$(strip $(HEADER_dirs)))
+	$(foreach dir,$(HEADER_dirs),$(call uninstall_headers,$(dir),$(HEADER_files_$(dir))))
 endif # HEADERS
 
 ifdef MODULE_big