Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
Junio C Hamano writes: > David Aguilar writes: > >> On Mon, Sep 08, 2014 at 12:57:46PM -0700, Junio C Hamano wrote: >>> Matthieu Moy writes: >>> ... >>> > for header in .h ewah/*.h vcs-svn/*.h xdiff/*.h >>> > do >>> > ... >>> > done >>> >>> Yes, that would be even better. Then you wouldn't even have to >>> worry about $IFS dance. >> >> The original motivation was to avoid picking up the generated >> common-cmds.h header file. > > for header > do > case "$header" in $exceptions) continue ;; esac > ... > done > > with comments describing why these exceptions are made would be a > better way to go in such a case. +1 from me. It would allow developers to use the rule without "git add"-ing new .h files, and the comment would document why the exceptions are there (which missed in the original patch IMHO). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
David Aguilar writes: > On Mon, Sep 08, 2014 at 12:57:46PM -0700, Junio C Hamano wrote: >> Matthieu Moy writes: >> ... >> > for header in .h ewah/*.h vcs-svn/*.h xdiff/*.h >> > do >> >... >> > done >> >> Yes, that would be even better. Then you wouldn't even have to >> worry about $IFS dance. > > The original motivation was to avoid picking up the generated > common-cmds.h header file. for header do case "$header" in $exceptions) continue ;; esac ... done with comments describing why these exceptions are made would be a better way to go in such a case. > It was the N_() function that was messing it up. > > Would it make sense to split out a separate patch that makes common-cmds.h > check-headers clean? Depends on why "gcc -c $header"-cleanliness needs to be strictly enforced, I think. "common-cmds.h" is merely a way to allow us maintain a part of its single includer help.c mechanically maintained, and if anybody else includes it, it is an error, even if that includer does so after including "gettext.h". Some effort would be required to butcher "common-cmds.h' to make it include "gettext.h" but that amount of effort can be better spent to add a check to make sure nobody else includes it, I would have to say. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
On Mon, Sep 08, 2014 at 12:57:46PM -0700, Junio C Hamano wrote: > Matthieu Moy writes: > > > Junio C Hamano writes: > > > >> David Aguilar writes: > >> > >>> +IFS=' > >>> +' > >>> +git ls-files *.h ewah/*.h vcs-svn/*.h xdiff/*.h | > >> > >> Hmm. This is only for true developers (not one who merely compiles > >> after expanding a tarball), so "git ls-files" may probably be OK. > >> > >> But "/bin/ls" would be equally fine for that, no? > > > > Actually, since this is "| while read header", I have to wonder why this > > is not written as > > > > for header in .h ewah/*.h vcs-svn/*.h xdiff/*.h > > do > > ... > > done > > Yes, that would be even better. Then you wouldn't even have to > worry about $IFS dance. The original motivation was to avoid picking up the generated common-cmds.h header file. It was the N_() function that was messing it up. Would it make sense to split out a separate patch that makes common-cmds.h check-headers clean? -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
Matthieu Moy writes: > Junio C Hamano writes: > >> David Aguilar writes: >> >>> +IFS=' >>> +' >>> +git ls-files *.h ewah/*.h vcs-svn/*.h xdiff/*.h | >> >> Hmm. This is only for true developers (not one who merely compiles >> after expanding a tarball), so "git ls-files" may probably be OK. >> >> But "/bin/ls" would be equally fine for that, no? > > Actually, since this is "| while read header", I have to wonder why this > is not written as > > for header in .h ewah/*.h vcs-svn/*.h xdiff/*.h > do > ... > done Yes, that would be even better. Then you wouldn't even have to worry about $IFS dance. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
Junio C Hamano writes: > David Aguilar writes: > >> +IFS=' >> +' >> +git ls-files *.h ewah/*.h vcs-svn/*.h xdiff/*.h | > > Hmm. This is only for true developers (not one who merely compiles > after expanding a tarball), so "git ls-files" may probably be OK. > > But "/bin/ls" would be equally fine for that, no? Actually, since this is "| while read header", I have to wonder why this is not written as for header in .h ewah/*.h vcs-svn/*.h xdiff/*.h do ... done -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
David Aguilar writes: > +IFS=' > +' > +git ls-files *.h ewah/*.h vcs-svn/*.h xdiff/*.h | Hmm. This is only for true developers (not one who merely compiles after expanding a tarball), so "git ls-files" may probably be OK. But "/bin/ls" would be equally fine for that, no? After all, you are letting your shell to expand the wildcard, and a developer would want to make sure his new header is kosher before running "git add" on it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/2] Makefile: add check-headers target
Hi, David Aguilar wrote: > --- /dev/null > +++ b/check-headers.sh > @@ -0,0 +1,29 @@ [...] > + "$@" -Wno-unused -I"$subdir" -c -o "$header".check -x c - <"$header" && All .c files in git are supposed to start by #include-ing git-compat-util.h, cache.h, or builtin.h to set the appropriate feature test macros and include system headers. Headers rely on that for basic types like int32_t. They don't need to include git-compat-util.h because the .c file that included them would have already, and .h files #include-ed by git-compat-util.h especially *shouldn't* #include the compat header, so how about something like the following for squashing in? A side-thought: as long as we're building pre-compiled headers, could we use them in the build? Thanks, Jonathan diff --git a/check-headers.sh b/check-headers.sh index bf85c41..08ca136 100755 --- a/check-headers.sh +++ b/check-headers.sh @@ -18,7 +18,7 @@ git ls-files *.h | while read header do echo "HEADER $header" && - "$@" -Wno-unused -x c -c -o "$header".bin - <"$header" && + "$@" -Wno-unused -x c -include git-compat-util.h -c -o "$header".bin - <"$header" && rm "$header".bin || maybe_exit $? done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH v2 1/2] Makefile: add check-headers target
This allows us to ensure that each header can be included
individually without needing to include other headers first.
Signed-off-by: David Aguilar
---
Changes since v1:
We now include xdiff, ewah, and vcs-svn headers.
Makefile | 6 ++
check-headers.sh | 29 +
2 files changed, 35 insertions(+)
create mode 100755 check-headers.sh
diff --git a/Makefile b/Makefile
index 30cc622..39ecc70 100644
--- a/Makefile
+++ b/Makefile
@@ -2591,6 +2591,12 @@ check-docs::
check-builtins::
./check-builtins.sh
+### Make sure headers include their dependencies
+#
+check-headers::
+ ./check-headers.sh $(CC) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS)
+
+
### Test suite coverage testing
#
.PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report
diff --git a/check-headers.sh b/check-headers.sh
new file mode 100755
index 000..2722dd2
--- /dev/null
+++ b/check-headers.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+exit_code=0
+
+maybe_exit () {
+ status="$1"
+ if test "$status" != 0
+ then
+ exit_code="$status"
+ if test -n "$CHECK_HEADERS_STOP"
+ then
+ exit "$status"
+ fi
+ fi
+}
+
+IFS='
+'
+git ls-files *.h ewah/*.h vcs-svn/*.h xdiff/*.h |
+while read header
+do
+ subdir=$(dirname "$header") &&
+ echo "HEADER $header" &&
+ "$@" -Wno-unused -I"$subdir" -c -o "$header".check -x c - <"$header" &&
+ rm "$header".check ||
+ maybe_exit $?
+done
+
+exit $exit_code
--
2.1.0.62.g3e88d26
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
