git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] Makefile: drop GEN_HDRS
@ 2019-12-13 23:25 Junio C Hamano
  2019-12-14  0:38 ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2019-12-13 23:25 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer, Jeff King, Ramsay Jones

When ebb7baf0 ("Makefile: add a hdr-check target", 2018-09-19)
implemented hdr-check target, it wanted to leave some header files
exempt from the stricter check the target implements, and added
GEN_HDRS macro to list them

This however was probably a bad move for two and half reasons:

 - If we value the header cleanliness check, we eventually want to
   teach our header generating scripts to produce clean headers.
   Keeping the blanket "generated headers can be left as dirty as we
   want" exception does not nudge us in the right direction.

 - There is a list of generated header files, GENERATED_H, which is
   used to keep track of dependencies.  Presence of GEN_HDRS that is
   too similarly named would confuse developers who are adding new
   generated header files which list to add theirs.

 - Even though unicode-width.h could be generated using a contrib/
   script, as far as our build infrastructure is concerned, it is a
   source file that is tracked in the source control system.  Its
   presence in GEN_HDRS list is doubly misleading.

Get rid of GEN_HDRS, which is used only once to list the headers we
do not run hdr-check test on, and instead explicitly list that the
ones, either tracked or generated, that we exempt from the test.

This allows GENERATED_H to be the sole "here are build artifact
header files that are expendable" list, so use it in the clean
target to $(RM) them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index b7d7374dac..9a9d35637d 100644
--- a/Makefile
+++ b/Makefile
@@ -2779,8 +2779,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-GEN_HDRS := command-list.h unicode-width.h
-EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%
+EXCEPT_HDRS := command-list.h unicode-width.h compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
@@ -3105,7 +3104,7 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) $(HCC)
 	$(RM) -r bin-wrappers $(dep_dirs)
 	$(RM) -r po/build/
