svn commit: r483758 - head/devel/libpru

[Mathieu Arnold]

On Fri, Nov 02, 2018 at 10:56:00AM +0100, Jan Beich wrote:
> Mathieu Arnold <mat at FreeBSD.org> writes:
> 
> > On Fri, Nov 02, 2018 at 01:35:52AM +0000, Mark Linimon wrote:
> >
> >> Author: linimon
> >> Date: Fri Nov  2 01:35:51 2018
> >> New Revision: 483758
> >> URL: https://svnweb.freebsd.org/changeset/ports/483758
> >> 
> >> Log:
> >>   Fix build with GCC-based architectures.
> >>   
> >>   PR:		232851
> >>   Submitted by:	Piotr Kubaj
> >> 
> >> Modified:
> >>   head/devel/libpru/Makefile
> >> 
> >> Modified: head/devel/libpru/Makefile
> >> ==============================================================================
> >> --- head/devel/libpru/Makefile	Fri Nov  2 01:34:15 2018	(r483757)
> >> +++ head/devel/libpru/Makefile	Fri Nov  2 01:35:51 2018	(r483758)
> >> @@ -11,13 +11,15 @@ COMMENT=	Library to interface with PRUs
> >>  LICENSE=	BSD2CLAUSE
> >>  
> >>  IGNORE_DragonFly=	only supported on FreeBSD
> >> -BROKEN_mips=	Does not build: unrecognized command line option -Weverything
> >> -BROKEN_mips64=	Does not build: unrecognized command line option -Weverything
> >> -BROKEN_powerpc64=	Does not build: unrecognized command line option -Weverything
> >> -BROKEN_sparc64=	Does not build: unrecognized command line option -Weverything
> >> -
> >>  USES=		cmake
> >>  
> >>  WRKSRC=		${WRKDIR}/rpaulo-libpru-5a74157b82b8
> >>  
> >> -.include <bsd.port.mk>
> >> +.include <bsd.port.pre.mk>
> >> +
> >> +post-patch:
> >> +.if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64
> >> +	${REINPLACE_CMD} -e 's/ -Weverything//'  ${WRKSRC}/CMakeLists.txt
> >> +.endif
> >> +
> >> +.include <bsd.port.post.mk>
> >
> > Could you try and put the whole target definition inside the if so that
> > it does not needlessly create an empty target, like this:
> >
> >
> > .if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64
> > post-patch:
> > 	${REINPLACE_CMD} -e 's/ -Weverything//'  ${WRKSRC}/CMakeLists.txt
> > .endif
> 
> I disagree. If a target is defined conditionally then adding unconditional
> one with the same name becomes error-prone in that the conflict would only
> occur if the condition is true. For example:
> 
>   post-patch:
>           @${REINPLACE_CMD} -e 's/ -Werror//' ${WRKSRC}/CMakeLists.txt
> 
>   .include <bsd.port.pre.mk>
> 
>   post-patch:
>   .if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64
>           ${REINPLACE_CMD} -e 's/ -Weverything//'  ${WRKSRC}/CMakeLists.txt
>   .endif
> 
>   .include <bsd.port.post.mk>
> 
> where the conflict is hidden on Tier1 archs while Tier2 ones are undertested.

This example is obviously very badly written.  It should either be:

post-patch:
        @${REINPLACE_CMD} -e 's/ -Werror//' ${WRKSRC}/CMakeLists.txt
.if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64
        ${REINPLACE_CMD} -e 's/ -Weverything//'  ${WRKSRC}/CMakeLists.txt
.endif

or:

post-patch:
.if ${ARCH:Mmips*} || ${ARCH:Mpowerpc*} || ${ARCH} == sparc64
        ${REINPLACE_CMD} -e 's/ -Weverything//'  ${WRKSRC}/CMakeLists.txt
.else
        @${REINPLACE_CMD} -e 's/ -Werror//' ${WRKSRC}/CMakeLists.txt
.endif

depending on what you want to do.

But to the point, it has nothing to do with what I was saying.

If the target you add is only required when the conditions are met, then
only define it inside the .if.

It makes everything a wee bit faster by not having to compute order for
one more target.

-- 
Mathieu Arnold
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 963 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-ports-all/attachments/20181102/668b887e/attachment.sig>