| Message ID | patch-2.3-6e164edb0b-20210617T095827Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Makefile: misc trivial fixes | expand |
Æ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.
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...
Æ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.
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.
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
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(-)