Re: [rfc] removing/conditionalising WERROR= in Makefiles

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_at_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_at_ 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.

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.

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)
> _at__at_ -6,8 +6,6 _at__at_
>  
>  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)
> _at__at_ -5,6 +5,5 _at__at_
>  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)
> _at__at_ -4,7 +4,6 _at__at_
>  .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)
> _at__at_ -20,7 +20,6 _at__at_
>  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)
> _at__at_ -6,7 +6,6 _at__at_
>  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)
> _at__at_ -7,7 +7,9 _at__at_
>  	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_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe_at_freebsd.org"