Re: Let's stop with the retail rebuilds of src/port/ files already

Andres Freund <andres@anarazel.de> writes:
> On 2018-09-26 19:10:40 -0400, Tom Lane wrote:
>> I'm getting tired of having to make fixes like ce4887bd0.  I think
>> we should rearrange things so that src/port/ and src/common/ compile
>> all their files a third time using shared-library-friendly switches,
>> put them into new .a files, and have libpq and the ecpg libraries
>> just include those libraries instead of what they're doing now.

> +1

Here's a partial patch for that: it adds the third build variant
to src/port/ and teaches libpq to use it.  We'd want to likewise
modify src/common/ and fix up other callers such as ecpg, but this
seems to be enough to test whether the idea works or not.

I've tried this on Linux, macOS and HPUX and it seems to work in
those cases, but I'm not foolish enough to imagine that that's
exhaustive.

What I think would make sense is to push this and see what the
buildfarm thinks of it.  If there are unfixable problems then
we won't have wasted time fleshing out the concept.  Otherwise,
I'll do the remaining pieces.

            regards, tom lane

diff --git a/src/interfaces/libpq/.gitignore b/src/interfaces/libpq/.gitignore
index ce1576e..8885e91 100644
*** a/src/interfaces/libpq/.gitignore
--- b/src/interfaces/libpq/.gitignore
***************
*** 1,27 ****
  /exports.list
  /libpq.rc
  # .c files that are symlinked in from elsewhere
- /chklocale.c
- /crypt.c
- /erand48.c
- /getaddrinfo.c
- /getpeereid.c
- /inet_aton.c
- /inet_net_ntop.c
- /noblock.c
- /open.c
- /system.c
- /pgsleep.c
- /pg_strong_random.c
- /pgstrcasecmp.c
- /pqsignal.c
- /snprintf.c
- /strerror.c
- /strlcpy.c
- /strnlen.c
- /thread.c
- /win32error.c
- /win32setlocale.c
  /ip.c
  /md5.c
  /base64.c
--- 1,6 ----
diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile
index ef8abaf..769c58b 100644
*** a/src/interfaces/libpq/Makefile
--- b/src/interfaces/libpq/Makefile
*************** ifneq ($(PORTNAME), win32)
*** 24,50 ****
  override CFLAGS += $(PTHREAD_CFLAGS)
  endif

- # Need to recompile any external C files because we need
- # all object files to use the same compile flags as libpq; some
- # platforms require special flags.
- LIBS := $(LIBS:-lpgport=)
-
  # We can't use Makefile variables here because the MSVC build system scrapes
  # OBJS from this file.
  OBJS=    fe-auth.o fe-auth-scram.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \
      fe-protocol2.o fe-protocol3.o pqexpbuffer.o fe-secure.o \
      libpq-events.o
- # libpgport C files we always use
- OBJS += chklocale.o inet_net_ntop.o noblock.o pgstrcasecmp.o pqsignal.o \
-     snprintf.o strerror.o thread.o
- # libpgport C files that are needed if identified by configure
- OBJS += $(filter crypt.o getaddrinfo.o getpeereid.o inet_aton.o open.o system.o strlcpy.o strnlen.o win32error.o
win32setlocale.o,$(LIBOBJS)) 
-
- ifeq ($(enable_strong_random), yes)
- OBJS += pg_strong_random.o
- else
- OBJS += erand48.o
- endif

  # src/backend/utils/mb
  OBJS += encnames.o wchar.o
--- 24,34 ----
*************** override shlib = cyg$(NAME)$(DLSUFFIX)
*** 62,69 ****
  endif

  ifeq ($(PORTNAME), win32)
! # pgsleep.o is from libpgport
! OBJS += pgsleep.o win32.o libpqrc.o

  libpqrc.o: libpq.rc
      $(WINDRES) -i $< -o $@
--- 46,52 ----
  endif

  ifeq ($(PORTNAME), win32)
! OBJS += win32.o libpqrc.o

  libpqrc.o: libpq.rc
      $(WINDRES) -i $< -o $@
*************** endif
*** 76,86 ****

  # Add libraries that libpq depends (or might depend) on into the
  # shared library link.  (The order in which you list them here doesn't
! # matter.)
  ifneq ($(PORTNAME), win32)
! SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket
-lnsl-lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS) 
  else
! SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket -lnsl -lresolv
-lintl-lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE) 
  endif
  ifeq ($(PORTNAME), win32)
  SHLIB_LINK += -lshell32 -lws2_32 -lsecur32 $(filter -leay32 -lssleay32 -lcomerr32 -lkrb5_32, $(LIBS))
--- 59,70 ----

  # Add libraries that libpq depends (or might depend) on into the
  # shared library link.  (The order in which you list them here doesn't
! # matter.)  Note that we filter out -lpgport from LIBS and instead
! # insert -lpgport_shlib, to get port files that are built correctly.
  ifneq ($(PORTNAME), win32)
! SHLIB_LINK += -lpgport_shlib $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi
-lssl-lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS) 
  else
