From 6995d8ac174d9d5d9cdb55cdd69f424cb2e5dc7b Mon Sep 17 00:00:00 2001 From: Mincho Paskalev Date: Tue, 13 May 2025 16:56:22 +0300 Subject: [PATCH] Fix build flags for dependencies (#14038) PR https://github.com/redis/redis/pull/13916 introduced a regression - by overriding the `CFLAGS` and `LDFLAGS` variables for all of the dependencies hiredis and fast_float lost some of their compiler/linker flags. This PR makes it so we can pass additional CFLAGS/LDFLAGS to hiredis, without overriding them as it contains a bit more complex Makefile. As for fast_float - passing CFLAGS/LDFLAGS from outside now doesn't break the expected behavior. The build step in the CI was changed so that the MacOS is now build with TLS to catch such errors in the future. --- .github/workflows/ci.yml | 4 +++- deps/Makefile | 4 +++- deps/fast_float/Makefile | 17 ++++++++++------- deps/hiredis/Makefile | 9 +++++++-- 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7605e19ee..b68e69560 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,7 +54,9 @@ jobs: steps: - uses: actions/checkout@v4 - name: make - run: make REDIS_CFLAGS='-Werror' + # Fail build if there are warnings + # build with TLS just for compilation coverage + run: make REDIS_CFLAGS='-Werror' BUILD_TLS=yes build-32bit: runs-on: ubuntu-latest diff --git a/deps/Makefile b/deps/Makefile index 56df41d34..9ce31e73b 100644 --- a/deps/Makefile +++ b/deps/Makefile @@ -68,9 +68,11 @@ ifneq (,$(filter $(BUILD_TLS),yes module)) HIREDIS_MAKE_FLAGS = USE_SSL=1 endif +# Special care is needed to pass additional CFLAGS/LDFLAGS to hiredis as it +# modifies these variables in its Makefile. hiredis: .make-prerequisites @printf '%b %b\n' $(MAKECOLOR)MAKE$(ENDCOLOR) $(BINCOLOR)$@$(ENDCOLOR) - cd hiredis && $(MAKE) static $(HIREDIS_MAKE_FLAGS) CFLAGS="$(DEPS_CFLAGS)" LDFLAGS="$(DEPS_LDFLAGS)" + cd hiredis && $(MAKE) static $(HIREDIS_MAKE_FLAGS) HIREDIS_CFLAGS="$(DEPS_CFLAGS)" HIREDIS_LDFLAGS="$(DEPS_LDFLAGS)" .PHONY: hiredis diff --git a/deps/fast_float/Makefile b/deps/fast_float/Makefile index d3e5d30c3..e3acaa500 100644 --- a/deps/fast_float/Makefile +++ b/deps/fast_float/Makefile @@ -2,20 +2,23 @@ CC ?= gcc CXX ?= g++ -CFLAGS=-Wall -O3 -# This avoids loosing the fastfloat specific compile flags when we override the CFLAGS via the main project -FASTFLOAT_CFLAGS=-std=c++11 -DFASTFLOAT_ALLOWS_LEADING_PLUS -LDFLAGS= +WARN=-Wall +OPT=-O3 +STD=-std=c++11 +DEFS=-DFASTFLOAT_ALLOWS_LEADING_PLUS + +FASTFLOAT_CFLAGS=$(WARN) $(OPT) $(STD) $(DEFS) $(CFLAGS) +FASTFLOAT_LDFLAGS=$(LDFLAGS) libfast_float: fast_float_strtod.o $(AR) -r libfast_float.a fast_float_strtod.o -32bit: CFLAGS += -m32 -32bit: LDFLAGS += -m32 +32bit: FASTFLOAT_CFLAGS += -m32 +32bit: FASTFLOAT_LDFLAGS += -m32 32bit: libfast_float fast_float_strtod.o: fast_float_strtod.cpp - $(CXX) $(CFLAGS) $(FASTFLOAT_CFLAGS) -c fast_float_strtod.cpp $(LDFLAGS) + $(CXX) $(FASTFLOAT_CFLAGS) -c fast_float_strtod.cpp $(FASTFLOAT_LDFLAGS) clean: rm -f *.o diff --git a/deps/hiredis/Makefile b/deps/hiredis/Makefile index bd2106b1d..5082512e5 100644 --- a/deps/hiredis/Makefile +++ b/deps/hiredis/Makefile @@ -35,14 +35,19 @@ define REDIS_TEST_CONFIG endef export REDIS_TEST_CONFIG +# Flags passed from outside so as not to override CFLAGS or LDFLAGS as they may +# be changed inside this Makefile +HIREDIS_CFLAGS= +HIREDIS_LDFLAGS= + # Fallback to gcc when $CC is not in $PATH. CC:=$(shell sh -c 'type $${CC%% *} >/dev/null 2>/dev/null && echo $(CC) || echo gcc') CXX:=$(shell sh -c 'type $${CXX%% *} >/dev/null 2>/dev/null && echo $(CXX) || echo g++') OPTIMIZATION?=-O3 WARNINGS=-Wall -Wextra -Werror -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers DEBUG_FLAGS?= -g -ggdb -REAL_CFLAGS=$(OPTIMIZATION) -fPIC $(CPPFLAGS) $(CFLAGS) $(WARNINGS) $(DEBUG_FLAGS) $(PLATFORM_FLAGS) -REAL_LDFLAGS=$(LDFLAGS) +REAL_CFLAGS=$(OPTIMIZATION) -fPIC $(CPPFLAGS) $(CFLAGS) $(WARNINGS) $(DEBUG_FLAGS) $(PLATFORM_FLAGS) $(HIREDIS_CFLAGS) +REAL_LDFLAGS=$(LDFLAGS) $(HIREDIS_LDFLAGS) DYLIBSUFFIX=so STLIBSUFFIX=a