svn commit: r452416 - in head/graphics/opensubdiv: . files

[Steve Wills]

Hi,

On 10/19/2017 13:18, Jan Beich wrote:
> Steve Wills <swills at FreeBSD.org> writes:
> 
>> -COMMENT=	High performance subdivision surface libraries
>> +COMMENT=	OpenSubdiv graphics library
> 
> why the regression? "graphics" is also the default theme for anything
> under graphics/ category.
> 

Yeah, the old comment was better now that you mention it.

>> 2. Do not include the package name or version number of software.
> 
> https://www.freebsd.org/doc/en/books/porters-handbook/makefile-comment.html
> 

Right

>> -DISTVERSIONPREFIX=	v
>> -DISTVERSION=	3_0_5
>> +PORTVERSION=	3.3.0
> [...]
>> +GH_TAGNAME=	v3_3_0
> 
> Why the regression?
> 

I just wasn't looking, I'll fix it.

> https://www.freebsd.org/doc/en/books/porters-handbook/makefile-distfiles.html#makefile-master_sites-github-ex3
> 
>> CMAKE_VERBOSE=	yes
> 
> Does nothing after r421635.
> 
>> +DOCS_CMAKE_ON=	-DNO_DOC:BOOL=OFF
>> +DOCS_CMAKE_OFF=	-DNO_DOC:BOOL=ON
> [...]
>> +EXAMPLES_CMAKE_ON=	-DNO_EXAMPLES:BOOL=OFF
>> +EXAMPLES_CMAKE_OFF=	-DNO_EXAMPLES:BOOL=ON
> [...]
>> +OPENCL_CMAKE_ON=	-DNO_OPENCL:BOOL=OFF
>> +OPENCL_CMAKE_OFF=	-DNO_OPENCL:BOOL=ON
> [...]
>> +PTEX_CMAKE_ON=	-DNO_PTEX:BOOL=OFF ...
>> +PTEX_CMAKE_OFF=	-DNO_PTEX:BOOL=ON
> [...]
>> +TBB_CMAKE_ON=	-DNO_TBB:BOOL=OFF
>> +TBB_CMAKE_OFF=	-DNO_TBB:BOOL=ON
> [...]
>> +TEST_CMAKE_ON=	-DNO_REGRESSION:BOOL=OFF -DNO_TESTS:BOOL=OFF -DNO_GLTESTS:BOOL=OFF
>> +TEST_CMAKE_OFF=	-DNO_REGRESSION:BOOL=ON -DNO_TESTS:BOOL=ON -DNO_GLTESTS:BOOL=ON
> [...]
>> +TUTORIALS_CMAKE_ON=	-DNO_TUTORIALS:BOOL=OFF
>> +TUTORIALS_CMAKE_OFF=	-DNO_TUTORIALS:BOOL=ON
> 
> Did you know _CMAKE_BOOL_OFF helper was added a year ago to simplify such cases?
> 

Ah yeah, forgot about that.

>> +OPENCL_CFLAGS=	-pthread
>> +OPENCL_CXXFLAGS=	-pthread
> 
> CFLAGS is appended to CXXFLAGS *by default*.
> 

Right, ok

>> +post-patch:
>> +	${FIND} ${WRKSRC}/tutorials -name CMakeLists.txt | ${XARGS} ${REINPLACE_CMD} \
>> +	-e 's|{CMAKE_BINDIR_BASE}/tutorials|{CMAKE_INSTALL_PREFIX}/share/${PORTNAME}/tutorials|g'
> 
> share/${PORTNAME} is ${DATADIR_REL}
> 

OK.

>> +	${FIND} ${WRKSRC}/examples -name CMakeLists.txt | ${XARGS} ${REINPLACE_CMD} \
>> +	-e 's|{CMAKE_BINDIR_BASE}|{CMAKE_INSTALL_PREFIX}/share/${PORTNAME}/examples|g'
> 
> share/${PORTNAME}/examples should probably be ${EXAMPLESDIR_REL}
> 

Sounds right

>> +	${FIND} ${WRKSRC}/regression -name CMakeLists.txt | ${XARGS} ${REINPLACE_CMD} \
>> +	-e 's|{CMAKE_BINDIR_BASE}|{CMAKE_INSTALL_PREFIX}/share/${PORTNAME}/test|g'
> 
> According to hier(7) and devel/kyua tests should probably go under tests/${PORTNAME}.
> 

Sure, tho the only other thing I see installing anything in 
${PREFIX}/tests is devel/libdistance.

> Why use xargs(1) for a feature built into find(1)?
> 
>       -exec utility [argument ...] {} +
>               Same as -exec, except that "{}" is replaced with as many
>               pathnames as possible for each invocation of utility.  This
>               behaviour is similar to that of xargs(1).  The primary always
>               returns true; if at least one invocation of utility returns a
>               non-zero exit status, find will return a non-zero exit status.
> 

There are many examples of this in ports Makefiles. The porter probably 
just copied one of those. To me, either is fine and works, but if you 
want to patch the 400+ ports that use find | xargs, so that it's 
uniform, that's fine.

Steve