git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] Makefile: misc trivial fixes
@ 2021-06-17 10:01 Ævar Arnfjörð Bjarmason
  2021-06-17 10:01 ` [PATCH 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-17 10:01 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

I'm re-rolling ab/config-based-hooks-base to deal with (among other
things) a vcbuild failure. This doesn't fix that, but allows me to fix
that in one place, instead copy/pasting things all over the place.

The reason I'd need to do that is because we define GENERATED_H, but
then proceed not to use it when we should. This small series fixes
that in 2/3, and fixes a couple of adjacent trivial issues in 1/3 and
3/3.

Ævar Arnfjörð Bjarmason (3):
  Makefile: mark "check" target as .PHONY
  Makefile: stop hardcoding {command,config}-list.h
  Makefile: remove an out-of-date comment

 Makefile              | 13 +++++--------
 compat/vcbuild/README |  2 +-
 config.mak.uname      |  6 +++---
 3 files changed, 9 insertions(+), 12 deletions(-)

-- 
2.32.0.576.g59759b6ca7d


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

* [PATCH 1/3] Makefile: mark "check" target as .PHONY
  2021-06-17 10:01 [PATCH 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
@ 2021-06-17 10:01 ` Ævar Arnfjörð Bjarmason
  2021-06-17 20:04   ` Felipe Contreras
  2021-06-17 10:01 ` [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-17 10:01 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Fix a bug in 44c9e8594e (Fix up header file dependencies and add
sparse checking rules, 2005-07-03), we never marked the phony "check"
target as such.

Perhaps we should just remove it, since as of a combination of
912f9980d2 (Makefile: help people who run 'make check' by mistake,
2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing
target-specific variables, 2011-04-21) we've been suggesting the user
run "make sparse" directly.

But under that mode it still does something, as well as directing the
user to run "make test" under non-sparse. So let's punt that and
narrowly fix the PHONY bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index c3565fc0f8..7f984bce50 100644
--- a/Makefile
+++ b/Makefile
@@ -2910,6 +2910,7 @@ hdr-check: $(HCO)
 style:
 	git clang-format --style file --diff --extensions c,h
 
+.PHONY: check
 check: config-list.h command-list.h
 	@if sparse; \
 	then \
-- 
2.32.0.576.g59759b6ca7d


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

* [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h
  2021-06-17 10:01 [PATCH 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
  2021-06-17 10:01 ` [PATCH 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
@ 2021-06-17 10:01 ` Ævar Arnfjörð Bjarmason
  2021-06-17 20:14   ` Felipe Contreras
  2021-06-17 10:01 ` [PATCH 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
  2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
  3 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-17 10:01 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Change various places that hardcode the names of these two files to
refer to either $(GENERATED_H), or to a new generated-hdrs
target. That target is consistent with the *-objs targets I recently
added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
& objects targets, 2021-02-23).

I have not tested the Windows-specific change in config.mak.uname
being made here, but we use other variables from the Makefile in the
same block, and the GENERATED_H is fully defined before we include
config.mak.uname.

Hardcoding command-list.h there seems to have been a case of
copy/paste programming in dce7d29551 (msvc: support building Git using
MS Visual C++, 2019-06-25). The config-list.h was added later in
709df95b78 (help: move list_config_help to builtin/help, 2020-04-16).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile              | 6 ++++--
 compat/vcbuild/README | 2 +-
 config.mak.uname      | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 7f984bce50..66f5ded3a4 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+.PHONY: generated-hdrs
+generated-hdrs: $(GENERATED_H)
 
 LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
 	$(FIND) . \
@@ -2888,7 +2890,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
+EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
@@ -2911,7 +2913,7 @@ style:
 	git clang-format --style file --diff --extensions c,h
 
 .PHONY: check
-check: config-list.h command-list.h
+check: $(GENERATED_H)
 	@if sparse; \
 	then \
 		echo >&2 "Use 'make sparse' instead"; \
diff --git a/compat/vcbuild/README b/compat/vcbuild/README
index 51fb083dbb..29ec1d0f10 100644
--- a/compat/vcbuild/README
+++ b/compat/vcbuild/README
@@ -92,7 +92,7 @@ The Steps of Build Git with VS2008
    the git operations.
 
 3. Inside Git's directory run the command:
-       make command-list.h config-list.h
+       make generated-hdrs
    to generate the header file needed to compile git.
 
 4. Then either build Git with the GNU Make Makefile in the Git projects
diff --git a/config.mak.uname b/config.mak.uname
index cb443b4e02..af4c5c540e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -722,9 +722,9 @@ vcxproj:
 	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
 	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
-	# Add command-list.h and config-list.h
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h
-	git add -f config-list.h command-list.h
+	# Add generated headers
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(GENERATED_H)
+	git add -f $(GENERATED_H)
 
 	# Add scripts
 	rm -f perl/perl.mak
-- 
2.32.0.576.g59759b6ca7d


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

* [PATCH 3/3] Makefile: remove an out-of-date comment
  2021-06-17 10:01 [PATCH 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
  2021-06-17 10:01 ` [PATCH 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
  2021-06-17 10:01 ` [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
@ 2021-06-17 10:01 ` Ævar Arnfjörð Bjarmason
  2021-06-17 20:55   ` Felipe Contreras
  2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
  3 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-17 10:01 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

This comment added in dfea575017 (Makefile: lazily compute header
dependencies, 2010-01-26) has been out of date since
92b88eba9f (Makefile: use `git ls-files` to list header files, if
possible, 2019-03-04), when we did exactly what it tells us not to do
and added $(GENERATED_H) to $(OBJECTS) dependencies.

The rest of it was also somewhere between inaccurate and outdated,
since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
it's not followed by a list of header files, that got moved earlier in
the file into LIB_H in b8ba629264 (Makefile: fold MISC_H into LIB_H,
2012-06-20).

Let's just remove it entirely, to the extent that we have anything
useful to say here the comment on the
"USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this
change does the job for us.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/Makefile b/Makefile
index 66f5ded3a4..29a152cd4f 100644
--- a/Makefile
+++ b/Makefile
@@ -2503,12 +2503,6 @@ ifneq ($(dep_files_present),)
 include $(dep_files_present)
 endif
 else
-# Dependencies on header files, for platforms that do not support
-# the gcc -MMD option.
-#
-# Dependencies on automatically generated headers such as command-list.h
-# should _not_ be included here, since they are necessary even when
-# building an object for the first time.
 
 $(OBJECTS): $(LIB_H) $(GENERATED_H)
 endif
-- 
2.32.0.576.g59759b6ca7d


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

* RE: [PATCH 1/3] Makefile: mark "check" target as .PHONY
  2021-06-17 10:01 ` [PATCH 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
@ 2021-06-17 20:04   ` Felipe Contreras
  0 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-06-17 20:04 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:
> --- a/Makefile
> +++ b/Makefile
> @@ -2910,6 +2910,7 @@ hdr-check: $(HCO)
>  style:
>  	git clang-format --style file --diff --extensions c,h
>  
> +.PHONY: check
>  check: config-list.h command-list.h
>  	@if sparse; \
>  	then \

Obviously correct.

-- 
Felipe Contreras

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

* RE: [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h
  2021-06-17 10:01 ` [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
@ 2021-06-17 20:14   ` Felipe Contreras
  2021-06-17 20:58     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 15+ messages in thread
From: Felipe Contreras @ 2021-06-17 20:14 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:
> Change various places that hardcode the names of these two files to
> refer to either $(GENERATED_H), or to a new generated-hdrs
> target.

Avoiding hard-coded things is generally a good idea, and I can smell
there's an advantage nearby, but it's not stated.

Can you spell out what you are trying to achieve?

> Hardcoding command-list.h there seems to have been a case of
> copy/paste programming in dce7d29551 (msvc: support building Git using
> MS Visual C++, 2019-06-25).

Actually that's not the commit, it's this one:

976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
Studio solution, 2019-07-29)

The changes themselves look good to me.

-- 
Felipe Contreras

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

* RE: [PATCH 3/3] Makefile: remove an out-of-date comment
  2021-06-17 10:01 ` [PATCH 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
@ 2021-06-17 20:55   ` Felipe Contreras
  0 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-06-17 20:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:
> This comment added in dfea575017 (Makefile: lazily compute header
> dependencies, 2010-01-26) has been out of date since
> 92b88eba9f (Makefile: use `git ls-files` to list header files, if
> possible, 2019-03-04), when we did exactly what it tells us not to do
> and added $(GENERATED_H) to $(OBJECTS) dependencies.

Very true.

> The rest of it was also somewhere between inaccurate and outdated,
> since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
> it's not followed by a list of header files, that got moved earlier in
> the file into LIB_H in b8ba629264 (Makefile: fold MISC_H into LIB_H,
> 2012-06-20).

I don't see the point in mentioning b8ba629264 twice, perhaps you meant:

60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H into LIB_H, 2012-07-06)

This commit also removed part of the comment:

  # XXX. Please check occasionally that these include all dependencies
  # gcc detects!

I have tried to understand what was the purpose of that comment in the
past, but I don't get it, it says:

  Dependencies on automatically generated headers such as command-list.h
  should _not_ be included here, since they are necessary even when
  building an object for the first time.

Why would it matter if we are building the object for the first time, or
rebuilding it? If we need command-list.h to build help.o once, we need
it always. What am I missing?

It seems to me this comment never made sense.

> --- a/Makefile
> +++ b/Makefile
> @@ -2503,12 +2503,6 @@ ifneq ($(dep_files_present),)
>  include $(dep_files_present)
>  endif
>  else
> -# Dependencies on header files, for platforms that do not support
> -# the gcc -MMD option.
> -#
> -# Dependencies on automatically generated headers such as command-list.h
> -# should _not_ be included here, since they are necessary even when
> -# building an object for the first time.
>  

Can we remove that extra space once we are at it?

>  $(OBJECTS): $(LIB_H) $(GENERATED_H)
>  endif

The change itself looks good to me.

Cheers.

-- 
Felipe Contreras

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

* Re: [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h
  2021-06-17 20:14   ` Felipe Contreras
@ 2021-06-17 20:58     ` Ævar Arnfjörð Bjarmason
  2021-06-17 21:58       ` Felipe Contreras
  0 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-17 20:58 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin


On Thu, Jun 17 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> Change various places that hardcode the names of these two files to
>> refer to either $(GENERATED_H), or to a new generated-hdrs
>> target.
>
> Avoiding hard-coded things is generally a good idea, and I can smell
> there's an advantage nearby, but it's not stated.
>
> Can you spell out what you are trying to achieve?

It's hinted at in the CL, but this is series 1/3 of a re-roll of the
base topic for config-based hooks, real use of this is made in step 2/3,
3/3 has a better overview:
http://lore.kernel.org/git/cover-00.27-0000000000-20210617T101216Z-avarab@gmail.com

>> Hardcoding command-list.h there seems to have been a case of
>> copy/paste programming in dce7d29551 (msvc: support building Git using
>> MS Visual C++, 2019-06-25).
>
> Actually that's not the commit, it's this one:
>
> 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
> Studio solution, 2019-07-29)

Thanks, I had both in my buffers somewhere and copied over the wrong
one. Will correct pending further feedback...

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

* Re: [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h
  2021-06-17 20:58     ` Ævar Arnfjörð Bjarmason
@ 2021-06-17 21:58       ` Felipe Contreras
  2021-06-18  8:05         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 15+ messages in thread
From: Felipe Contreras @ 2021-06-17 21:58 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Felipe Contreras
  Cc: git, Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin

Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jun 17 2021, Felipe Contreras wrote:
> 
> > Ævar Arnfjörð Bjarmason wrote:
> >> Change various places that hardcode the names of these two files to
> >> refer to either $(GENERATED_H), or to a new generated-hdrs
> >> target.
> >
> > Avoiding hard-coded things is generally a good idea, and I can smell
> > there's an advantage nearby, but it's not stated.
> >
> > Can you spell out what you are trying to achieve?
> 
> It's hinted at in the CL,

Yes, but the commit message should stand on its own.

> but this is series 1/3 of a re-roll of the
> base topic for config-based hooks, real use of this is made in step 2/3,
> 3/3 has a better overview:
> http://lore.kernel.org/git/cover-00.27-0000000000-20210617T101216Z-avarab@gmail.com

Yeah, I read the cover letter afterwards, but I'm just putting my
reviewer cap; the rationale belongs in the commit message: we will want
more generated files in GENERATED_H.

Also, I presume you meant this one:

https://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com/

Cheers.

-- 
Felipe Contreras

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

* Re: [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h
  2021-06-17 21:58       ` Felipe Contreras
@ 2021-06-18  8:05         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-18  8:05 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin


On Thu, Jun 17 2021, Felipe Contreras wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Jun 17 2021, Felipe Contreras wrote:
>> 
>> > Ævar Arnfjörð Bjarmason wrote:
>> >> Change various places that hardcode the names of these two files to
>> >> refer to either $(GENERATED_H), or to a new generated-hdrs
>> >> target.
>> >
>> > Avoiding hard-coded things is generally a good idea, and I can smell
>> > there's an advantage nearby, but it's not stated.
>> >
>> > Can you spell out what you are trying to achieve?
>> 
>> It's hinted at in the CL,
>
> Yes, but the commit message should stand on its own.
>
>> but this is series 1/3 of a re-roll of the
>> base topic for config-based hooks, real use of this is made in step 2/3,
>> 3/3 has a better overview:
>> http://lore.kernel.org/git/cover-00.27-0000000000-20210617T101216Z-avarab@gmail.com
>
> Yeah, I read the cover letter afterwards, but I'm just putting my
> reviewer cap; the rationale belongs in the commit message: we will want
> more generated files in GENERATED_H.
>


Thanks, I'll reword it. I initially figured in this case it was better
not to distract from a stand-alone change with what inspired it, but
will do.

> Also, I presume you meant this one:
> https://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com/

That's 2/3, but yes. That comes after this one. I meant to link to 3/3
to give the general overview of how this relates to that re-rolled topic
at large.

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

* [PATCH v2 0/3] Makefile: misc trivial fixes
  2021-06-17 10:01 [PATCH 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2021-06-17 10:01 ` [PATCH 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
@ 2021-06-29 19:03 ` Ævar Arnfjörð Bjarmason
  2021-06-29 19:03   ` [PATCH v2 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
                     ` (3 more replies)
  3 siblings, 4 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 19:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Felipe Contreras, SZEDER Gábor,
	Eric Sunshine, René Scharfe,
	Ævar Arnfjörð Bjarmason

A base topic for some larger changes. See the v1 CL for a summary:
http://lore.kernel.org/git/cover-0.3-0000000000-20210617T095827Z-avarab@gmail.com

The only changes since v1 are to commit message issues pointed out by
Felipe and a trivial whitespace change. I also updated the commit
message of 2/3 as he suggested to point out why the change is being
made.

Ævar Arnfjörð Bjarmason (3):
  Makefile: mark "check" target as .PHONY
  Makefile: stop hardcoding {command,config}-list.h
  Makefile: remove an out-of-date comment

 Makefile              | 14 +++++---------
 compat/vcbuild/README |  2 +-
 config.mak.uname      |  6 +++---
 3 files changed, 9 insertions(+), 13 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  27c94247f8 Makefile: mark "check" target as .PHONY
1:  6e164edb0b ! 2:  983d072d52 Makefile: stop hardcoding {command,config}-list.h
    @@ Commit message
         added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
         & objects targets, 2021-02-23).
     
    +    A follow-up commit (not part of this series) will add a new generated
    +    hook-list.h. By doing this refactoring we'll only need to add the new
    +    file to the GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README
    +    etc.
    +
         I have not tested the Windows-specific change in config.mak.uname
         being made here, but we use other variables from the Makefile in the
         same block, and the GENERATED_H is fully defined before we include
         config.mak.uname.
     
         Hardcoding command-list.h there seems to have been a case of
    -    copy/paste programming in dce7d29551 (msvc: support building Git using
    -    MS Visual C++, 2019-06-25). The config-list.h was added later in
    -    709df95b78 (help: move list_config_help to builtin/help, 2020-04-16).
    +    copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
    +    pre-generate the Visual Studio solution, 2019-07-29). The
    +    config-list.h was added later in 709df95b78 (help: move
    +    list_config_help to builtin/help, 2020-04-16).
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  ddae86802e ! 3:  44a4781218 Makefile: remove an out-of-date comment
    @@ Commit message
         The rest of it was also somewhere between inaccurate and outdated,
         since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
         it's not followed by a list of header files, that got moved earlier in
    -    the file into LIB_H in b8ba629264 (Makefile: fold MISC_H into LIB_H,
    -    2012-06-20).
    +    the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
    +    into LIB_H, 2012-07-06).
     
         Let's just remove it entirely, to the extent that we have anything
         useful to say here the comment on the
    @@ Makefile: ifneq ($(dep_files_present),)
     -# Dependencies on automatically generated headers such as command-list.h
     -# should _not_ be included here, since they are necessary even when
     -# building an object for the first time.
    - 
    +-
      $(OBJECTS): $(LIB_H) $(GENERATED_H)
      endif
    + 
-- 
2.32.0.615.g90fb4d7369


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

* [PATCH v2 1/3] Makefile: mark "check" target as .PHONY
  2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
@ 2021-06-29 19:03   ` Ævar Arnfjörð Bjarmason
  2021-06-29 19:03   ` [PATCH v2 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 19:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Felipe Contreras, SZEDER Gábor,
	Eric Sunshine, René Scharfe,
	Ævar Arnfjörð Bjarmason

Fix a bug in 44c9e8594e (Fix up header file dependencies and add
sparse checking rules, 2005-07-03), we never marked the phony "check"
target as such.

Perhaps we should just remove it, since as of a combination of
912f9980d2 (Makefile: help people who run 'make check' by mistake,
2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing
target-specific variables, 2011-04-21) we've been suggesting the user
run "make sparse" directly.

But under that mode it still does something, as well as directing the
user to run "make test" under non-sparse. So let's punt that and
narrowly fix the PHONY bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index c3565fc0f8..7f984bce50 100644
--- a/Makefile
+++ b/Makefile
@@ -2910,6 +2910,7 @@ hdr-check: $(HCO)
 style:
 	git clang-format --style file --diff --extensions c,h
 
+.PHONY: check
 check: config-list.h command-list.h
 	@if sparse; \
 	then \
-- 
2.32.0.615.g90fb4d7369


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

* [PATCH v2 2/3] Makefile: stop hardcoding {command,config}-list.h
  2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
  2021-06-29 19:03   ` [PATCH v2 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
@ 2021-06-29 19:03   ` Ævar Arnfjörð Bjarmason
  2021-06-29 19:03   ` [PATCH v2 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
  2021-06-29 19:25   ` [PATCH v2 0/3] Makefile: misc trivial fixes Felipe Contreras
  3 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 19:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Felipe Contreras, SZEDER Gábor,
	Eric Sunshine, René Scharfe,
	Ævar Arnfjörð Bjarmason

Change various places that hardcode the names of these two files to
refer to either $(GENERATED_H), or to a new generated-hdrs
target. That target is consistent with the *-objs targets I recently
added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
& objects targets, 2021-02-23).

A follow-up commit (not part of this series) will add a new generated
hook-list.h. By doing this refactoring we'll only need to add the new
file to the GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README
etc.

I have not tested the Windows-specific change in config.mak.uname
being made here, but we use other variables from the Makefile in the
same block, and the GENERATED_H is fully defined before we include
config.mak.uname.

Hardcoding command-list.h there seems to have been a case of
copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29). The
config-list.h was added later in 709df95b78 (help: move
list_config_help to builtin/help, 2020-04-16).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile              | 6 ++++--
 compat/vcbuild/README | 2 +-
 config.mak.uname      | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 7f984bce50..66f5ded3a4 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+.PHONY: generated-hdrs
+generated-hdrs: $(GENERATED_H)
 
 LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
 	$(FIND) . \
@@ -2888,7 +2890,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
+EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
@@ -2911,7 +2913,7 @@ style:
 	git clang-format --style file --diff --extensions c,h
 
 .PHONY: check
-check: config-list.h command-list.h
+check: $(GENERATED_H)
 	@if sparse; \
 	then \
 		echo >&2 "Use 'make sparse' instead"; \
diff --git a/compat/vcbuild/README b/compat/vcbuild/README
index 51fb083dbb..29ec1d0f10 100644
--- a/compat/vcbuild/README
+++ b/compat/vcbuild/README
@@ -92,7 +92,7 @@ The Steps of Build Git with VS2008
    the git operations.
 
 3. Inside Git's directory run the command:
-       make command-list.h config-list.h
+       make generated-hdrs
    to generate the header file needed to compile git.
 
 4. Then either build Git with the GNU Make Makefile in the Git projects
diff --git a/config.mak.uname b/config.mak.uname
index cb443b4e02..af4c5c540e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -722,9 +722,9 @@ vcxproj:
 	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
 	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
-	# Add command-list.h and config-list.h
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h
-	git add -f config-list.h command-list.h
+	# Add generated headers
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(GENERATED_H)
+	git add -f $(GENERATED_H)
 
 	# Add scripts
 	rm -f perl/perl.mak
-- 
2.32.0.615.g90fb4d7369


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

* [PATCH v2 3/3] Makefile: remove an out-of-date comment
  2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
  2021-06-29 19:03   ` [PATCH v2 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
  2021-06-29 19:03   ` [PATCH v2 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
@ 2021-06-29 19:03   ` Ævar Arnfjörð Bjarmason
  2021-06-29 19:25   ` [PATCH v2 0/3] Makefile: misc trivial fixes Felipe Contreras
  3 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-29 19:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Felipe Contreras, SZEDER Gábor,
	Eric Sunshine, René Scharfe,
	Ævar Arnfjörð Bjarmason

This comment added in dfea575017 (Makefile: lazily compute header
dependencies, 2010-01-26) has been out of date since
92b88eba9f (Makefile: use `git ls-files` to list header files, if
possible, 2019-03-04), when we did exactly what it tells us not to do
and added $(GENERATED_H) to $(OBJECTS) dependencies.

The rest of it was also somewhere between inaccurate and outdated,
since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
it's not followed by a list of header files, that got moved earlier in
the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
into LIB_H, 2012-07-06).

Let's just remove it entirely, to the extent that we have anything
useful to say here the comment on the
"USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this
change does the job for us.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/Makefile b/Makefile
index 66f5ded3a4..14996b6840 100644
--- a/Makefile
+++ b/Makefile
@@ -2503,13 +2503,6 @@ ifneq ($(dep_files_present),)
 include $(dep_files_present)
 endif
 else
-# Dependencies on header files, for platforms that do not support
-# the gcc -MMD option.
-#
-# Dependencies on automatically generated headers such as command-list.h
-# should _not_ be included here, since they are necessary even when
-# building an object for the first time.
-
 $(OBJECTS): $(LIB_H) $(GENERATED_H)
 endif
 
-- 
2.32.0.615.g90fb4d7369


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

* RE: [PATCH v2 0/3] Makefile: misc trivial fixes
  2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2021-06-29 19:03   ` [PATCH v2 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
@ 2021-06-29 19:25   ` Felipe Contreras
  3 siblings, 0 replies; 15+ messages in thread
From: Felipe Contreras @ 2021-06-29 19:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Emily Shaffer, Jeff Hostetler,
	Johannes Schindelin, Felipe Contreras, SZEDER Gábor,
	Eric Sunshine, René Scharfe,
	Ævar Arnfjörð Bjarmason

Ævar Arnfjörð Bjarmason wrote:
> A base topic for some larger changes. See the v1 CL for a summary:
> http://lore.kernel.org/git/cover-0.3-0000000000-20210617T095827Z-avarab@gmail.com
> 
> The only changes since v1 are to commit message issues pointed out by
> Felipe and a trivial whitespace change. I also updated the commit
> message of 2/3 as he suggested to point out why the change is being
> made.

This version looks good to me:

Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>

-- 
Felipe Contreras

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

end of thread, other threads:[~2021-06-29 19:25 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 10:01 [PATCH 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
2021-06-17 10:01 ` [PATCH 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
2021-06-17 20:04   ` Felipe Contreras
2021-06-17 10:01 ` [PATCH 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
2021-06-17 20:14   ` Felipe Contreras
2021-06-17 20:58     ` Ævar Arnfjörð Bjarmason
2021-06-17 21:58       ` Felipe Contreras
2021-06-18  8:05         ` Ævar Arnfjörð Bjarmason
2021-06-17 10:01 ` [PATCH 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
2021-06-17 20:55   ` Felipe Contreras
2021-06-29 19:03 ` [PATCH v2 0/3] Makefile: misc trivial fixes Ævar Arnfjörð Bjarmason
2021-06-29 19:03   ` [PATCH v2 1/3] Makefile: mark "check" target as .PHONY Ævar Arnfjörð Bjarmason
2021-06-29 19:03   ` [PATCH v2 2/3] Makefile: stop hardcoding {command,config}-list.h Ævar Arnfjörð Bjarmason
2021-06-29 19:03   ` [PATCH v2 3/3] Makefile: remove an out-of-date comment Ævar Arnfjörð Bjarmason
2021-06-29 19:25   ` [PATCH v2 0/3] Makefile: misc trivial fixes Felipe Contreras

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).