From: Lars Schneider <redacted>
The global Travis-CI environment variable CFLAGS did not override the
CFLAGS variable in the makefile. Pass CFLAGS as make variable to
override it properly.
In addition to that, add '-Wdeclaration-after-statement' to make a
Travis-CI build fail (because of '-Werror') if the code does not adhere
to the Git coding style.
Inspired-by: Jeff King [off-list ref]
Signed-off-by: Lars Schneider <redacted>
---
I made this patch because Peff pointed out to me that "git style doesn't
allow declaration-after-statement" [1]. I wonder if it would make sense
to add this check even in the makefile [2]? I am no make expert, but I
also wonder why we don't use the override directive [3] for the CFLAGS?
AFAIK this would allow a make invocation like this:
make target CFLAGS+=-Wdeclaration-after-statement
Thanks,
Lars
[1] http://www.spinics.net/lists/git/msg267273.html
[2] https://github.com/git/git/blob/ff4ea6004fb48146330d663d64a71e7774f059f9/Makefile#L377
[3] https://www.gnu.org/software/make/manual/make.html#Override-Directive
.travis.yml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index c3bf9c6..29abff4 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -20,7 +20,7 @@ env:
- DEFAULT_TEST_TARGET=prove
- GIT_PROVE_OPTS="--timer --jobs 3"
- GIT_TEST_OPTS="--verbose --tee"
- - CFLAGS="-g -O2 -Wall -Werror"
+ - CFLAGS="-g -O2 -Wall -Werror -Wdeclaration-after-statement"
- GIT_TEST_CLONE_2GB=YesPlease
# t9810 occasionally fails on Travis CI OS X
# t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X@@ -68,7 +68,7 @@ before_install:
echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
git-lfs version;
-before_script: make --jobs=2
+before_script: make CFLAGS="$CFLAGS" --jobs=2
script: make --quiet test
--2.5.1
On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschneider@gmail.com wrote:
From: Lars Schneider <redacted>
The global Travis-CI environment variable CFLAGS did not override the
CFLAGS variable in the makefile. Pass CFLAGS as make variable to
override it properly.
Makes sense.
In addition to that, add '-Wdeclaration-after-statement' to make a
Travis-CI build fail (because of '-Werror') if the code does not adhere
to the Git coding style.
I think this is a good step, but is probably the tip of the iceberg. I
typically use:
-Wall -Werror -Wdeclaration-after-statement
-Wpointer-arith -Wstrict-prototypes -Wvla
-Wold-style-declaration -Wold-style-definition
Junio does his integration testing with a similar set, which I think you
can find in one of the scripts in his "todo" branch.
I made this patch because Peff pointed out to me that "git style doesn't
allow declaration-after-statement" [1]. I wonder if it would make sense
to add this check even in the makefile [2]?
I think we keep the default CFLAGS to a pretty tame set, so that we
build out of the box on a large number of compilers. I know I have run
into problems with -Wold-style-* on clang (though perhaps that is no
longer the case, as I haven't tried recently), let alone truly antique
compilers.
I have, however, wondered if it would make sense to codify this
knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1
roughly mean "you are a git dev, you have a reasonably modern compiler,
and want to be as careful as possible before sending your patches".
I am no make expert, but I
also wonder why we don't use the override directive [3] for the CFLAGS?
AFAIK this would allow a make invocation like this:
make target CFLAGS+=-Wdeclaration-after-statement
I think it is rather the opposite. If we used:
override CFLAGS+= ...
in the Makefile, then if a user did:
make CFLAGS=...
we would add to their CFLAGS (whereas without the override, our
appending is ignored). Our Makefile solves that in a different way,
though. We pass $(ALL_CFLAGS) to the compiler, and that in turn is made
up of $(CFLAGS) and $(BASIC_CFLAGS). We assume the user tweaks the
former, and we set the latter based on Makefile knobs (e.g., NO_CURL,
etc).
-Peff
On 08 Feb 2016, at 13:25, Jeff King [off-list ref] wrote:
On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
The global Travis-CI environment variable CFLAGS did not override the
CFLAGS variable in the makefile. Pass CFLAGS as make variable to
override it properly.
Makes sense.
quoted
In addition to that, add '-Wdeclaration-after-statement' to make a
Travis-CI build fail (because of '-Werror') if the code does not adhere
to the Git coding style.
I think this is a good step, but is probably the tip of the iceberg. I
typically use:
-Wall -Werror -Wdeclaration-after-statement
-Wpointer-arith -Wstrict-prototypes -Wvla
-Wold-style-declaration -Wold-style-definition
Junio does his integration testing with a similar set, which I think you
can find in one of the scripts in his "todo" branch.
I collected the warnings from Junio's Make [1] script and merged them with
yours. This is the resulting warning list for clang and gcc:
-Wdeclaration-after-statement -Wno-format-zero-length -Wold-style-definition
-Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla
Gcc processes one extra warning that clang doesn't like:
-Wold-style-declaration
The build runs clean with all these checks enabled.
[1] https://github.com/git/git/blob/todo/Make
quoted
I made this patch because Peff pointed out to me that "git style doesn't
allow declaration-after-statement" [1]. I wonder if it would make sense
to add this check even in the makefile [2]?
I think we keep the default CFLAGS to a pretty tame set, so that we
build out of the box on a large number of compilers. I know I have run
into problems with -Wold-style-* on clang (though perhaps that is no
longer the case, as I haven't tried recently), let alone truly antique
compilers.
I have, however, wondered if it would make sense to codify this
knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1
roughly mean "you are a git dev, you have a reasonably modern compiler,
and want to be as careful as possible before sending your patches".
That make sense. However, in "development mode" I don't like Werror.
How about two knobs? One called DEVELOPER which enables all the warnings
above and one called CONTINUOUS_INTEGRATION that enables Werror
in addition?
quoted
I am no make expert, but I
also wonder why we don't use the override directive [3] for the CFLAGS?
AFAIK this would allow a make invocation like this:
make target CFLAGS+=-Wdeclaration-after-statement
I think it is rather the opposite. If we used:
override CFLAGS+= ...
in the Makefile, then if a user did:
make CFLAGS=...
we would add to their CFLAGS (whereas without the override, our
appending is ignored). Our Makefile solves that in a different way,
though. We pass $(ALL_CFLAGS) to the compiler, and that in turn is made
up of $(CFLAGS) and $(BASIC_CFLAGS). We assume the user tweaks the
former, and we set the latter based on Makefile knobs (e.g., NO_CURL,
etc).
I see. Thanks for the explanation.
Regarding CI checks:
Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]
where I found checkpatch.pl [3]. Would it make sense to check all commits
that are not in next/master/maint with this script on Travis-CI?
Stefan Beller recently mentioned "Adhere to the common coding style of
Git" [4] where he removed explicit NULL checks. This kind of stuff could be
checked automatically with checkpatch.pl as far as I can see.
[2] http://www.spinics.net/lists/git/msg267445.html
[3] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
[4] http://permalink.gmane.org/gmane.comp.version-control.git/280842
Thanks,
Lars
On Tue, Feb 09, 2016 at 11:06:17AM +0100, Lars Schneider wrote:
I collected the warnings from Junio's Make [1] script and merged them with
yours. This is the resulting warning list for clang and gcc:
-Wdeclaration-after-statement -Wno-format-zero-length -Wold-style-definition
-Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla
Sounds right.
quoted
I have, however, wondered if it would make sense to codify this
knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1
roughly mean "you are a git dev, you have a reasonably modern compiler,
and want to be as careful as possible before sending your patches".
That make sense. However, in "development mode" I don't like Werror.
How about two knobs? One called DEVELOPER which enables all the warnings
above and one called CONTINUOUS_INTEGRATION that enables Werror
in addition?
I'm curious why you don't want -Werror. Junio is going to compile the
result of applying your patch with it, so it makes sense to me to catch
problems as early as possible.
And while there are certainly false positives from gcc's warnings, we
work to squelch them whether -Werror is in effect or not. So IMHO,
-Werror is mostly about making sure you _see_ the warnings and don't
lose them in a flood of compilation messages.
Regarding CI checks:
Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]
where I found checkpatch.pl [3]. Would it make sense to check all commits
that are not in next/master/maint with this script on Travis-CI?
Stefan Beller recently mentioned "Adhere to the common coding style of
Git" [4] where he removed explicit NULL checks. This kind of stuff could be
checked automatically with checkpatch.pl as far as I can see.
Perhaps. I'm not sure that people actually use checkpatch.pl for git.
Out of curiosity, I tried:
mkdir out
git format-patch -o out v2.6.0..v2.7.0
checkpatch.pl out/*
It's rather noisy, and after skimming, I'd say (subjectively) that only
a small fraction are actual style issues we try to enforce. So it would
certainly need a fair bit of tweaking for regular use, I think.
-Peff