! SHLIB_LINK += -lpgport_shlib $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi32 -lssl -lsocket
-lnsl-lresolv -lintl -lm $(PTHREAD_LIBS), $(LIBS)) $(LDAP_LIBS_FE) 
  endif
  ifeq ($(PORTNAME), win32)
  SHLIB_LINK += -lshell32 -lws2_32 -lsecur32 $(filter -leay32 -lssleay32 -lcomerr32 -lkrb5_32, $(LIBS))
*************** SHLIB_EXPORTS = exports.txt
*** 90,111 ****

  all: all-lib

  # Shared library stuff
  include $(top_srcdir)/src/Makefile.shlib
  backend_src = $(top_srcdir)/src/backend


! # We use several libpgport and backend modules verbatim, but since we need
  # to compile with appropriate options to build a shared lib, we can't
! # necessarily use the same object files built for libpgport and the backend.
  # Instead, symlink the source files in here and build our own object files.
- # For some libpgport modules, this only happens if configure decides
- # the module is needed (see filter hack in OBJS, above).
  # When you add a file here, remember to add it in the "clean" target below.

- chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c system.c
pgsleep.cpg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c win32error.c
win32setlocale.c:% : $(top_srcdir)/src/port/% 
-     rm -f $@ && $(LN_S) $< .
-
  ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c: % :
$(top_srcdir)/src/common/%
      rm -f $@ && $(LN_S) $< .

--- 74,92 ----

  all: all-lib

+ all-lib: | submake-libpgport
+
  # Shared library stuff
  include $(top_srcdir)/src/Makefile.shlib
  backend_src = $(top_srcdir)/src/backend


! # We use a few backend modules verbatim, but since we need
  # to compile with appropriate options to build a shared lib, we can't
! # use the same object files built for the backend.
  # Instead, symlink the source files in here and build our own object files.
  # When you add a file here, remember to add it in the "clean" target below.

  ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c: % :
$(top_srcdir)/src/common/%
      rm -f $@ && $(LN_S) $< .

*************** libpq.rc libpq-dist.rc: libpq.rc.in
*** 123,128 ****
--- 104,110 ----
  # installations and is only updated by distprep.)
  libpq.rc: $(top_builddir)/src/Makefile.global

+ # Make dependencies on pg_config_paths.h visible, too.
  fe-connect.o: fe-connect.c $(top_builddir)/src/port/pg_config_paths.h
  fe-misc.o: fe-misc.c $(top_builddir)/src/port/pg_config_paths.h

*************** clean distclean: clean-lib
*** 154,161 ****
      rm -f $(OBJS) pthread.h libpq.rc
  # Might be left over from a Win32 client-only build
      rm -f pg_config_paths.h
! # Remove files we (may have) symlinked in from src/port and other places
!     rm -f chklocale.c crypt.c erand48.c getaddrinfo.c getpeereid.c inet_aton.c inet_net_ntop.c noblock.c open.c
system.cpgsleep.c pg_strong_random.c pgstrcasecmp.c pqsignal.c snprintf.c strerror.c strlcpy.c strnlen.c thread.c
win32error.cwin32setlocale.c 
      rm -f ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c
      rm -f encnames.c wchar.c

--- 136,142 ----
      rm -f $(OBJS) pthread.h libpq.rc
  # Might be left over from a Win32 client-only build
      rm -f pg_config_paths.h
! # Remove files we (may have) symlinked in from src/common and other places
      rm -f ip.c md5.c base64.c link-canary.c scram-common.c sha2.c sha2_openssl.c saslprep.c unicode_norm.c
      rm -f encnames.c wchar.c

diff --git a/src/port/.gitignore b/src/port/.gitignore
index 53a4032..2037b7d 100644
*** a/src/port/.gitignore
--- b/src/port/.gitignore
***************
*** 1,3 ****
--- 1,4 ----
  /libpgport.a
+ /libpgport_shlib.a
  /libpgport_srv.a
  /pg_config_paths.h
diff --git a/src/port/Makefile b/src/port/Makefile
index a2ee8e2..ec62a31 100644
*** a/src/port/Makefile
--- b/src/port/Makefile
***************
*** 1,18 ****
  #-------------------------------------------------------------------------
  #
  # Makefile
! #    Makefile for the port-specific subsystem of the backend
  #
! # These files are used in other directories for portability on systems
! # with broken/missing library files, and for common code sharing.
  #
! # This makefile generates two outputs:
  #
  #    libpgport.a - contains object files with FRONTEND defined,
! #        for use by client application and libraries
  #
  #    libpgport_srv.a - contains object files without FRONTEND defined,
! #        for use only by the backend binaries
  #
  # LIBOBJS is set by configure (via Makefile.global) to be the list of object
  # files that are conditionally needed as determined by configure's probing.
--- 1,23 ----
  #-------------------------------------------------------------------------
  #
  # Makefile
! #    Makefile for src/port
  #
! # These files are used by the Postgres backend, and also by frontend
! # programs.  Primarily, they are meant to provide portability on systems
! # with broken/missing library files.
  #
! # This makefile generates three outputs:
  #
  #    libpgport.a - contains object files with FRONTEND defined,