-	$(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags cscope*
+	$(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope*
 	$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
 	$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
 	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
-- 
2.24.1-672-gdc02b1e6c6


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-13 23:25 [PATCH] Makefile: drop GEN_HDRS Junio C Hamano
@ 2019-12-14  0:38 ` Jeff King
  2019-12-14  1:00   ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2019-12-14  0:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Emily Shaffer, Ramsay Jones

On Fri, Dec 13, 2019 at 03:25:41PM -0800, Junio C Hamano wrote:

> Get rid of GEN_HDRS, which is used only once to list the headers we
> do not run hdr-check test on, and instead explicitly list that the
> ones, either tracked or generated, that we exempt from the test.

Yeah, I think this is an improvement by itself.

After reading this, though:

>  - If we value the header cleanliness check, we eventually want to
>    teach our header generating scripts to produce clean headers.
>    Keeping the blanket "generated headers can be left as dirty as we
>    want" exception does not nudge us in the right direction.

I did expect to see the actual hdr-check behavior move towards checking
these generated versions. However, both are kind of interesting.

unicode-width.h isn't a "real" header file; it's meant to be included in
the middle of a function. I think it _could_ be changed to define
"struct interval" itself, and then be a static file-scope variable. But
there's not really a compelling reason to do so.

But "command-list.h" is more of a traditional header file, being
included at the top of help.c. In theory the hdr-check target could add
a dependency on it, and then we could check it along with everything
else. But even without that first step, if I remove it from EXCEPT_HDRS,
nothing happens!

That's because LIB_H is created by running find in the local filesystem.
So until it's generated, we don't realize it's there to check. I kind of
wonder if it should be part of LIB_H. I suspect that on some systems,
we'd fail to notice a rebuild when command-list.txt is updated (but
nobody noticed, because it is only systems that do not have
compiler-supported dependency tracking, and most developers are no
modern platforms that do).

-Peff

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-14  0:38 ` Jeff King
@ 2019-12-14  1:00   ` Jeff King
  2019-12-16 18:55     ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2019-12-14  1:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Emily Shaffer, Ramsay Jones

On Fri, Dec 13, 2019 at 07:38:21PM -0500, Jeff King wrote:

> That's because LIB_H is created by running find in the local filesystem.
> So until it's generated, we don't realize it's there to check. I kind of
> wonder if it should be part of LIB_H. I suspect that on some systems,
> we'd fail to notice a rebuild when command-list.txt is updated (but
> nobody noticed, because it is only systems that do not have
> compiler-supported dependency tracking, and most developers are no
> modern platforms that do).

Actually, this probably isn't true. We have an explicit dependency for
help.o on command-list.h, so it would get built properly then.

Its inclusion in LIB_H is still wonky, though. It sometimes is included
and sometimes not, depending on whether ls-files or find is used.

-Peff

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-14  1:00   ` Jeff King
@ 2019-12-16 18:55     ` Junio C Hamano
  2019-12-16 19:20       ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2019-12-16 18:55 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Emily Shaffer, Ramsay Jones

Jeff King <peff@peff.net> writes:

> On Fri, Dec 13, 2019 at 07:38:21PM -0500, Jeff King wrote:
>
>> That's because LIB_H is created by running find in the local filesystem.
>> So until it's generated, we don't realize it's there to check. I kind of
>> wonder if it should be part of LIB_H. I suspect that on some systems,
>> we'd fail to notice a rebuild when command-list.txt is updated (but
>> nobody noticed, because it is only systems that do not have
>> compiler-supported dependency tracking, and most developers are no
>> modern platforms that do).
>
> Actually, this probably isn't true. We have an explicit dependency for
> help.o on command-list.h, so it would get built properly then.
>
> Its inclusion in LIB_H is still wonky, though. It sometimes is included
> and sometimes not, depending on whether ls-files or find is used.

As long as GENERATED_H is maintained properly to list headers that
are actually used (e.g. if we ever start creating and using a header
only when some Makefile macro tells us to, we make sure to place the
header in GENERATED_H only when we create and use it), I think we
should just add it to LIB_H, regardless of what is tracked.

LIB_H could contain command-list.h (and other GENERATED_H files) if
we did this, but dups in dependency does not hurt in general, and I
did not find anything potentially problematic in the existing use of
$(LIB_H) in our Makefile.

How about doing this as a further clean-up?  I am reasonably sure
the status-quo description is correct, but I find the justification
a bit weak (in other words, I do not have a good answer to "who
cares if those that depend on $(LIB_H) are not rebuilt when
command-list.h gets rebuilt?")

--- >8 ---
Makefile: include GENERATED_H in LIB_H

$(LIB_H), which is meant to be the list of header files that can
affect (hence trigger recompilation) the objects that go in
libgit.a, in a directory extracted from a tarball is computed by
running "find \*.h" but instead computed with "ls-files \*.h" in a
working tree managed by a git repository.  The former can include
generated header files after a build, and omit them in a clean
state.  The latter would not, as generated header files are by
definition not tracked.

Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 9a9d35637d..552c43c3d8 100644
--- a/Makefile
+++ b/Makefile
@@ -822,6 +822,7 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat
 	-name t -prune -o \
 	-name Documentation -prune -o \
 	-name '*.h' -print)))
+LIB_H += $(GENERATED_H)
 
 LIB_OBJS += abspath.o
 LIB_OBJS += add-interactive.o
@@ -2399,7 +2400,7 @@ else
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
-$(OBJECTS): $(LIB_H) $(GENERATED_H)
+$(OBJECTS): $(LIB_H)
 endif
 
 exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX
@@ -2521,7 +2522,7 @@ XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
 	--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
 	--keyword=__ --keyword=N__ --keyword="__n:1,2"
-LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
+LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
 LOCALIZED_SH += git-rebase--preserve-merges.sh



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-16 18:55     ` Junio C Hamano
@ 2019-12-16 19:20       ` Jeff King
  2019-12-17  1:27         ` Emily Shaffer
  2019-12-17  1:43         ` Jonathan Nieder
  0 siblings, 2 replies; 12+ messages in thread
From: Jeff King @ 2019-12-16 19:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Emily Shaffer, Ramsay Jones

On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote:

> LIB_H could contain command-list.h (and other GENERATED_H files) if
> we did this, but dups in dependency does not hurt in general, and I
> did not find anything potentially problematic in the existing use of
> $(LIB_H) in our Makefile.
> 
> How about doing this as a further clean-up?  I am reasonably sure
> the status-quo description is correct, but I find the justification
> a bit weak (in other words, I do not have a good answer to "who
> cares if those that depend on $(LIB_H) are not rebuilt when
> command-list.h gets rebuilt?")

Yeah, I don't think there's any change in behavior here, since with the
exception of hdr-check, every mention of $(LIB_H) also mentioned
$(GENERATED_H). And in the case of hdr-check, we explicitly exclude the
only item found in $(GENERATED_H).

But this would enable us to start checking command-list.h. I'm on the
fence on whether that's useful or not; the patch below makes it pass,
but I'm not sure if that is really turning up any useful problems. I
suppose somebody besides help.c could include command-list.h, in which
case some of those MAYBE_UNUSED bits could become useful.

I actually wonder if the whole thing would be simpler if command-list.h
was a static tracked file with the declarations, and we generated
command-list.c with "extern const char *command_list[]", etc.

> --- >8 ---
> Makefile: include GENERATED_H in LIB_H
> 
> $(LIB_H), which is meant to be the list of header files that can
> affect (hence trigger recompilation) the objects that go in
> libgit.a, in a directory extracted from a tarball is computed by
> running "find \*.h" but instead computed with "ls-files \*.h" in a
> working tree managed by a git repository.  The former can include
> generated header files after a build, and omit them in a clean
> state.  The latter would not, as generated header files are by
> definition not tracked.
> 
> Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.

I do think this is slightly simpler to reason about than the existing
setup (though see my "should it just be a C file?" above).

Here's the patch that would make hdr-check work:

---
diff --git a/Makefile b/Makefile
index 87b68962ed..1eac8e7a7a 100644
--- a/Makefile
+++ b/Makefile
@@ -2780,7 +2780,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-GEN_HDRS := command-list.h unicode-width.h
+GEN_HDRS := unicode-width.h
 EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 71158f7d8b..7b0751e3e1 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -48,6 +48,7 @@ define_categories () {
 define_category_names () {
 	echo
 	echo "/* Category names */"
+	echo "MAYBE_UNUSED"
 	echo "static const char *category_names[] = {"
 	bit=0
 	category_list "$1" |
@@ -61,6 +62,7 @@ define_category_names () {
 }
 
 print_command_list () {
+	echo "MAYBE_UNUSED"
 	echo "static struct cmdname_help command_list[] = {"
 
 	command_list "$1" |
@@ -78,6 +80,7 @@ print_command_list () {
 
 print_config_list () {
 	cat <<EOF
+MAYBE_UNUSED
 static const char *config_name_list[] = {
 EOF
 	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
@@ -101,7 +104,8 @@ do
 	shift
 done
 
-echo "/* Automatically generated by generate-cmdlist.sh */
+echo "#include \"gettext.h\"
+/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
 	const char *name;
 	const char *help;

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-16 19:20       ` Jeff King
@ 2019-12-17  1:27         ` Emily Shaffer
  2019-12-17  1:40           ` Jonathan Nieder
  2019-12-17  5:22           ` Jeff King
  2019-12-17  1:43         ` Jonathan Nieder
  1 sibling, 2 replies; 12+ messages in thread
From: Emily Shaffer @ 2019-12-17  1:27 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Ramsay Jones

On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote:
> On Mon, Dec 16, 2019 at 10:55:40AM -0800, Junio C Hamano wrote:
> 
> > LIB_H could contain command-list.h (and other GENERATED_H files) if
> > we did this, but dups in dependency does not hurt in general, and I
> > did not find anything potentially problematic in the existing use of
> > $(LIB_H) in our Makefile.
> > 
> > How about doing this as a further clean-up?  I am reasonably sure
> > the status-quo description is correct, but I find the justification
> > a bit weak (in other words, I do not have a good answer to "who
> > cares if those that depend on $(LIB_H) are not rebuilt when
> > command-list.h gets rebuilt?")
> 
> Yeah, I don't think there's any change in behavior here, since with the
> exception of hdr-check, every mention of $(LIB_H) also mentioned
> $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the
> only item found in $(GENERATED_H).

To check my understanding - hdr-check just says "the headers are
syntactically correct", right? $HCO's target '-o /dev/null' says
"don't save the output", '-c' says "just compile, don't link", and '-xc'
says "in C"; it expands to a target for each file ending in .h but not
in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets,
so I think I understand hdr-check is compiling each header...

> 
> But this would enable us to start checking command-list.h. I'm on the
> fence on whether that's useful or not; the patch below makes it pass,
> but I'm not sure if that is really turning up any useful problems. I
> suppose somebody besides help.c could include command-list.h, in which
> case some of those MAYBE_UNUSED bits could become useful.

Firstly, I think if someone besides help.c includes command-list.h it
blows up because there's no include guards :)

My gut wants to say, "I need to be sure my generated header is correct!"
But it seems that will also be checked when I try to include that header
from something actually important. So maybe it's not actually useful.
But then it seems like hdr-check target isn't that useful for anybody,
since those headers should always be included sometime down the road (or
why have them). So that makes me think I must still be missing
something, like maybe I parsed hdr-check wrong.

> 
> I actually wonder if the whole thing would be simpler if command-list.h
> was a static tracked file with the declarations, and we generated
> command-list.c with "extern const char *command_list[]", etc.
> 
> > --- >8 ---
> > Makefile: include GENERATED_H in LIB_H
> > 
> > $(LIB_H), which is meant to be the list of header files that can
> > affect (hence trigger recompilation) the objects that go in
> > libgit.a, in a directory extracted from a tarball is computed by
> > running "find \*.h" but instead computed with "ls-files \*.h" in a
> > working tree managed by a git repository.  The former can include
> > generated header files after a build, and omit them in a clean
> > state.  The latter would not, as generated header files are by
> > definition not tracked.
> > 
> > Explicitly add $(GENERATED_H) to $(LIB_H) to make things consistent.
> 
> I do think this is slightly simpler to reason about than the existing
> setup (though see my "should it just be a C file?" above).
> 
> Here's the patch that would make hdr-check work:
> 
> ---
> diff --git a/Makefile b/Makefile
> index 87b68962ed..1eac8e7a7a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2780,7 +2780,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> -GEN_HDRS := command-list.h unicode-width.h
> +GEN_HDRS := unicode-width.h
>  EXCEPT_HDRS := $(GEN_HDRS) compat/% xdiff/%
>  ifndef GCRYPT_SHA256
>  	EXCEPT_HDRS += sha256/gcrypt.h
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> index 71158f7d8b..7b0751e3e1 100755
> --- a/generate-cmdlist.sh
> +++ b/generate-cmdlist.sh
> @@ -48,6 +48,7 @@ define_categories () {
>  define_category_names () {
>  	echo
>  	echo "/* Category names */"
> +	echo "MAYBE_UNUSED"
>  	echo "static const char *category_names[] = {"
>  	bit=0
>  	category_list "$1" |
> @@ -61,6 +62,7 @@ define_category_names () {
>  }
>  
>  print_command_list () {
> +	echo "MAYBE_UNUSED"
>  	echo "static struct cmdname_help command_list[] = {"
>  
>  	command_list "$1" |
> @@ -78,6 +80,7 @@ print_command_list () {
>  
>  print_config_list () {
>  	cat <<EOF
> +MAYBE_UNUSED
>  static const char *config_name_list[] = {
>  EOF
>  	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
> @@ -101,7 +104,8 @@ do
>  	shift
>  done
>  
> -echo "/* Automatically generated by generate-cmdlist.sh */
> +echo "#include \"gettext.h\"
> +/* Automatically generated by generate-cmdlist.sh */
>  struct cmdname_help {
>  	const char *name;
>  	const char *help;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-17  1:27         ` Emily Shaffer
@ 2019-12-17  1:40           ` Jonathan Nieder
  2019-12-17  5:22           ` Jeff King
  1 sibling, 0 replies; 12+ messages in thread
From: Jonathan Nieder @ 2019-12-17  1:40 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: Jeff King, Junio C Hamano, git, Ramsay Jones

Emily Shaffer wrote:
> On Mon, Dec 16, 2019 at 02:20:14PM -0500, Jeff King wrote:

>> But this would enable us to start checking command-list.h. I'm on the
>> fence on whether that's useful or not; the patch below makes it pass,
>> but I'm not sure if that is really turning up any useful problems. I
>> suppose somebody besides help.c could include command-list.h, in which
>> case some of those MAYBE_UNUSED bits could become useful.
>
> Firstly, I think if someone besides help.c includes command-list.h it
> blows up because there's no include guards :)
>
> My gut wants to say, "I need to be sure my generated header is correct!"
> But it seems that will also be checked when I try to include that header
> from something actually important. So maybe it's not actually useful.
> But then it seems like hdr-check target isn't that useful for anybody,
> since those headers should always be included sometime down the road (or
> why have them). So that makes me think I must still be missing
> something, like maybe I parsed hdr-check wrong.

"git log -Shdr-check Makefile" gives a hint:

  $ git log -Shdr-check Makefile
  commit ebb7baf02f69f2164b1f89148945d18c376fc6a8
  Author: Ramsay Jones <ramsay@ramsayjones.plus.com>
  Date:   Wed Sep 19 01:07:08 2018 +0100

      Makefile: add a hdr-check target

      Commit ef3ca95475 ("Add missing includes and forward declarations",
      2018-08-15) resulted from the author employing a manual method to
      create a C file consisting of a pair of pre-processor #include
      lines (for 'git-compat-util.h' and a given toplevel header), and
      fixing any resulting compiler errors or warnings.

      Add a Makefile target to automate this process.

So it's primarily about making sure that each header is #include-able
on its own (i.e., that it #includes the headers for any types it
relies on).

That seems like something I'd want to hold for command-list.h, too. :)

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-16 19:20       ` Jeff King
  2019-12-17  1:27         ` Emily Shaffer
@ 2019-12-17  1:43         ` Jonathan Nieder
  2019-12-17  5:28           ` Jeff King
  1 sibling, 1 reply; 12+ messages in thread
From: Jonathan Nieder @ 2019-12-17  1:43 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Emily Shaffer, Ramsay Jones

Jeff King wrote:

> I actually wonder if the whole thing would be simpler if command-list.h
> was a static tracked file with the declarations, and we generated
> command-list.c with "extern const char *command_list[]", etc.

Right, or a "command-list.inc" file.

extern-ing it seems like the simplest way to go.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-17  1:27         ` Emily Shaffer
  2019-12-17  1:40           ` Jonathan Nieder
@ 2019-12-17  5:22           ` Jeff King
  1 sibling, 0 replies; 12+ messages in thread
From: Jeff King @ 2019-12-17  5:22 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: Junio C Hamano, git, Ramsay Jones

On Mon, Dec 16, 2019 at 05:27:56PM -0800, Emily Shaffer wrote:

> > Yeah, I don't think there's any change in behavior here, since with the
> > exception of hdr-check, every mention of $(LIB_H) also mentioned
> > $(GENERATED_H). And in the case of hdr-check, we explicitly exclude the
> > only item found in $(GENERATED_H).
> 
> To check my understanding - hdr-check just says "the headers are
> syntactically correct", right? $HCO's target '-o /dev/null' says
> "don't save the output", '-c' says "just compile, don't link", and '-xc'
> says "in C"; it expands to a target for each file ending in .h but not
> in $EXCEPT_HDRS, and hdr-check calls each one of those expanded targets,
> so I think I understand hdr-check is compiling each header...

Sort of.  It's less about "syntactically correct" (which we'd find out
easily when we try to compile it) and more about "does not have
unexpected dependencies on other files".

E.g., imagine I write a header foo.h that mentions "struct bar", which
is declared in bar.h. If the only C file that uses that does:

  #include "bar.h"
  #include "foo.h"

then the compiler is happy. But I've laid a trap for somebody later, who
does just:

  #include "foo.h"

and gets an annoying compiler error (which is trivial to fix in this
example, but can sometimes get complicated to untangle).

So we declared a rule that header files should be self-sufficient
(modulo git-compat-util.h), and hdr-check is the way to find out if that
is true.

> > but I'm not sure if that is really turning up any useful problems. I
> > suppose somebody besides help.c could include command-list.h, in which
> > case some of those MAYBE_UNUSED bits could become useful.
> 
> Firstly, I think if someone besides help.c includes command-list.h it
> blows up because there's no include guards :)

Only if another header file does it, which could easily cause double
inclusion within the same source file. It's perfectly fine for another
translation unit to include it.

(Side note: builtin/help.c is declared in the Makefile as depending on
it, but doesn't seem to actually include it).

> My gut wants to say, "I need to be sure my generated header is correct!"
> But it seems that will also be checked when I try to include that header
> from something actually important. So maybe it's not actually useful.
> But then it seems like hdr-check target isn't that useful for anybody,
> since those headers should always be included sometime down the road (or
> why have them). So that makes me think I must still be missing
> something, like maybe I parsed hdr-check wrong.

I think this is the "it compiles now but you've laid a trap..." thing
mentioned above.

As it is, command-list.h _does_ have such a trap; you need to include
gettext.h first. But since so few callers use it (and are likely to use
it) nobody has really noticed or cared.

-Peff

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-17  1:43         ` Jonathan Nieder
@ 2019-12-17  5:28           ` Jeff King
  2019-12-17 11:35             ` vcxproj target, was " Johannes Schindelin
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff King @ 2019-12-17  5:28 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git, Emily Shaffer, Ramsay Jones

On Mon, Dec 16, 2019 at 05:43:21PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > I actually wonder if the whole thing would be simpler if command-list.h
> > was a static tracked file with the declarations, and we generated
> > command-list.c with "extern const char *command_list[]", etc.
> 
> Right, or a "command-list.inc" file.
> 
> extern-ing it seems like the simplest way to go.

If only. I took a brief look at this. Besides the Makefile chaos (did
you know that the vcxproj rule manually builds and git-adds
command-list.h? No idea what is going on there), it looks like we
dynamically generate the category bitfield, which is then used directly
in help.c. We _could_ declare those bitfields as externs themselves, but
part of the point is that the full list of categories is generated
dynamically from command-list.txt.

So we'd either split the list into two (one list of categories special
enough to be manually declared, and the rest that get generated
automatically) or we'd end up just duplicating the whole list.

Certainly this could all be untangled, but given that the system is
working just fine as it is, it doesn't seem worth anybody's time (and
the risk of weird follow-on problems) to adjust it.

-Peff

^ permalink raw reply	[flat|nested] 12+ messages in thread

* vcxproj target, was Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-17  5:28           ` Jeff King
@ 2019-12-17 11:35             ` Johannes Schindelin
  2019-12-19  4:51               ` Jeff King
  0 siblings, 1 reply; 12+ messages in thread
From: Johannes Schindelin @ 2019-12-17 11:35 UTC (permalink / raw)
  To: Jeff King
  Cc: Jonathan Nieder, Junio C Hamano, git, Emily Shaffer, Ramsay Jones

Hi Peff,

On Tue, 17 Dec 2019, Jeff King wrote:

> (did you know that the vcxproj rule manually builds and git-adds
> command-list.h? No idea what is going on there),

The idea of this is that contributors can clone the `vs/master` branch
from Git for Windows repository, check it out, and open it in Visual
Studio, then build.

Meaning: we cannot use any Unix shell scripts or Makefile targets to
generate _anything_. That's just not an option. It is such a foreign
concept in Visual Studio projects, it is very much Unix-y to think that
everything uses `make` anyway and everybody has access to a Unix shell
interpreter and the many Unix tools including `sed`, `awk`, etc.

Therefore we pre-generate all of those generated files, commit them, and
the user does not have to worry about getting ~700MB worth of compressed
data, unpack that to a ~2GB build environment that I like to call "Git
for Windows SDK", _just_ to generate those files.

That's what's going on there.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: vcxproj target, was Re: [PATCH] Makefile: drop GEN_HDRS
  2019-12-17 11:35             ` vcxproj target, was " Johannes Schindelin
@ 2019-12-19  4:51               ` Jeff King
  0 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2019-12-19  4:51 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Jonathan Nieder, Junio C Hamano, git, Emily Shaffer, Ramsay Jones

On Tue, Dec 17, 2019 at 12:35:58PM +0100, Johannes Schindelin wrote:

> On Tue, 17 Dec 2019, Jeff King wrote:
> 
> > (did you know that the vcxproj rule manually builds and git-adds
> > command-list.h? No idea what is going on there),
> 
> The idea of this is that contributors can clone the `vs/master` branch
> from Git for Windows repository, check it out, and open it in Visual
> Studio, then build.
> [...]
> That's what's going on there.

Thanks for satisfying my curiosity. Though I still think I stand by my
statement that the cost/benefit isn't really there to switch how we
handle command-list.h.

-Peff

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-12-19  4:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-13 23:25 [PATCH] Makefile: drop GEN_HDRS Junio C Hamano
2019-12-14  0:38 ` Jeff King
2019-12-14  1:00   ` Jeff King
2019-12-16 18:55     ` Junio C Hamano
2019-12-16 19:20       ` Jeff King
2019-12-17  1:27         ` Emily Shaffer
2019-12-17  1:40           ` Jonathan Nieder
2019-12-17  5:22           ` Jeff King
2019-12-17  1:43         ` Jonathan Nieder
2019-12-17  5:28           ` Jeff King
2019-12-17 11:35             ` vcxproj target, was " Johannes Schindelin
2019-12-19  4:51               ` Jeff King

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).