From 4f14a8c18034a9304654739343163d2452bedce1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Feb 2019 15:41:22 +0100 Subject: [PATCH 1/6] Makefile: remove an out-of-date comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove a comment referring to a caveat that hasn't been applicable since 18b0fc1ce1 ("Git.pm: Kill Git.xs for now", 2006-09-23). At the time of 8d7f586f13 ("Git.pm: Support for perl/ being built by a different compiler", 2006-06-25) some of the code in perl would be built by a C compiler, but support for that went away a few months later in 18b0fc1ce1 discussed above. Since my 20d2a30f8f ("Makefile: replace perl/Makefile.PL with simple make rules", 2017-12-10) the perl/ directory doesn't even have its own build process. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Makefile | 3 --- 1 file changed, 3 deletions(-) diff --git a/Makefile b/Makefile index c5240942f2..97e922cc41 100644 --- a/Makefile +++ b/Makefile @@ -591,9 +591,6 @@ SPATCH_FLAGS = --all-includes --patch . ### --- END CONFIGURATION SECTION --- -# Those must not be GNU-specific; they are shared with perl/ which may -# be built by a different compiler. (Note that this is an artifact now -# but it still might be nice to keep that distinction.) BASIC_CFLAGS = -I. BASIC_LDFLAGS = From 9559f8ffb57ddf9ab3ec64b52aea1d9a645efba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Feb 2019 15:41:23 +0100 Subject: [PATCH 2/6] Makefile: move "strip" assignment down from flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the assignment of the "STRIP" variable down to where we're setting variables with the names of other programs. For consistency with those use "=" for the assignment instead of "?=". I can't imagine why this would need to be different than the rest, and 4dc00021f7 ("Makefile: add 'strip' target", 2006-01-12) which added it doesn't provide an explanation. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 97e922cc41..c53727e44b 100644 --- a/Makefile +++ b/Makefile @@ -512,7 +512,6 @@ CFLAGS = -g -O2 -Wall LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) -STRIP ?= strip # Create as necessary, replace existing, make ranlib unneeded. ARFLAGS = rcs @@ -576,6 +575,7 @@ CURL_CONFIG = curl-config PTHREAD_LIBS = -lpthread PTHREAD_CFLAGS = GCOV = gcov +STRIP = strip SPATCH = spatch export TCL_PATH TCLTK_PATH From 65260a4f23761eceb37917e8bfc4aca36cc890ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Feb 2019 15:41:24 +0100 Subject: [PATCH 3/6] Makefile: add/remove comments at top and tweak whitespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The top of the Makfile is mostly separated into logical steps like set default configuration, set programs etc., but there's some deviation from that. Let's add mostly comments where they're missing, remove those that don't add anything. The whitespace tweaking makes subsequent patches smaller. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Makefile | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index c53727e44b..0870fd4651 100644 --- a/Makefile +++ b/Makefile @@ -507,15 +507,14 @@ GIT-VERSION-FILE: FORCE -include GIT-VERSION-FILE # CFLAGS and LDFLAGS are for the users to override from the command line. - CFLAGS = -g -O2 -Wall LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) - -# Create as necessary, replace existing, make ranlib unneeded. ARFLAGS = rcs +# Set our default configuration. +# # Among the variables below, these: # gitexecdir # template_dir @@ -560,6 +559,7 @@ perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir)) export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir +# Set our default programs CC = cc AR = ar RM = rm -f @@ -587,10 +587,6 @@ SP_EXTRA_FLAGS = SPATCH_FLAGS = --all-includes --patch . - - -### --- END CONFIGURATION SECTION --- - BASIC_CFLAGS = -I. BASIC_LDFLAGS = From 8fb2a231bfc09a04ca5ecc623a9d95f9d24eb054 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Feb 2019 15:41:25 +0100 Subject: [PATCH 4/6] Makefile: Move *_LIBS assignment into its own section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now the only other non-program assignment in the previous list is PTHREAD_CFLAGS, which'll be moved elsewhere in a follow-up change. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 0870fd4651..59674ce9d7 100644 --- a/Makefile +++ b/Makefile @@ -572,7 +572,6 @@ TCLTK_PATH = wish XGETTEXT = xgettext MSGFMT = msgfmt CURL_CONFIG = curl-config -PTHREAD_LIBS = -lpthread PTHREAD_CFLAGS = GCOV = gcov STRIP = strip @@ -580,6 +579,9 @@ SPATCH = spatch export TCL_PATH TCLTK_PATH +# Set our default LIBS variables +PTHREAD_LIBS = -lpthread + # user customisation variable for 'sparse' target SPARSE_FLAGS ?= # internal/platform customisation variable for 'sparse' From 71a7894ba6b0546f28458b8b962674084a08019d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Feb 2019 15:41:26 +0100 Subject: [PATCH 5/6] Makefile: move the setting of *FLAGS closer to "include" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the setting of variables like CFLAGS down past settings like "prefix" and default programs like "TAR" to just before we do the include from "config.mak.*". There's no functional changes here yet, but move note that "ALL_CFLAGS" and "ALL_LDFLAGS" are moved below the include. A follow-up change will tweak those depending on a variable set in config.mak.dev. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Makefile | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index 59674ce9d7..82cfd6c2e4 100644 --- a/Makefile +++ b/Makefile @@ -506,13 +506,6 @@ GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN -include GIT-VERSION-FILE -# CFLAGS and LDFLAGS are for the users to override from the command line. -CFLAGS = -g -O2 -Wall -LDFLAGS = -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) -ALL_LDFLAGS = $(LDFLAGS) -ARFLAGS = rcs - # Set our default configuration. # # Among the variables below, these: @@ -572,7 +565,6 @@ TCLTK_PATH = wish XGETTEXT = xgettext MSGFMT = msgfmt CURL_CONFIG = curl-config -PTHREAD_CFLAGS = GCOV = gcov STRIP = strip SPATCH = spatch @@ -582,16 +574,6 @@ export TCL_PATH TCLTK_PATH # Set our default LIBS variables PTHREAD_LIBS = -lpthread -# user customisation variable for 'sparse' target -SPARSE_FLAGS ?= -# internal/platform customisation variable for 'sparse' -SP_EXTRA_FLAGS = - -SPATCH_FLAGS = --all-includes --patch . - -BASIC_CFLAGS = -I. -BASIC_LDFLAGS = - # Guard against environment variables BUILTIN_OBJS = BUILT_INS = @@ -1160,6 +1142,25 @@ ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/s DC_SHA1_SUBMODULE = auto endif +# Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be +# tweaked by config.* below as well as the command-line, both of +# which'll override these defaults. +CFLAGS = -g -O2 -Wall +LDFLAGS = +BASIC_CFLAGS = -I. +BASIC_LDFLAGS = + +# library flags +ARFLAGS = rcs +PTHREAD_CFLAGS = + +# For the 'sparse' target +SPARSE_FLAGS ?= +SP_EXTRA_FLAGS = + +# For the 'coccicheck' target +SPATCH_FLAGS = --all-includes --patch . + include config.mak.uname -include config.mak.autogen -include config.mak @@ -1168,6 +1169,9 @@ ifdef DEVELOPER include config.mak.dev endif +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) +ALL_LDFLAGS = $(LDFLAGS) + comma := , empty := space := $(empty) $(empty) From 6d5d4b4e9326021f080508126be38ea1beaba6aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Feb 2019 15:41:27 +0100 Subject: [PATCH 6/6] Makefile: allow for combining DEVELOPER=1 and CFLAGS="..." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ever since the DEVELOPER=1 facility introduced there's been no way to have custom CFLAGS (e.g. CFLAGS="-O0 -g -ggdb3") while still benefiting from the set of warnings and assertions DEVELOPER=1 enables. This is because the semantics of variables in the Makefile are such that the user setting CFLAGS overrides anything we set, including what we're doing in config.mak.dev[1]. So let's introduce a "DEVELOPER_CFLAGS" variable in config.mak.dev and add it to ALL_CFLAGS. Before this the ALL_CFLAGS variable would (basically, there's some nuance we won't go into) be set to: $(CPPFLAGS) [$(CFLAGS) *or* $(CFLAGS) in config.mak.dev] $(BASIC_CFLAGS) $(EXTRA_CPPFLAGS) But will now be: $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(BASIC_CFLAGS) $(EXTRA_CPPFLAGS) The reason for putting DEVELOPER_CFLAGS first is to allow for selectively overriding something DEVELOPER=1 brings in. On both GCC and Clang later settings override earlier ones. E.g. "-Wextra -Wno-extra" will enable no "extra" warnings, but not if those two arguments are reversed. Examples of things that weren't possible before, but are now: # Use -O0 instead of -O2 for less painful debuggng DEVELOPER=1 CFLAGS="-O0 -g" # DEVELOPER=1 plus -Wextra, but disable some of the warnings DEVELOPER=1 DEVOPTS="no-error extra-all" CFLAGS="-O0 -g -Wno-unused-parameter" The reason for the patches leading up to this one re-arranged the various *FLAGS assignments and includes is just for readability. The Makefile supports assignments out of order, e.g.: $ cat Makefile X = $(A) $(B) $(C) A = A B = B include c.mak all: @echo $(X) $ cat c.mak C=C $ make A B C So we could have gotten away with the much smaller change of changing "CFLAGS" in config.mak.dev to "DEVELOPER_CFLAGS" and adding that to ALL_CFLAGS earlier in the Makefile "before" the config.mak.* includes. But I think it's more readable to use variables "after" they're included. 1. https://www.gnu.org/software/make/manual/html_node/Overriding.html Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Makefile | 8 ++++++-- config.mak.dev | 44 ++++++++++++++++++++++---------------------- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/Makefile b/Makefile index 82cfd6c2e4..710a3475a9 100644 --- a/Makefile +++ b/Makefile @@ -479,7 +479,11 @@ all:: # # Define DEVELOPER to enable more compiler warnings. Compiler version # and family are auto detected, but could be overridden by defining -# COMPILER_FEATURES (see config.mak.dev) +# COMPILER_FEATURES (see config.mak.dev). You can still set +# CFLAGS="..." in combination with DEVELOPER enables, whether that's +# for tweaking something unrelated (e.g. optimization level), or for +# selectively overriding something DEVELOPER or one of the DEVOPTS +# (see just below) brings in. # # When DEVELOPER is set, DEVOPTS can be used to control compiler # options. This variable contains keywords separated by @@ -1169,7 +1173,7 @@ ifdef DEVELOPER include config.mak.dev endif -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) +ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) comma := , diff --git a/config.mak.dev b/config.mak.dev index 7354fe15b3..bf1f3fcdee 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -1,41 +1,41 @@ ifeq ($(filter no-error,$(DEVOPTS)),) -CFLAGS += -Werror +DEVELOPER_CFLAGS += -Werror endif ifneq ($(filter pedantic,$(DEVOPTS)),) -CFLAGS += -pedantic +DEVELOPER_CFLAGS += -pedantic # don't warn for each N_ use -CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0 -endif -CFLAGS += -Wall -CFLAGS += -Wdeclaration-after-statement -CFLAGS += -Wformat-security -CFLAGS += -Wno-format-zero-length -CFLAGS += -Wold-style-definition -CFLAGS += -Woverflow -CFLAGS += -Wpointer-arith -CFLAGS += -Wstrict-prototypes -CFLAGS += -Wunused -CFLAGS += -Wvla +DEVELOPER_CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0 +endif +DEVELOPER_CFLAGS += -Wall +DEVELOPER_CFLAGS += -Wdeclaration-after-statement +DEVELOPER_CFLAGS += -Wformat-security +DEVELOPER_CFLAGS += -Wno-format-zero-length +DEVELOPER_CFLAGS += -Wold-style-definition +DEVELOPER_CFLAGS += -Woverflow +DEVELOPER_CFLAGS += -Wpointer-arith +DEVELOPER_CFLAGS += -Wstrict-prototypes +DEVELOPER_CFLAGS += -Wunused +DEVELOPER_CFLAGS += -Wvla ifndef COMPILER_FEATURES COMPILER_FEATURES := $(shell ./detect-compiler $(CC)) endif ifneq ($(filter clang4,$(COMPILER_FEATURES)),) -CFLAGS += -Wtautological-constant-out-of-range-compare +DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare endif ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) -CFLAGS += -Wextra +DEVELOPER_CFLAGS += -Wextra # if a function is public, there should be a prototype and the right # header file should be included. If not, it should be static. -CFLAGS += -Wmissing-prototypes +DEVELOPER_CFLAGS += -Wmissing-prototypes ifeq ($(filter extra-all,$(DEVOPTS)),) # These are disabled because we have these all over the place. -CFLAGS += -Wno-empty-body -CFLAGS += -Wno-missing-field-initializers -CFLAGS += -Wno-sign-compare -CFLAGS += -Wno-unused-parameter +DEVELOPER_CFLAGS += -Wno-empty-body +DEVELOPER_CFLAGS += -Wno-missing-field-initializers +DEVELOPER_CFLAGS += -Wno-sign-compare +DEVELOPER_CFLAGS += -Wno-unused-parameter endif endif @@ -43,6 +43,6 @@ endif # not worth fixing since newer compilers correctly stop complaining ifneq ($(filter gcc4,$(COMPILER_FEATURES)),) ifeq ($(filter gcc5,$(COMPILER_FEATURES)),) -CFLAGS += -Wno-uninitialized +DEVELOPER_CFLAGS += -Wno-uninitialized endif endif