* [PATCH 1/9] Makefile: add a hdr-check target @ 2018-09-19 0:07 Ramsay Jones 2018-09-19 17:49 ` Martin Ågren 2018-09-20 14:26 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Ramsay Jones @ 2018-09-19 0:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list 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. This implementation relies on the '-include' and '-xc' arguments to the 'gcc' and 'clang' compilers, which allows us to effectively create the required C compilation unit on-the-fly. This limits the portability of this solution to those systems which have such a compiler. The new 'hdr-check' target can be used to check most header files in the project (for various reasons, the 'compat' and 'xdiff' directories are not included). Also, note that individual header files can be checked directly using the '.hco' extension (read: Hdr-Check Object) like so: $ make config.hco HDR config.h $ Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Makefile | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Makefile b/Makefile index b567ccca45..835030e22b 100644 --- a/Makefile +++ b/Makefile @@ -1793,6 +1793,7 @@ ifndef V QUIET_MSGFMT = @echo ' ' MSGFMT $@; QUIET_GCOV = @echo ' ' GCOV $@; QUIET_SP = @echo ' ' SP $<; + QUIET_HDR = @echo ' ' HDR $<; QUIET_RC = @echo ' ' RC $@; QUIET_SUBDIR0 = +@subdir= QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ @@ -2675,6 +2676,17 @@ $(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% +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) + +$(HCO): %.hco: %.h FORCE + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< + +.PHONY: hdr-check $(HCO) +hdr-check: $(HCO) + .PHONY: style style: git clang-format --style file --diff --extensions c,h -- 2.19.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/9] Makefile: add a hdr-check target 2018-09-19 0:07 [PATCH 1/9] Makefile: add a hdr-check target Ramsay Jones @ 2018-09-19 17:49 ` Martin Ågren 2018-09-19 19:37 ` Ramsay Jones 2018-09-20 14:26 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Martin Ågren @ 2018-09-19 17:49 UTC (permalink / raw) To: Ramsay Jones; +Cc: Junio C Hamano, Jeff King, Git Mailing List Hi Ramsay, On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE > .PHONY: sparse $(SP_OBJ) > sparse: $(SP_OBJ) > > +GEN_HDRS := command-list.h unicode-width.h Most of the things happening around here are obvious steps towards the end-goal and seem to logically belong here, together. This one though, "generated headers"(?) seems like it is more general in nature, so could perhaps live somewhere else. Actually, we have a variable `GENERATED_H` which seems to try to do more or less the same thing. It lists just one file, though, command-list.h. And unicode-width.h seems to be tracked in git.git. Maybe use `GENERATED_H` instead, and list unicode-width.h on the next line instead? Or am I completely misreading "GEN_HDRS"? > +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% > +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) > +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) > + > +$(HCO): %.hco: %.h FORCE > + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< > + > +.PHONY: hdr-check $(HCO) > +hdr-check: $(HCO) > + > .PHONY: style > style: > git clang-format --style file --diff --extensions c,h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/9] Makefile: add a hdr-check target 2018-09-19 17:49 ` Martin Ågren @ 2018-09-19 19:37 ` Ramsay Jones 2018-09-20 5:53 ` Martin Ågren 0 siblings, 1 reply; 7+ messages in thread From: Ramsay Jones @ 2018-09-19 19:37 UTC (permalink / raw) To: Martin Ågren; +Cc: Junio C Hamano, Jeff King, Git Mailing List On 19/09/18 18:49, Martin Ågren wrote: > Hi Ramsay, > > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: >> @@ -2675,6 +2676,17 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE >> .PHONY: sparse $(SP_OBJ) >> sparse: $(SP_OBJ) >> >> +GEN_HDRS := command-list.h unicode-width.h > > Most of the things happening around here are obvious steps towards the > end-goal and seem to logically belong here, together. This one though, > "generated headers"(?) seems like it is more general in nature, so could > perhaps live somewhere else. Yes, generated headers, but no, not more general. ;-) I originally included those headers directly in the EXCEPT_HDRS macro, along with the list of excluded directories (which was much longer at one point). The 'command-list.h' is generated as part of the build (and so may or may not be part of the LIB_H macro), whereas the unicode-width.h header is only re-generated when someone runs the 'contrib/update-unicode/update_unicode.sh' script (presumably when a new standard version is announced) and commits the result. Both headers fail the 'hdr-check', although both generator scripts could be 'fixed' so that they passed. I just didn't think it was worth the effort - neither header was likely to be #included anywhere else. I guess I could eliminate that macro, absorbing it into EXCEPT_HDRS, if that would lead to less confusion ... [I suspect the fact that LIB_H (almost always) contains 'command-list.h' has not been noticed ... :-P ] > Actually, we have a variable `GENERATED_H` which seems to try to do more > or less the same thing. It lists just one file, though, command-list.h. > And unicode-width.h seems to be tracked in git.git. Hmm, GENERATED_H seems only to be used by the i18n part of the makefile and, as a result of its appearance in LIB_H, sometimes results in command-list.h appearing twice in LOCALIZED_C. (which is probably not a problem). ATB, Ramsay Jones > Maybe use `GENERATED_H` instead, and list unicode-width.h on the next > line instead? Or am I completely misreading "GEN_HDRS"? > >> +EXCEPT_HDRS := $(GEN_HDRS) compat% xdiff% >> +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(patsubst ./%,%,$(LIB_H))) >> +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) >> + >> +$(HCO): %.hco: %.h FORCE >> + $(QUIET_HDR)$(CC) -include git-compat-util.h -I. -o /dev/null -c -xc $< >> + >> +.PHONY: hdr-check $(HCO) >> +hdr-check: $(HCO) >> + >> .PHONY: style >> style: >> git clang-format --style file --diff --extensions c,h > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/9] Makefile: add a hdr-check target 2018-09-19 19:37 ` Ramsay Jones @ 2018-09-20 5:53 ` Martin Ågren 0 siblings, 0 replies; 7+ messages in thread From: Martin Ågren @ 2018-09-20 5:53 UTC (permalink / raw) To: Ramsay Jones; +Cc: Junio C Hamano, Jeff King, Git Mailing List On Wed, 19 Sep 2018 at 22:15, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > On 19/09/18 18:49, Martin Ågren wrote: > > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > >> +GEN_HDRS := command-list.h unicode-width.h > > > > Most of the things happening around here are obvious steps towards the > > end-goal and seem to logically belong here, together. This one though, > > "generated headers"(?) seems like it is more general in nature, so could > > perhaps live somewhere else. > > Yes, generated headers, but no, not more general. ;-) > The 'command-list.h' is generated as part of the build > (and so may or may not be part of the LIB_H macro), whereas > the unicode-width.h header is only re-generated when someone > runs the 'contrib/update-unicode/update_unicode.sh' script > (presumably when a new standard version is announced) and > commits the result. Ah, I wasn't aware of how unicode-width.h works, thanks. > Both headers fail the 'hdr-check', although both generator > scripts could be 'fixed' so that they passed. I just didn't > think it was worth the effort - neither header was likely > to be #included anywhere else. With the above background, I'd tend to agree. > I guess I could eliminate > that macro, absorbing it into EXCEPT_HDRS, if that would > lead to less confusion ... I'm just a single data point, so don't trust my experience too much. > > Actually, we have a variable `GENERATED_H` which seems to try to do more > > or less the same thing. It lists just one file, though, command-list.h. > > Hmm, GENERATED_H seems only to be used by the i18n part of the > makefile and, as a result of its appearance in LIB_H, sometimes > results in command-list.h appearing twice in LOCALIZED_C. Just thinking out loud, I suppose you could use $(GENERATED_H) instead of hard-coding command-list.h in your patch. But with the background you explained above, I think there's a good counter-argument to be made: When we gain new auto-generated headers, we wouldn't actually mind `make hdr-check` failing. We'd get the opportunity to decide whether making the new header pass the check is worth it, or whether the correct solution is to blacklist the auto-generated header. That's probably better than just blacklisting the new header by default so that we don't even notice that it has a potential problem. So I think your approach makes sense. I stumbled on the similarity of the name to something we already have. Maybe something like # Ignore various generated files, rather than updating the # generating scripts for little or no benefit. GEN_HDRS := ... or a remark in the commit message, or rolling this into EXCEPT_HDRS, but I'd be perfectly fine just leaving this as it is. Please trust your own judgment more than mine. Thanks Martin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/9] Makefile: add a hdr-check target 2018-09-19 0:07 [PATCH 1/9] Makefile: add a hdr-check target Ramsay Jones 2018-09-19 17:49 ` Martin Ågren @ 2018-09-20 14:26 ` Junio C Hamano 2018-09-20 16:03 ` Ramsay Jones 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2018-09-20 14:26 UTC (permalink / raw) To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > 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. It makes sense to have tool do what we do not have to do manually. One thing that makes me wonder with the patch is that the new check command does not seem to need to see what is on CFLAGS and friends. Having seen that "make DEVELOPER=1" adds more -W... on the command line of the compiler and makes a build fail on a source that otherwise would build, I am wondering if there are some (subset of) options that the header-check command line wants to give to the compiler. Of course, there are also conditionally compiled sections of code, which are affected by the choice of -DMACRO=VALUE; how does this new feature handle that? > Add a Makefile target to automate this process. This implementation > relies on the '-include' and '-xc' arguments to the 'gcc' and 'clang' > compilers, which allows us to effectively create the required C > compilation unit on-the-fly. This limits the portability of this > solution to those systems which have such a compiler. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/9] Makefile: add a hdr-check target 2018-09-20 14:26 ` Junio C Hamano @ 2018-09-20 16:03 ` Ramsay Jones 2018-09-20 18:49 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Ramsay Jones @ 2018-09-20 16:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, GIT Mailing-list, Elijah Newren On 20/09/18 15:26, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> 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. > > It makes sense to have tool do what we do not have to do manually. > > One thing that makes me wonder with the patch is that the new check > command does not seem to need to see what is on CFLAGS and friends. > Having seen that "make DEVELOPER=1" adds more -W... on the command > line of the compiler and makes a build fail on a source that > otherwise would build, I am wondering if there are some (subset of) > options that the header-check command line wants to give to the > compiler. Yes, this was one of my first concerns (I even asked Elijah what compiler options he used), but I was getting useful results without passing CFLAGS, so I just ignored that issue ... :-D [The 'on-the-fly' compilation units don't correspond to any _actual_ compilation unit, so it's not easy to use existing rules ... but we could use 'hco' rule specific definitions to add flags, I suppose ...] > Of course, there are also conditionally compiled sections of code, > which are affected by the choice of -DMACRO=VALUE; how does this new > feature handle that? Indeed. This bothered me as well. The 'compat' directory does not follow the 'usual pattern' of the main headers and is particularly sensitive to the lack of various -DMACROs. I had initially included _all_ sub-directories in the 'exclude list' (to follow what Elijah had done), but then removed one at a time ... I am open to suggestions for improvements. ;-) ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/9] Makefile: add a hdr-check target 2018-09-20 16:03 ` Ramsay Jones @ 2018-09-20 18:49 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2018-09-20 18:49 UTC (permalink / raw) To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list, Elijah Newren Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > Yes, this was one of my first concerns (I even asked Elijah what > compiler options he used), but I was getting useful results without > passing CFLAGS, so I just ignored that issue ... :-D > ... > Indeed. This bothered me as well. The 'compat' directory does not > follow the 'usual pattern' of the main headers and is particularly > sensitive to the lack of various -DMACROs. I had initially included > _all_ sub-directories in the 'exclude list' (to follow what Elijah > had done), but then removed one at a time ... > > I am open to suggestions for improvements. ;-) Perhaps it is sufficient to add a mention of these two issues in log message and an in-code comment nearby the .hco rule in Makefile, so that people can come back later to discover what design choice we made (i.e. "without worrying about these two issues, we are getting useful enough results, so we decided it is OK at least for now not to worry about them"). Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-20 18:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-19 0:07 [PATCH 1/9] Makefile: add a hdr-check target Ramsay Jones 2018-09-19 17:49 ` Martin Ågren 2018-09-19 19:37 ` Ramsay Jones 2018-09-20 5:53 ` Martin Ågren 2018-09-20 14:26 ` Junio C Hamano 2018-09-20 16:03 ` Ramsay Jones 2018-09-20 18:49 ` Junio C Hamano
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).