All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	git@vger.kernel.org,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>
Subject: Re: [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
Date: Thu, 23 Sep 2021 00:40:04 +0200	[thread overview]
Message-ID: <8735pw6w8z.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YUuPYDkxjDIvIfwI@coredump.intra.peff.net>


On Wed, Sep 22 2021, Jeff King wrote:

> It's a shame we can't just try to do the _real_ compiles using the
> auto-dependency stuff, and then fall back if they fail. But I think
> there's a chicken-and-egg problem there with "make" doing real work, and
> figuring out the dependencies to do real work.

Isn't this just a chicken & egg problem because we've made it a chicken
& egg problem? I.e. this WIP hack seems to work for me to avoid it:

===============================================================================
diff --git a/Makefile b/Makefile
index 0fe0a9aaa19..68e1714a100 100644
--- a/Makefile
+++ b/Makefile
@@ -1953,6 +1953,7 @@ endif
 ifneq ($(findstring s,$(MAKEFLAGS)),s)
 ifndef V
 	QUIET_CC       = @echo '   ' CC $@;
+	QUIET_DEP      = @echo '   ' DEP $@;
 	QUIET_AR       = @echo '   ' AR $@;
 	QUIET_LINK     = @echo '   ' LINK $@;
 	QUIET_BUILT_IN = @echo '   ' BUILTIN $@;
@@ -2454,24 +2455,6 @@ endif
 .PHONY: objects
 objects: $(OBJECTS)
 
-dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
-dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
-
-ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-$(dep_dirs):
-	@mkdir -p $@
-
-missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
-dep_file = $(dir $@).depend/$(notdir $@).d
-dep_args = -MF $(dep_file) -MQ $@ -MMD -MP
-endif
-
-ifneq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
-dep_dirs =
-missing_dep_dirs =
-dep_args =
-endif
-
 compdb_dir = compile_commands
 
 ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
@@ -2489,10 +2472,22 @@ endif
 ASM_SRC := $(wildcard $(OBJECTS:o=S))
 ASM_OBJ := $(ASM_SRC:S=o)
 C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS))
+C_DEP_OBJ := $(OBJECTS:o=dep)
 
 .SUFFIXES:
 
-$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
+ifeq ($(COMPUTE_HEADER_DEPENDENCIES),yes)
+$(C_DEP_OBJ): $(GENERATED_H)
+$(C_DEP_OBJ): %.dep : %.c command-list.h
+	$(QUIET_DEP)$(CC) -MF $*.dep -MQ $*.o -MM $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
+
+ALL.dep: $(C_DEP_OBJ)
+	$(QUIET_DEP)cat $(C_DEP_OBJ) >$@
+
+include ALL.dep
+endif
+
+$(C_OBJ): %.o: %.c %.dep GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
 $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir)
 	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
===============================================================================

This is just stealing a pattern we already use for
Documentation/doc.dep.

I.e. here every *.c file depends on a corresponding *.dep file, we
generate the *.dep files and *.c files seperately, instead of making the
*.dep a "while we're at it" as we make the *.o.

It's on obvious WIP, e.g. there's no ASM_DEP_OBJ yet, and e.g. a
"grep.dep" itself should in turn depend on say "strbuf.h", since if we
touch strbuf.h we should re-generate the grep.dep file before we make
the "grep.o" from the "grep.c".

It also means that if you have a clean tree and make "grep.o" we'll
generate all the *.dep files and the ALL.dep first, although skipping
that could be done with some clever use of $(MAKECMDGOALS) if anyone
cared.

It also makes "git clean -dxf; make -j8 all" take ~17s on my box instead
of ~13s.

I wonder if it's worth doing anyway, the cost of doing incremental
compiles isn't much affected.

I think the cases where the current schema bites us are rather obscure
though, but maybe I haven't thought of some obvious ones.

I.e. if we don't have the dep file we'll make it for the first time when
making the grep.o file, then include those generated dependencies.

If one of those dependencies changes we'll update the dependencies by
virtue of re-making the grep.o. I *think* the only edge cases are if
you've created a grep.o with COMPUTE_HEADER_DEPENDENCIES=yes, then
recompiled and it's gained a new dependency while using
COMPUTE_HEADER_DEPENDENCIES=no, and finally you try to recompile with
COMPUTE_HEADER_DEPENDENCIES=yes and it does nothing, because you're
referencing a stale ".depends" directory.

But that could be solved by just making the grep.o depend on its own
dependency file, which we don't do now, but could easily do.

So maybe I've talked myself out of there being an inherent dependency
graph violation with the current schema, i.e. the current one seems like
an easily solved bug.

One good thing the above approach still gives you is that you can use
GCC or Clang to make the dependency graph, but then use another compiler
to compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if
I'm compiling with xlc or suncc I can still use the present GCC just to
create the dependency graph.

  parent reply	other threads:[~2021-09-22 23:12 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22 10:38 [PATCH] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic Ævar Arnfjörð Bjarmason
2021-09-22 10:55 ` Carlo Arenas
2021-09-22 11:04   ` Ævar Arnfjörð Bjarmason
2021-09-22 16:08 ` Junio C Hamano
2021-09-22 17:04   ` Jeff King
2021-09-22 18:28     ` Junio C Hamano
2021-09-22 18:44       ` Carlo Arenas
2021-09-22 20:17       ` Jeff King
2021-09-22 20:36         ` Carlo Arenas
2021-09-22 22:40         ` Ævar Arnfjörð Bjarmason [this message]
2021-09-23 17:32           ` Jeff King
2021-09-23  0:03         ` Junio C Hamano
2021-09-23 16:20           ` Jeff King
2021-09-23 17:41             ` Junio C Hamano
2021-09-22 19:45   ` Ævar Arnfjörð Bjarmason
2021-09-22 22:08 ` [PATCH v2 0/2] Makefile: "pedantic" fallout on .depend and "compdb" Ævar Arnfjörð Bjarmason
2021-09-22 22:08   ` [PATCH v2 1/2] Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic Ævar Arnfjörð Bjarmason
2021-09-22 22:08   ` [PATCH v2 2/2] Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes Ævar Arnfjörð Bjarmason
2021-09-23  0:05     ` Carlo Arenas
2021-09-23 21:33     ` Junio C Hamano
2021-09-23 17:38   ` [PATCH v2 0/2] Makefile: "pedantic" fallout on .depend and "compdb" Jeff King

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=8735pw6w8z.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /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.