From 6f2fd3011ea2b479c1b3ab98de7cf627e7d7c65c Mon Sep 17 00:00:00 2001
From: Jordan Lewis <jordanthelewis@gmail.com>
Date: Sun, 28 Mar 2021 12:17:31 -0300
Subject: [PATCH] build: fix spurious rebuilds due to geo .dylib

Previously, the .ALWAYS_REBUILD target for the libgeos .dylib caused
some bad behavior. Since the .dylib would be replaced on every
invocation to `make`, and since the .dylib was also included in the list
of C_LIBS which are dependencies to nearly every Makefile binary target,
it meant that the Makefile would cause spurious rebuilds of binary
targets every other build.

This was particularly obvious when running the `make testlogic`
variants, which usually permit editing the testdata text files without
causing a rebuild to bin/logictest. Before this patch, the bin/logictest
binary would be rebuilt every other invocation to `make`, even if no
source files changed.

The fix is to move the .dylib files to their own dependency list, and
mark them as order-only dependencies in the places that need them. This
is okay because, as dynamic libraries, changes to them don't need to
force changes to the binaries that depend on them.

Release note: None
---
 Makefile           | 13 +++++++------
 build/variables.mk |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 574121be0538..7c2e793787af 100644
--- a/Makefile
+++ b/Makefile
@@ -519,10 +519,11 @@ LIBGEOS     := $(DYN_LIB_DIR)/libgeos.$(DYN_EXT)
 C_LIBS_COMMON = \
 	$(if $(use-stdmalloc),,$(LIBJEMALLOC)) \
 	$(if $(target-is-windows),,$(LIBEDIT)) \
-	$(LIBPROJ) $(LIBGEOS) $(LIBROACH)
+	$(LIBPROJ) $(LIBROACH)
 C_LIBS_SHORT = $(C_LIBS_COMMON)
 C_LIBS_OSS = $(C_LIBS_COMMON) $(LIBPROTOBUF)
 C_LIBS_CCL = $(C_LIBS_COMMON) $(LIBPROTOBUF)
+C_LIBS_DYNAMIC = $(LIBGEOS)
 
 # We only include krb5 on linux, non-musl builds.
 ifeq "$(findstring linux-gnu,$(TARGET_TRIPLE))" "linux-gnu"
@@ -942,7 +943,7 @@ go-targets := $(go-targets-ccl) $(COCKROACHOSS) $(COCKROACHSHORT)
 all: build
 
 .PHONY: c-deps
-c-deps: $(C_LIBS_CCL)
+c-deps: $(C_LIBS_CCL) | $(C_LIBS_DYNAMIC)
 
 build-mode = build -o $@
 
@@ -951,16 +952,16 @@ go-install: build-mode = install
 $(COCKROACH) go-install generate: pkg/ui/distccl/bindata.go
 
 $(COCKROACHOSS): BUILDTARGET = ./pkg/cmd/cockroach-oss
-$(COCKROACHOSS): $(C_LIBS_OSS) pkg/ui/distoss/bindata.go
+$(COCKROACHOSS): $(C_LIBS_OSS) pkg/ui/distoss/bindata.go | $(C_LIBS_DYNAMIC)
 
 $(COCKROACHSHORT): BUILDTARGET = ./pkg/cmd/cockroach-short
 $(COCKROACHSHORT): TAGS += short
-$(COCKROACHSHORT): $(C_LIBS_SHORT)
+$(COCKROACHSHORT): $(C_LIBS_SHORT) | $(C_LIBS_DYNAMIC)
 
 # For test targets, add a tag (used to enable extra assertions).
 $(test-targets): TAGS += crdb_test
 
-$(go-targets-ccl): $(C_LIBS_CCL)
+$(go-targets-ccl): $(C_LIBS_CCL) | $(C_LIBS_DYNAMIC)
 
 BUILDINFO = .buildinfo/tag .buildinfo/rev
 BUILD_TAGGED_RELEASE =
@@ -1768,7 +1769,7 @@ logictest-bins := bin/logictest bin/logictestopt bin/logictestccl
 # TODO(benesch): Derive this automatically. This is getting out of hand.
 bin/workload bin/docgen bin/execgen bin/roachtest $(logictest-bins): $(SQLPARSER_TARGETS) $(LOG_TARGETS) $(PROTOBUF_TARGETS)
 bin/workload bin/docgen bin/roachtest $(logictest-bins): $(LIBPROJ) $(CGO_FLAGS_FILES)
-bin/roachtest $(logictest-bins): $(C_LIBS_CCL) $(CGO_FLAGS_FILES) $(OPTGEN_TARGETS)
+bin/roachtest $(logictest-bins): $(C_LIBS_CCL) $(CGO_FLAGS_FILES) $(OPTGEN_TARGETS) | $(C_LIBS_DYNAMIC)
 
 $(bins): bin/%: bin/%.d | bin/prereqs bin/.submodules-initialized
 	@echo go install -v $*
diff --git a/build/variables.mk b/build/variables.mk
index a6bb260cf050..e2c49e5262f2 100644
--- a/build/variables.mk
+++ b/build/variables.mk
@@ -34,6 +34,7 @@ define VALID_VARS
   C_DEPS_DIR
   C_LIBS_CCL
   C_LIBS_COMMON
+  C_LIBS_DYNAMIC
   C_LIBS_OSS
   C_LIBS_SHORT
   DESTDIR