All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Nanako Shiraishi <nanako3@lavabit.com>,
	Junio C Hamano <gitster@pobox.com>,
	Johannes Sixt <j.sixt@viscovery.net>,
	Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: [PATCH/RFC 5/5] Teach Makefile to check header dependencies
Date: Thu, 7 Jan 2010 01:30:11 -0600	[thread overview]
Message-ID: <20100107073010.GF11777@progeny.tock> (raw)
In-Reply-To: <20100107071305.GA11777@progeny.tock>

Portability means we cannot completely switch over to
automatically generated dependencies on header files, since some
compilers do not support them.  This would seem to lead to a
dangerous situation in which the hand-written dependency rules
are needed for some configurations but poorly maintained because
most configurations do not use them.

Luckily, there is a way out: as part of testing git, ask the
build system to verify that the hand-written dependency rules are
consistent with the automatically generated ones.  This patch is
a start towards that goal.

The actual patch requires a few steps:

 1. Separate out a USE_COMPUTED_HEADER_DEPENDENCIES option to
    tell make to use the makefile snippets stored in deps/*
    without necessarily regenerating them;

 2. Add a PRINT_HEADER_DEPENDENCIES option to turn on
    USE_COMPUTED_HEADER_DEPENDENCIES and make the %.o: %.c rule
    print its prerequisites instead of compiling anything;

 3. Add a CHECK_HEADER_DEPENDENCIES option to turn off
    USE_COMPUTED_HEADER_DEPENDENCIES and make the %.o: %.c rule
    check that its prerequisites includes all files listed by
    'make -s PRINT_HEADER_DEPENDENCIES=YesPlease $@' instead of
    compiling anything.

With this patch applied,

	echo COMPUTE_HEADER_DEPENDENCIES=YesPlease >> config.mak
	make clean
	make
	make CHECK_HEADER_DEPENDENCIES=YesPlease

produces a useful error message:

	CHECK fast-import.o
	missing dependencies: exec_cmd.h
	make: *** [fast-import.o] Error 1

Probably we should check for missing deps/%.o.d files to avoid
false negatives if some are missing.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I look forward to your thoughts.

This is a bit clunky, but it is useful enough to detect a few problems
with the current dependency rules.  Patches should follow soon.

 Makefile |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 578843c..e642a24 100644
--- a/Makefile
+++ b/Makefile
@@ -224,6 +224,8 @@ all::
 #
 # Define COMPUTE_HEADER_DEPENDENCIES if you want to avoid rebuilding objects
 # when an unrelated header file changes and your compiler supports it.
+#
+# Define CHECK_HEADER_DEPENDENCIES after a successful build to find problems.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1064,6 +1066,28 @@ endif
 -include config.mak.autogen
 -include config.mak
 
+ifdef PRINT_HEADER_DEPENDENCIES
+CHECK_HEADER_DEPENDENCIES = YesPlease
+endif
+
+ifdef CHECK_HEADER_DEPENDENCIES
+ifndef COMPUTE_HEADER_DEPENDENCIES
+$(error checking dependencies requires build with COMPUTE_HEADER_DEPENDENCIES)
+endif
+endif
+
+ifdef COMPUTE_HEADER_DEPENDENCIES
+ifdef CHECK_HEADER_DEPENDENCIES
+ifdef PRINT_HEADER_DEPENDENCIES
+USE_COMPUTED_HEADER_DEPENDENCIES = YesPlease
+else
+USE_COMPUTED_HEADER_DEPENDENCIES =
+endif
+else
+USE_COMPUTED_HEADER_DEPENDENCIES = YesPlease
+endif
+endif
+
 ifdef SANE_TOOL_PATH
 SANE_TOOL_PATH_SQ = $(subst ','\'',$(SANE_TOOL_PATH))
 BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(SANE_TOOL_PATH_SQ)|'
@@ -1669,14 +1693,48 @@ endif
 
 .SUFFIXES:
 
+ifdef CHECK_HEADER_DEPENDENCIES
+
+ifdef PRINT_HEADER_DEPENDENCIES
+$(C_OBJ): %.o: %.c FORCE
+	echo $^
+$(ASM_OBJ): %.o: %.S FORCE
+	echo $^
+else
+missing_deps = $(filter-out $^, \
+	$(shell $(MAKE) -s PRINT_HEADER_DEPENDENCIES=1 $@))
+
+$(C_OBJ): %.o: %.c FORCE
+	@set -e; echo CHECK $@; \
+	missing_deps="$(missing_deps)"; \
+	if test "$$missing_deps"; \
+	then \
+		echo missing dependencies: $$missing_deps; \
+		false; \
+	fi
+$(ASM_OBJ): %.o: %.S FORCE
+	@set -e; echo CHECK $@; \
+	missing_deps="$(missing_deps)"; \
+	if test "$$missing_deps"; \
+	then \
+		echo missing dependencies: $$missing_deps; \
+		false; \
+	fi
+endif
+
+else
+
 $(C_OBJ): %.o: %.c GIT-CFLAGS $(dep_dir_dep)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $<
-%.s: %.c GIT-CFLAGS FORCE
-	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(dep_dir_dep)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $<
 
-ifdef COMPUTE_HEADER_DEPENDENCIES
+endif
+
+%.s: %.c GIT-CFLAGS FORCE
+	$(QUIET_CC)$(CC) -S $(ALL_CFLAGS) $<
+
+ifdef USE_COMPUTED_HEADER_DEPENDENCIES
 # Take advantage of gcc's dependency generation
 # See <http://gcc.gnu.org/gcc-3.0/features.html>.
 dep_files := $(wildcard $(foreach f,$(OBJECTS),$(dir f)deps/$(notdir $f).d))
@@ -1684,9 +1742,6 @@ dep_files := $(wildcard $(foreach f,$(OBJECTS),$(dir f)deps/$(notdir $f).d))
 ifneq ($(dep_files),)
 include $(dep_files)
 endif
-
-dep_file = $(dir $@)deps/$(notdir $@).d
-dep_args = -MF $(dep_file) -MMD -MP
 else
 $(GIT_OBJS): $(LIB_H)
 http.o http-walker.o http-push.o: http.h
@@ -1695,7 +1750,12 @@ builtin-revert.o wt-status.o: wt-status.h
 
 $(XDIFF_OBJS): xdiff/xinclude.h xdiff/xmacros.h xdiff/xdiff.h xdiff/xtypes.h \
 	xdiff/xutils.h xdiff/xprepare.h xdiff/xdiffi.h xdiff/xemit.h
+endif
 
+ifdef COMPUTE_HEADER_DEPENDENCIES
+dep_file = $(dir $@)deps/$(notdir $@).d
+dep_args = -MF $(dep_file) -MMD -MP
+else
 dep_args =
 endif
 
@@ -1823,7 +1883,7 @@ test-delta$X: diff-delta.o patch-delta.o
 
 test-parse-options$X: parse-options.o
 
-ifndef COMPUTE_HEADER_DEPENDENCIES
+ifndef USE_COMPUTED_HEADER_DEPENDENCIES
 test-parse-options.o: parse-options.h
 endif
 
-- 
1.6.6.rc2

  parent reply	other threads:[~2010-01-07  7:30 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-27  8:04 [PATCH] Makefile: determine the list of header files using a glob Johannes Sixt
2009-11-27  8:26 ` Mike Hommey
2009-11-27  8:50   ` Johannes Sixt
2009-11-27  8:58     ` Mike Hommey
2009-11-27 18:28       ` Junio C Hamano
2009-12-30  8:00         ` Mike Hommey
2009-12-30  8:45           ` Junio C Hamano
2009-11-27  9:36 ` Johannes Schindelin
2009-11-27 17:45   ` [PATCH/RFC 0/2] Lazily generate header dependencies Jonathan Nieder
2009-11-27 17:49     ` [PATCH/RFC 1/2] Makefile: use target-specific variable to pass flags to cc Jonathan Nieder
2009-11-27 17:50     ` [PATCH/RFC 2/2] Makefile: automatically compute header dependencies Jonathan Nieder
2009-11-27 22:57       ` Sverre Rabbelier
2009-11-28  4:24         ` Jonathan Nieder
2009-11-28  9:29       ` [PATCH/RFC 2/2 v3] Makefile: lazily " Jonathan Nieder
2009-11-28  9:26         ` Andreas Schwab
2009-11-28 11:49           ` Jonathan Nieder
2010-01-01  0:05     ` [PATCH/RFC 0/2] Lazily generate " Nanako Shiraishi
2010-01-06  1:06       ` Junio C Hamano
2010-01-06  9:26         ` Johannes Schindelin
2010-01-06  9:36           ` Jonathan Nieder
2010-01-07  7:13       ` [PATCH v2 0/5] " Jonathan Nieder
2010-01-07  7:16         ` [PATCH 1/5] Makefile: rearrange dependency rules Jonathan Nieder
2010-01-07  7:18         ` [PATCH 2/5] Makefile: clear list of default rules Jonathan Nieder
2010-01-07  7:19         ` [PATCH 3/5] Makefile: add OBJECTS variable listing object files Jonathan Nieder
2010-01-07  7:23         ` [PATCH 4/5] Makefile: lazily compute header dependencies Jonathan Nieder
2010-01-07  7:30         ` Jonathan Nieder [this message]
2010-01-07 13:22         ` [PATCH v2 0/5] Lazily generate " Erik Faye-Lund
2009-11-27 18:28   ` [PATCH] Makefile: determine the list of header files using a glob Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100107073010.GF11777@progeny.tock \
    --to=jrnieder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j.sixt@viscovery.net \
    --cc=nanako3@lavabit.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.