| Message ID | d24df21a-7ab2-84f6-8b18-83fd9c8c2b30@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] Makefile: add a hdr-check target | expand |
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
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 >
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
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.
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
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.
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
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(+)