Date: Tue, 27 Dec 2011 19:27:08 +0000 From: Alexander Best <arundel@freebsd.org> To: Luigi Rizzo <rizzo@iet.unipi.it> Cc: freebsd-current@freebsd.org, freebsd-arch@freebsd.org Subject: Re: [rfc] removing/conditionalising WERROR= in Makefiles Message-ID: <20111227192708.GA58521@freebsd.org> In-Reply-To: <20111227120409.GA64631@onelab2.iet.unipi.it> References: <20111226101040.GA6361@freebsd.org> <20111227010449.GA6244@twoflower.paeps.cx> <20111227112743.GA154@freebsd.org> <20111227120409.GA64631@onelab2.iet.unipi.it>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue Dec 27 11, Luigi Rizzo wrote:
> On Tue, Dec 27, 2011 at 11:27:43AM +0000, Alexander Best wrote:
> > On Tue Dec 27 11, Philip Paeps wrote:
> > > On 2011-12-26 10:10:40 (+0000), Alexander Best <arundel@freebsd.org> wrote:
> > > > i grep'ed through src/sys and found several places where WERROR= was set in
> > > > order to get rid of the default -Werror setting. i tried to remove those
> > > > WERROR= overrides from any Makefile, where doing so did not break tinderbox.
> > > >
> > > > in those cases, where it couldn't be completely removed, i added conditions to
> > > > only set WERROR= for the particular achitecture or compiler, where tinderbox
> > > > did not suceed without the WERROR=.
> > >
> > > Wouldn't it be better to set WARNS=x rather than WERROR=? WERROR= says "this
> > > code has bugs, it breaks tinderbox" whereas WARNS=x says "this code has the
> > > following kind of bugs which break tinderbox".
> > >
> > > Possibly wrapped in an architecture-test where appropriate.
> >
> > well there are a few issues here:
> >
> > 1) Jan Beich informed me via a private email that enclosing WERROR in arch
> > specific conditions is a bad idea. if the code gets ported to a new
> > architecture WERROR doesn't get set and so for every new architecture one
> > has to evaluate again, whether WERROR needs to be set or not.
> > so i'm inclined to agree with dim@ that we should not add architecture
> > specific conditions -- however i think adding compiler specific conditions
> > is a good idea.
> >
> > 2) the problem with settings WARNS= or specific -Wno-X or -Wno-error=X is that
> > expecially GCC doesn't have specific -WX flags for certain warnings. some
> > warnings are implied by -Wall and cannot be turned off seperately. so in
> > order to get rid of these warnings (which are being handled as errors), we
> > would need to disable -Wall. and i think setting WERROR= in order to handle
> > all warnings for specific code as warnings rather than as errors is the
> > better solution.
> >
> > i've reworked the patch to only remove WERROR=, where it is not needed anymore
> > for any supported arch, or where it can be wrapped in a compiler condition.
>
> It seems to me that the removal of unnecessary WERROR= needed no
> discussion since day one so why don't you go ahead and commit it.
anybody is free to commit this part, since i don't own a commit bit. ;)
>
> I don't understand the comment on issue #1 above. There is a minuscule
> (six, before your patch ?)
> number of Makefiles with WERROR= . If you make the assignment
> architecture-specific, the worst it can happen is that the variable
> is not cleared, and if the build breaks, all you need is to
> add the extra architecture in these few places.
good point. basically the question with WERROR is: should it be a big hammer
to disable turning warnings into errors for all archs or do we want to set
WERROR in a more specific manor, where it's absolutely necessary.
cheers.
alex
>
> cheers
> luigi
>
> > cheers.
> > alex
> >
> > >
> > > - Philip
> > >
> > > --
> > > Philip Paeps
> > > Senior Reality Engineer
> > > Ministry of Information
>
> > Index: sys/modules/xfs/Makefile
> > ===================================================================
> > --- sys/modules/xfs/Makefile (revision 228911)
> > +++ sys/modules/xfs/Makefile (working copy)
> > @@ -6,8 +6,6 @@
> >
> > KMOD= xfs
> >
> > -WERROR=
> > -
> > SRCS = vnode_if.h \
> > xfs_alloc.c \
> > xfs_alloc_btree.c \
> > Index: sys/modules/sound/driver/maestro/Makefile
> > ===================================================================
> > --- sys/modules/sound/driver/maestro/Makefile (revision 228911)
> > +++ sys/modules/sound/driver/maestro/Makefile (working copy)
> > @@ -5,6 +5,5 @@
> > KMOD= snd_maestro
> > SRCS= device_if.h bus_if.h pci_if.h
> > SRCS+= maestro.c
> > -WERROR=
> >
> > .include <bsd.kmod.mk>
> > Index: sys/modules/aic7xxx/ahd/Makefile
> > ===================================================================
> > --- sys/modules/aic7xxx/ahd/Makefile (revision 228911)
> > +++ sys/modules/aic7xxx/ahd/Makefile (working copy)
> > @@ -4,7 +4,6 @@
> > .PATH: ${.CURDIR}/../../../dev/aic7xxx
> > KMOD= ahd
> >
> > -WERROR=
> > GENSRCS= aic79xx_seq.h aic79xx_reg.h
> > REG_PRINT_OPT=
> > AHD_REG_PRETTY_PRINT=1
> > Index: sys/modules/agp/Makefile
> > ===================================================================
> > --- sys/modules/agp/Makefile (revision 228911)
> > +++ sys/modules/agp/Makefile (working copy)
> > @@ -20,7 +20,6 @@
> > SRCS+= device_if.h bus_if.h agp_if.h pci_if.h
> > SRCS+= opt_agp.h opt_bus.h
> > MFILES= kern/device_if.m kern/bus_if.m dev/agp/agp_if.m dev/pci/pci_if.m
> > -WERROR=
> >
> > EXPORT_SYMS= agp_find_device \
> > agp_state \
> > Index: sys/modules/bios/smapi/Makefile
> > ===================================================================
> > --- sys/modules/bios/smapi/Makefile (revision 228911)
> > +++ sys/modules/bios/smapi/Makefile (working copy)
> > @@ -6,7 +6,6 @@
> > KMOD= smapi
> > SRCS= smapi.c smapi_bios.S \
> > bus_if.h device_if.h
> > -WERROR=
> > .if ${CC:T:Mclang} == "clang"
> > # XXX: clang integrated-as doesn't grok 16-bit assembly yet
> > CFLAGS+= ${.IMPSRC:T:Msmapi_bios.S:C/^.+$/-no-integrated-as/}
> > Index: sys/modules/nve/Makefile
> > ===================================================================
> > --- sys/modules/nve/Makefile (revision 228911)
> > +++ sys/modules/nve/Makefile (working copy)
> > @@ -7,7 +7,9 @@
> > device_if.h bus_if.h pci_if.h miibus_if.h \
> > os+%DIKED-nve.h
> > OBJS+= nvenetlib.o
> > +.if ${CC:T:Mclang} == "clang"
> > WERROR=
> > +.endif
> >
> > CLEANFILES+= nvenetlib.o os+%DIKED-nve.h
> > nvenetlib.o: ${.CURDIR}/../../contrib/dev/nve/${MACHINE}/${.TARGET}.bz2.uu
>
> > _______________________________________________
> > freebsd-arch@freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111227192708.GA58521>