Re: [RFC PATCH 1/2] Makefile: add check-headers target

2014-09-06 Thread René Scharfe 

Am 06.09.2014 um 21:20 schrieb David Aguilar:

This allows us to ensure that each header can be included
individually without needing to include other headers first.


Sounds like a good objective.


Signed-off-by: David Aguilar dav...@gmail.com
---
This patch demonstrates how to verify PATCH 2/2.

  Makefile |  6 ++
  check-headers.sh | 26 ++
  2 files changed, 32 insertions(+)
  create mode 100755 check-headers.sh

diff --git a/Makefile b/Makefile
index 30cc622..bc54024 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)
+
+
  ### 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..bf85c41
--- /dev/null
+++ b/check-headers.sh
@@ -0,0 +1,26 @@
+#!/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
+}
+
+git ls-files *.h |


This checks all .h files in the top directory.  Would it be better to 
check all files in LIB_H instead?  Or even all .h files in the tree 
(using git ls-files '*.h')?  The latter might be difficult because 
some of the files in compat/ #include system-specific headers.



+while read header
+do
+   echo HEADER $header 
+   $@ -Wno-unused -x c -c -o $header.bin - $header 
+   rm $header.bin ||
+   maybe_exit $?
+done
+
+exit $exit_code



--
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 1/2] Makefile: add check-headers target

2014-09-06 Thread David Aguilar 
On Sat, Sep 06, 2014 at 11:20:32PM +0200, René Scharfe wrote:
 Am 06.09.2014 um 21:20 schrieb David Aguilar:
 This allows us to ensure that each header can be included
 individually without needing to include other headers first.
 
 Sounds like a good objective.
 
 Signed-off-by: David Aguilar dav...@gmail.com
 ---
 This patch demonstrates how to verify PATCH 2/2.
 
   Makefile |  6 ++
   check-headers.sh | 26 ++
   2 files changed, 32 insertions(+)
   create mode 100755 check-headers.sh
 
 diff --git a/Makefile b/Makefile
 index 30cc622..bc54024 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)
 +
 +
   ### 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..bf85c41
 --- /dev/null
 +++ b/check-headers.sh
 @@ -0,0 +1,26 @@
 +#!/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
 +}
 +
 +git ls-files *.h |
 
 This checks all .h files in the top directory.  Would it be better
 to check all files in LIB_H instead?  Or even all .h files in the
 tree (using git ls-files '*.h')?  The latter might be difficult
 because some of the files in compat/ #include system-specific
 headers.

Ah, I hadn't thought of using LIB_H; that might be the most
practical solution.

Changing it to quoted '*.h' finds compat/ files which seemed
like too much trouble.  OTOH blindly using all *.h files (i.e.
not using git ls-files) picks up the generated common-cmds.h
which would require changing generate-cmdlist.h, and that seemed
like it might be out of scope for this patch.

I'd certainly be interested in improving this part, so I'll see if
we can reformulate this patch to use LIB_H.

I'll also fixup the unneeded strbuf.h addition in the reroll.

 
 +while read header
 +do
 +echo HEADER $header 
 +$@ -Wno-unused -x c -c -o $header.bin - $header 
 +rm $header.bin ||
 +maybe_exit $?
 +done
 +
 +exit $exit_code
 
 

-- 
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 1/2] Makefile: add check-headers target

2014-09-06 Thread Jeff King 
On Sat, Sep 06, 2014 at 03:57:39PM -0700, David Aguilar wrote:

  This checks all .h files in the top directory.  Would it be better
  to check all files in LIB_H instead?  Or even all .h files in the
  tree (using git ls-files '*.h')?  The latter might be difficult
  because some of the files in compat/ #include system-specific
  headers.
 
 Ah, I hadn't thought of using LIB_H; that might be the most
 practical solution.

Maybe not; see d85b0dff7297fb43a57a0c1e697417bb7723247c, which is in
'next'.

-Peff
--
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 1/2] Makefile: add check-headers target

2014-09-06 Thread David Aguilar 
On Sat, Sep 06, 2014 at 07:58:09PM -0400, Jeff King wrote:
 On Sat, Sep 06, 2014 at 03:57:39PM -0700, David Aguilar wrote:
 
   This checks all .h files in the top directory.  Would it be better
   to check all files in LIB_H instead?  Or even all .h files in the
   tree (using git ls-files '*.h')?  The latter might be difficult
   because some of the files in compat/ #include system-specific
   headers.
  
  Ah, I hadn't thought of using LIB_H; that might be the most
  practical solution.
 
 Maybe not; see d85b0dff7297fb43a57a0c1e697417bb7723247c, which is in
 'next'.

Thanks for the heads-up. I'll send v2 now which will basically use
the original approach plus a few extra paths so that we catch xdiff/,
vcs-svn, and ewah/.

Widening any more (e.g. it would be nice to say compat/*.h too) breaks
it due to platform-specific includes so it seems good enough for catching
the main cross-platform headers.
-- 
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