Re: [PATCH 1/9] Makefile: add a hdr-check target
Ramsay Jones 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.
Re: [PATCH 1/9] Makefile: add a hdr-check target
On 20/09/18 15:26, Junio C Hamano wrote: > Ramsay Jones 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
Re: [PATCH 1/9] Makefile: add a hdr-check target
Ramsay Jones 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.
Re: [PATCH 1/9] Makefile: add a hdr-check target
On Wed, 19 Sep 2018 at 22:15, Ramsay Jones wrote: > On 19/09/18 18:49, Martin Ågren wrote: > > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones > > 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
Re: [PATCH 1/9] Makefile: add a hdr-check target
On 19/09/18 18:49, Martin Ågren wrote: > Hi Ramsay, > > On Wed, 19 Sep 2018 at 02:07, Ramsay Jones > 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 >
Re: [PATCH 1/9] Makefile: add a hdr-check target
Hi Ramsay, On Wed, 19 Sep 2018 at 02:07, Ramsay Jones 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