git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: git@vger.kernel.org
Cc: Tay Ray Chuan <rctay89@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Erik Faye-Lund <kusmabite@googlemail.com>,
	Sverre Rabbelier <srabbelier@gmail.com>,
	Andreas Schwab <schwab@linux-m68k.org>
Subject: [PATCH/RFC 15/12] Makefile: check for unnecessary dependencies with CHECK...
Date: Sun, 8 Aug 2010 16:52:20 -0500	[thread overview]
Message-ID: <20100808215220.GD6962@burratino> (raw)
In-Reply-To: <20100808214859.GC6962@burratino>

When the CHECK_HEADER_DEPENDENCIES facility is turned on, report
files in three categories:

 a. detected dependencies missing from the hard-coded dependencies
 b. hard-coded dependencies that were neither detected nor in
    $(LIB_H)
 c. duplicate hard-coded dependencies

Unfortunately, due to my ignorance of GNU make syntax, a and b
each read the detected dependencies from disk independently,
so this is very slow when using an old laptop drive.

Stale dependency rules can easily accumulate unnoticed, so
despite the slowdown, this should be a useful automated check.
The unnecessary dependencies (b) do not include files from
$(LIB_H) to avoid noise from the $(GIT_OBJS): $(LIB_H) rule,
which avoids patch noise adjusting dependencies as files start to
use different parts of libgit.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> That would not be hard to do, but wouldn’t the $(GIT_OBJS): $(LIB_H)
>> rule create a lot of noise?
>>
>> How about if it checks for duplicate dependencies and unnecessary
>> dependencies that are not in LIB_H?
>
> That would be ideal, I think.

Here is a mockup for that.  I still have to speed this up before I
find it bearable to use.  Currently this forks submakes twice to read
in the same computed list of dependencies, but this should be
avoidable (maybe using $(call ...)?).

 Makefile |   53 ++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/Makefile b/Makefile
index 93e1a92..ba4ea79 100644
--- a/Makefile
+++ b/Makefile
@@ -1718,11 +1718,42 @@ endif
 
 ifdef CHECK_HEADER_DEPENDENCIES
 ifndef PRINT_HEADER_DEPENDENCIES
-missing_deps = $(filter-out $(notdir $^), \
-	$(notdir $(shell $(MAKE) -s $@ \
+dep = $(notdir $^)
+all_dep = $(notdir $+)
+computed_dep = $(notdir $(shell $(MAKE) -s $@ \
 		CHECK_HEADER_DEPENDENCIES=YesPlease \
 		USE_COMPUTED_HEADER_DEPENDENCIES=YesPlease \
-		PRINT_HEADER_DEPENDENCIES=YesPlease)))
+		PRINT_HEADER_DEPENDENCIES=YesPlease))
+
+missing_dep = $(filter-out $(dep), $(computed_dep))
+
+sloppy_dep := $(notdir $(LIB_H)) $(notdir $(dep_files))
+extra_dep = $(filter-out $(sloppy_dep) $(computed_dep), $(dep))
+
+nondup_dep = $(foreach f, $(dep), \
+		$(word $(words $(filter $f,$(all_dep))), $f))
+# If the list of dependencies including duplicates has the same size
+# as the list without, there are no dups.
+dup_dep = $(if $(filter $(words $^),$(words $+)),, \
+		$(filter-out $(nondup_dep), $(dep)))
+
+cmd_check_deps = @set -e; echo CHECK $@; \
+	missing_dep="$(missing_dep)"; \
+	extra_dep="$(extra_dep)"; \
+	dup_dep="$(dup_dep)"; \
+	if test "$$missing_dep"; \
+	then \
+		echo missing dependencies: $$missing_dep; \
+	fi; \
+	if test "$$extra_dep"; \
+	then \
+		echo unnecessary dependencies: $$extra_dep; \
+	fi; \
+	if test "$$dup_dep"; \
+	then \
+		echo duplicate dependencies: $$dup_dep; \
+	fi; \
+	test -z "$$missing_dep$$extra_dep$$dup_dep"
 endif
 endif
 
@@ -1747,21 +1778,9 @@ endif
 ifndef PRINT_HEADER_DEPENDENCIES
 ifdef CHECK_HEADER_DEPENDENCIES
 $(C_OBJ): %.o: %.c $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
+	$(cmd_check_deps)
 $(ASM_OBJ): %.o: %.S $(dep_files) FORCE
-	@set -e; echo CHECK $@; \
-	missing_deps="$(missing_deps)"; \
-	if test "$$missing_deps"; \
-	then \
-		echo missing dependencies: $$missing_deps; \
-		false; \
-	fi
+	$(cmd_check_deps)
 endif
 endif
 
-- 
1.7.0.rc1.457.gef7d.dirty

  reply	other threads:[~2010-08-08 21:53 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-08 21:19 [PATCH 0/2] add missing dependencies on headers Jonathan Nieder
2010-08-08 21:25 ` [PATCH 1/2] Makefile: add missing dependencies on url.h Jonathan Nieder
2010-08-08 23:00   ` Jeff King
2010-08-08 21:48 ` [PATCH 2/2] Makefile: add missing dependency on http.h Jonathan Nieder
2010-08-08 21:52   ` Jonathan Nieder [this message]
2010-08-09  4:01   ` Tay Ray Chuan

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=20100808215220.GD6962@burratino \
    --to=jrnieder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kusmabite@googlemail.com \
    --cc=rctay89@gmail.com \
    --cc=schwab@linux-m68k.org \
    --cc=srabbelier@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).