Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
Jeff King writes: > Unfortunately I think that's hard to do in pure make. But we're already > relying on $(shell) here, so we could just move the logic there. > > Something like the patch below, perhaps. It should do the right thing on > clang and gcc, and I added in an extra clang-only warning I've found > useful. Otherwise the list of flags comes from your patch. I see this discussion is going in the right direction, and like the approach taken by this "how about this" patch. Thanks.
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
On Sat, Mar 17, 2018 at 12:08:32PM -0400, Jeff King wrote: > +case "$($CC --version 2>&1)" in > +gcc*) > + print_flags gcc > + ;; > +clang*) > + print_flags clang > + ;; > +*) > + : unknown compiler family > + ;; > +esac By the way, one annoying thing is that running "cc --version" when "cc" is actually "gcc" will print something like: cc (Debian 7.3.0-11) 7.3.0 even though it supports all of the gcc knobs. This means that: make DEVELOPER=1 without further config won't get these knobs, because we (rightly) default CC=cc. Probably this detection could be a bit more clever, like: cc*Free Software Foundation") print_flags gcc or something. I don't have any non-gcc/clang compilers to test with, so I'm not sure what they even print for "--version" (if anything). -Peff
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
On Sat, Mar 17, 2018 at 03:59:23PM +0100, Duy Nguyen wrote: > On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote: > > I interpreted Peff's comment like this: > > > > If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, > > then we could set your additional flags in the makefile. > > > > This way every developer with a new compiler would run these > > flags locally (if DEVELOPER=1 is set). > > Aha. Something like this? I split developer cflags out to a separate > file because I imagine people may start to add gcc7, clang Yeah, gcc 7 is already standard on debian unstable, so... > +GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) > -dumpversion | cut -f 1 -d .') > + > +ifeq ($(GCC_VER),6) ...this probably needs to be "greater than or equal to". Unfortunately I think that's hard to do in pure make. But we're already relying on $(shell) here, so we could just move the logic there. Something like the patch below, perhaps. It should do the right thing on clang and gcc, and I added in an extra clang-only warning I've found useful. Otherwise the list of flags comes from your patch. The "clang4" limit is just what I happened to have on my system to test with. Some of those flags might work for clang3, and some of the gcc6-only flags might work on newer versions of clang. I was puzzled by -Wno-maybe-uninitialized, though. I think that has found bugs for us before (but also false positives, but we usually squelch those in the code). And it's part of -Wall, not -Wextra, so we shouldn't need it as part of this patch, should we? diff --git a/Makefile b/Makefile index a1d8775adb..9dfd152a1e 100644 --- a/Makefile +++ b/Makefile @@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE # CFLAGS and LDFLAGS are for the users to override from the command line. CFLAGS = -g -O2 -Wall -DEVELOPER_CFLAGS = -Werror \ - -Wdeclaration-after-statement \ - -Wno-format-zero-length \ - -Wold-style-definition \ - -Woverflow \ - -Wpointer-arith \ - -Wstrict-prototypes \ - -Wunused \ - -Wvla LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) @@ -1051,7 +1042,7 @@ include config.mak.uname -include config.mak ifdef DEVELOPER -CFLAGS += $(DEVELOPER_CFLAGS) +include config.mak.dev endif comma := , diff --git a/config.mak.dev b/config.mak.dev new file mode 100644 index 00..59aef342c4 --- /dev/null +++ b/config.mak.dev @@ -0,0 +1,28 @@ +CFLAGS += -Werror +CFLAGS += -Wdeclaration-after-statement +CFLAGS += -Wno-format-zero-length +CFLAGS += -Wold-style-definition +CFLAGS += -Woverflow +CFLAGS += -Wpointer-arith +CFLAGS += -Wstrict-prototypes +CFLAGS += -Wunused +CFLAGS += -Wvla + +COMPILER_FEATURES := $(shell ./detect-compiler $(CC)) + +ifneq ($(filter clang4,$(COMPILER_FEATURES)),) +CFLAGS += -Wtautological-constant-out-of-range-compare +endif + +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) +CFLAGS += -Wextra +CFLAGS += -Wmissing-prototypes +CFLAGS += -Wno-empty-body +CFLAGS += -Wno-missing-field-initializers +CFLAGS += -Wno-sign-compare +CFLAGS += -Wno-unused-function +CFLAGS += -Wno-unused-parameter +ifneq ($(filter gcc6,$(COMPILER_FEATURES)),) +CFLAGS += -Wno-maybe-uninitialized +endif +endif diff --git a/detect-compiler b/detect-compiler new file mode 100755 index 00..043367828c --- /dev/null +++ b/detect-compiler @@ -0,0 +1,32 @@ +#!/bin/sh +# +# Probe the compiler for vintage, version, etc. This is used for setting +# optional make knobs under the DEVELOPER knob. + +CC="$*" + +print_flags() { + family=$1 + version=$($CC -dumpversion | cut -f 1 -d .) + + # Print a feature flag not only for the current version, but also + # for any prior versions we encompass. This avoids needing to do + # numeric comparisons in make, which are awkward. + while test "$version" -gt 0 + do + echo $family$version + version=$((version - 1)) + done +} + +case "$($CC --version 2>&1)" in +gcc*) + print_flags gcc + ;; +clang*) + print_flags clang + ;; +*) + : unknown compiler family + ;; +esac -- 2.17.0.rc0.402.ged0b3fd1ee
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote: > >> Why isn't this just turning on DEVELOPER=1 if we know we have a capable > >> compiler? > > > > DEVELOPER=1 is always set even before this patch. It's set and > > exported in lib-travisci.sh. > > I interpreted Peff's comment like this: > > If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, > then we could set your additional flags in the makefile. > > This way every developer with a new compiler would run these > flags locally (if DEVELOPER=1 is set). Actually, I was mostly just confused, and didn't realize that these were going above and beyond what's in DEVELOPER. But that said, now that I understand, I agree completely with your suggestion. :) -Peff
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote: > I interpreted Peff's comment like this: > > If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, > then we could set your additional flags in the makefile. > > This way every developer with a new compiler would run these > flags locally (if DEVELOPER=1 is set). Aha. Something like this? I split developer cflags out to a separate file because I imagine people may start to add gcc7, clang -- 8< -- diff --git a/.travis.yml b/.travis.yml index 5f5ee4f3bd..0b3c50f5e7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,10 +16,13 @@ compiler: addons: apt: +sources: +- ubuntu-toolchain-r-test packages: - language-pack-is - git-svn - apache2 +- gcc-6 matrix: include: diff --git a/Makefile b/Makefile index de4b8f0c02..e8321e44d6 100644 --- a/Makefile +++ b/Makefile @@ -436,15 +436,6 @@ GIT-VERSION-FILE: FORCE # CFLAGS and LDFLAGS are for the users to override from the command line. CFLAGS = -g -O2 -Wall -DEVELOPER_CFLAGS = -Werror \ - -Wdeclaration-after-statement \ - -Wno-format-zero-length \ - -Wold-style-definition \ - -Woverflow \ - -Wpointer-arith \ - -Wstrict-prototypes \ - -Wunused \ - -Wvla LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) @@ -1045,7 +1036,8 @@ include config.mak.uname -include config.mak ifdef DEVELOPER -CFLAGS += $(DEVELOPER_CFLAGS) +include config.mak.dev +CFLAGS += $(DEV_CFLAGS) endif comma := , diff --git a/config.mak.dev b/config.mak.dev index e69de29bb2..644d0d581f 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -0,0 +1,23 @@ +DEV_CFLAGS = +DEV_CFLAGS += -Werror +DEV_CFLAGS += -Wdeclaration-after-statement +DEV_CFLAGS += -Wno-format-zero-length +DEV_CFLAGS += -Wold-style-definition +DEV_CFLAGS += -Woverflow +DEV_CFLAGS += -Wpointer-arith +DEV_CFLAGS += -Wstrict-prototypes +DEV_CFLAGS += -Wunused +DEV_CFLAGS += -Wvla + +GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) -dumpversion | cut -f 1 -d .') + +ifeq ($(GCC_VER),6) +DEV_CFLAGS += -Wextra +DEV_CFLAGS += -Wmissing-prototypes +DEV_CFLAGS += -Wno-empty-body +DEV_CFLAGS += -Wno-maybe-uninitialized +DEV_CFLAGS += -Wno-missing-field-initializers +DEV_CFLAGS += -Wno-sign-compare +DEV_CFLAGS += -Wno-unused-function +DEV_CFLAGS += -Wno-unused-parameter +endif -- 8< --
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
> On 17 Mar 2018, at 09:01, Duy Nguyen wrote: > > On Fri, Mar 16, 2018 at 10:22 PM, Jeff King wrote: >>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh >>> index 3735ce413f..f6f346c468 100755 >>> --- a/ci/run-build-and-tests.sh >>> +++ b/ci/run-build-and-tests.sh >>> @@ -7,6 +7,22 @@ >>> >>> ln -s "$cache_dir/.prove" t/.prove >>> >>> +if [ "$jobname" = linux-gcc ]; then >>> + gcc-6 --version >>> + cat >config.mak <<-EOF >>> + CC=gcc-6 >>> + CFLAGS = -g -O2 -Wall >>> + CFLAGS += -Wextra >>> + CFLAGS += -Wmissing-prototypes >>> + CFLAGS += -Wno-empty-body >>> + CFLAGS += -Wno-maybe-uninitialized >>> + CFLAGS += -Wno-missing-field-initializers >>> + CFLAGS += -Wno-sign-compare >>> + CFLAGS += -Wno-unused-function >>> + CFLAGS += -Wno-unused-parameter >>> + EOF >>> +fi >> >> Why isn't this just turning on DEVELOPER=1 if we know we have a capable >> compiler? > > DEVELOPER=1 is always set even before this patch. It's set and > exported in lib-travisci.sh. I interpreted Peff's comment like this: If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, then we could set your additional flags in the makefile. This way every developer with a new compiler would run these flags locally (if DEVELOPER=1 is set). - Lars
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
On Fri, Mar 16, 2018 at 10:22 PM, Jeff King wrote: >> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh >> index 3735ce413f..f6f346c468 100755 >> --- a/ci/run-build-and-tests.sh >> +++ b/ci/run-build-and-tests.sh >> @@ -7,6 +7,22 @@ >> >> ln -s "$cache_dir/.prove" t/.prove >> >> +if [ "$jobname" = linux-gcc ]; then >> + gcc-6 --version >> + cat >config.mak <<-EOF >> + CC=gcc-6 >> + CFLAGS = -g -O2 -Wall >> + CFLAGS += -Wextra >> + CFLAGS += -Wmissing-prototypes >> + CFLAGS += -Wno-empty-body >> + CFLAGS += -Wno-maybe-uninitialized >> + CFLAGS += -Wno-missing-field-initializers >> + CFLAGS += -Wno-sign-compare >> + CFLAGS += -Wno-unused-function >> + CFLAGS += -Wno-unused-parameter >> + EOF >> +fi > > Why isn't this just turning on DEVELOPER=1 if we know we have a capable > compiler? DEVELOPER=1 is always set even before this patch. It's set and exported in lib-travisci.sh. -- Duy
Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job
On Fri, Mar 16, 2018 at 08:33:55PM +0100, Nguyễn Thái Ngọc Duy wrote: > We have DEVELOPER config to enable more warnings, but since we can't set > a fixed gcc version to all devs, that has to be a bit more conservative. > On travis, we know almost exact version to be used (bumped to 6.x for > more warnings), we could be more aggressive. Certainly it makes sense to dial up any static checks we can for the Travis environment, but... > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 3735ce413f..f6f346c468 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -7,6 +7,22 @@ > > ln -s "$cache_dir/.prove" t/.prove > > +if [ "$jobname" = linux-gcc ]; then > + gcc-6 --version > + cat >config.mak <<-EOF > + CC=gcc-6 > + CFLAGS = -g -O2 -Wall > + CFLAGS += -Wextra > + CFLAGS += -Wmissing-prototypes > + CFLAGS += -Wno-empty-body > + CFLAGS += -Wno-maybe-uninitialized > + CFLAGS += -Wno-missing-field-initializers > + CFLAGS += -Wno-sign-compare > + CFLAGS += -Wno-unused-function > + CFLAGS += -Wno-unused-parameter > + EOF > +fi Why isn't this just turning on DEVELOPER=1 if we know we have a capable compiler? -Peff