! #        for use by client applications
! #
! #    libpgport_shlib.a - contains object files with FRONTEND defined,
! #        built suitably for use in shared libraries; for use
! #        by libpq and other frontend libraries
  #
  #    libpgport_srv.a - contains object files without FRONTEND defined,
! #        for use only by the backend
  #
  # LIBOBJS is set by configure (via Makefile.global) to be the list of object
  # files that are conditionally needed as determined by configure's probing.
*************** ifeq ($(enable_strong_random), yes)
*** 40,51 ****
  OBJS += pg_strong_random.o
  endif

! # foo_srv.o and foo.o are both built from foo.c, but only foo.o has -DFRONTEND
  OBJS_SRV = $(OBJS:%.o=%_srv.o)

! all: libpgport.a libpgport_srv.a

  # libpgport is needed by some contrib
  install: all installdirs
      $(INSTALL_STLIB) libpgport.a '$(DESTDIR)$(libdir)/libpgport.a'

--- 45,59 ----
  OBJS += pg_strong_random.o
  endif

! # libpgport.a, libpgport_shlib.a, and libpgport_srv.a contain the same files
! # foo.o, foo_shlib.o, and foo_srv.o are all built from foo.c
! OBJS_SHLIB = $(OBJS:%.o=%_shlib.o)
  OBJS_SRV = $(OBJS:%.o=%_srv.o)

! all: libpgport.a libpgport_shlib.a libpgport_srv.a

  # libpgport is needed by some contrib
+ # currently we don't install libpgport_shlib.a, maybe we should?
  install: all installdirs
      $(INSTALL_STLIB) libpgport.a '$(DESTDIR)$(libdir)/libpgport.a'

*************** libpgport.a: $(OBJS)
*** 59,76 ****
      rm -f $@
      $(AR) $(AROPT) $@ $^

! # thread.o needs PTHREAD_CFLAGS (but thread_srv.o does not)
  thread.o: CFLAGS+=$(PTHREAD_CFLAGS)

! # pg_crc32c_sse42.o and its _srv.o version need CFLAGS_SSE42
  pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42)
  pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42)

! # pg_crc32c_armv8.o and its _srv.o version need CFLAGS_ARMV8_CRC32C
  pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)
  pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)

  #
  # Server versions of object files
  #

--- 67,104 ----
      rm -f $@
      $(AR) $(AROPT) $@ $^

! # thread.o and thread_shlib.o need PTHREAD_CFLAGS (but thread_srv.o does not)
  thread.o: CFLAGS+=$(PTHREAD_CFLAGS)
+ thread_shlib.o: CFLAGS+=$(PTHREAD_CFLAGS)

! # all versions of pg_crc32c_sse42.o need CFLAGS_SSE42
  pg_crc32c_sse42.o: CFLAGS+=$(CFLAGS_SSE42)
+ pg_crc32c_sse42_shlib.o: CFLAGS+=$(CFLAGS_SSE42)
  pg_crc32c_sse42_srv.o: CFLAGS+=$(CFLAGS_SSE42)

! # all versions of pg_crc32c_armv8.o need CFLAGS_ARMV8_CRC32C
  pg_crc32c_armv8.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)
+ pg_crc32c_armv8_shlib.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)
  pg_crc32c_armv8_srv.o: CFLAGS+=$(CFLAGS_ARMV8_CRC32C)

  #
+ # Shared library versions of object files
+ #
+
+ libpgport_shlib.a: $(OBJS_SHLIB)
+     rm -f $@
+     $(AR) $(AROPT) $@ $^
+
+ # Because this uses its own compilation rule, it doesn't use the
+ # dependency tracking logic from Makefile.global.  To make sure that
+ # dependency tracking works anyway for the *_shlib.o files, depend on
+ # their *.o siblings as well, which do have proper dependencies.  It's
+ # a hack that might fail someday if there is a *_shlib.o without a
+ # corresponding *.o, but there seems little reason for that.
+ %_shlib.o: %.c %.o
+     $(CC) $(CFLAGS) $(CFLAGS_SL) $(CPPFLAGS) -c $< -o $@
+
+ #
  # Server versions of object files
  #

*************** libpgport_srv.a: $(OBJS_SRV)
*** 92,97 ****
--- 120,127 ----

  path.o: path.c pg_config_paths.h

+ path_shlib.o: path.c pg_config_paths.h
+
  path_srv.o: path.c pg_config_paths.h

  # We create a separate file rather than put these in pg_config.h
*************** pg_config_paths.h: $(top_builddir)/src/M
*** 112,115 ****
      echo "#define MANDIR \"$(mandir)\"" >>$@

  clean distclean maintainer-clean:
!     rm -f libpgport.a libpgport_srv.a $(OBJS) $(OBJS_SRV) pg_config_paths.h
--- 142,146 ----
      echo "#define MANDIR \"$(mandir)\"" >>$@

  clean distclean maintainer-clean:
!     rm -f libpgport.a libpgport_shlib.a libpgport_srv.a
!     rm -f $(OBJS) $(OBJS_SHLIB) $(OBJS_SRV) pg_config_paths.h