Bug 218332 - lang/pocl: update to 0.14 and use llvm40
Summary: lang/pocl: update to 0.14 and use llvm40
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Matthew Rezny
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-03 14:20 UTC by Matthew Rezny
Modified: 2017-05-12 17:09 UTC (History)
3 users (show)

See Also:
rezny: maintainer-feedback-


Attachments
update pocl to 0.14rc1 (10.71 KB, patch)
2017-04-03 14:20 UTC, Matthew Rezny
no flags Details | Diff
update pocl to 0.14 (12.61 KB, patch)
2017-04-11 11:38 UTC, Matthew Rezny
no flags Details | Diff
update pocl to 0.14 (12.77 KB, patch)
2017-05-03 10:21 UTC, Matthew Rezny
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Rezny freebsd_committer 2017-04-03 14:20:12 UTC
Created attachment 181431 [details]
update pocl to 0.14rc1

pocl 0.14 adds support for LLVM 3.9 and 4.0, and I've confirmed the build of rc1 with both. pocl also switched from autotools to CMake with this version, so there's a bit of churn in the Makefile. I hope I got it all right, please look closely to be sure.

The default LLVM version is set to llvm40, but I have also made it follow MESA_LLVM_VER to allow users to set a version for all the graphics related (OpenGL/OpenCL) ports at once.

I relaxed the ARCH restriction after verifying the build on i386. It looks like the ports should support some non-x86 architectures, but I do not have time right now to test building on any others. I may check PPC64 if time permits. I have not run the test suite as I do not have the CPU cycles to spare (need them for test builds, can't afford more hardware).

The port now builds a selection of OCL kernels for distribution. Since the default is to build "native", I suspect the binary packages for previous releases were of limited use.
Comment 1 Bugzilla Automation freebsd_committer 2017-04-03 14:20:12 UTC
Maintainer informed via mail
Comment 2 Martin Wilke freebsd_committer 2017-04-10 12:34:25 UTC
Submitter is committer, cc maintainer
Comment 3 Matthew Rezny freebsd_committer 2017-04-11 11:38:28 UTC
Created attachment 181686 [details]
update pocl to 0.14

I have updated the diff now that 0.14 has been released.
Comment 4 Matthew Rezny freebsd_committer 2017-04-26 11:24:17 UTC
Maintainer timeout
Comment 5 Jan Beich freebsd_committer 2017-04-29 20:56:49 UTC
Comment on attachment 181686 [details]
update pocl to 0.14

Mostly cosmetic nits:

> LICENSE=	GPLv3
> LICENSE_FILE=	${WRKSRC}/LICENSE

The code only references MIT license. pocl-0.14 added tinyprintf.[ch] under LGPL21+ but it's not yet used.

> OCL_ICD_VENDORS?=	${PREFIX}/etc/OpenCL/vendors

Without .export (see make(1)) or via MAKE_ENV the line has no effect. Am I wrong?

> WRKSRC=		${WRKDIR}/${PORTNAME}-${DISTVERSION}

This line is redundant. See DISTNAME and WRKSRC defaults in Mk/bsd.port.mk.

> PLIST_SUB=	OPSYS=${OPSYS:tl} ARCH=${ARCH:C/amd64/x86_64/}

As you don't use regex :S would work fine as well.

>  USE_GL=		yes

Nothing in the code uses <GL/glu.h>. Probably cruft or an obscure workaround.

> USES=		cmake gmake localbase ncurses pathfix pkgconfig libtool:keepla

- USES=cmake obsoletes at least USES=gmake and USES=libtool
- Explicit POCL_INSTALL_PKGCONFIG_DIR obsoletes USES=pathfix

> # needed for the libltdl configure check
> LDFLAGS+=	-L${LOCALBASE}/lib

Replace USES=localbase with USES=localbase:ldflags instead.

> DEBUG_CMAKE_BOOL_ON=	WITH_DEBUG

- _BOOL_ON helper doesn't exist, see Mk/bsd.options.mk. Either use _BOOL or _ON.
- WITH_DEBUG isn't used by pocl build system. Did you mean POCL_DEBUG_MESSAGES?

> OCLBOOK_ALL_TARGET=	test
>
> TEST_TARGET=	test

One of these is redundant but as https://github.com/freebsd/poudriere/pull/355 workaround it looks inconsistent. Maybe try the following:

  TEST_TARGET=	test

  OPTIONS_DEFINE=	TEST

  TEST_CMAKE_OFF=	-DENABLE_TESTSUITES=""
  TEST_CMAKE_ON=	-DENABLE_TESTSUITES="all"

  pre-install-TEST-on: do-test

> 	@${REINPLACE_CMD} -e 's/AMD64/amd64/' -e 's/i\.86/i386/' \

Why do you need to replace i.86 ? . (dot) matches "any single character" according to re_format(7) while CMake docs say:

  if(<variable|string> MATCHES regex)
      True if the given string or variable's value matches the given
      regular expression.
Comment 6 Matthew Rezny freebsd_committer 2017-05-03 10:20:45 UTC
(In reply to Jan Beich from comment #5)

You're late to the party; this update is now the topic of D10508. While it would be better in general to do reviews in phabricator, the questions were already posted here and the maintainer can reply here if so inclined.

>Mostly cosmetic nits:
>
> LICENSE=	GPLv3
> LICENSE_FILE=	${WRKSRC}/LICENSE
>
>The code only references MIT license. pocl-0.14 added tinyprintf.[ch] under LGPL21+ but it's not yet used.

I avoid touching LICENSE except when it's abundantly clear, e.g. none specified and there is only one, or the changelog indicates a re-licensing that can be easily verified. As there was one specified already, I did not question the maintainer's choice, but it does say MIT license on their website, so now wonder if there was a GPLv3 component that's since removed, so it should have been marked dual, or if that was a complete mistake.
>
> OCL_ICD_VENDORS?=	${PREFIX}/etc/OpenCL/vendors
>
>Without .export (see make(1)) or via MAKE_ENV the line has no effect. Am I wrong?

You are probably correct. I do not see any reference to it, so it may have been cruft even before the switch from autotools to cmake.
>
> WRKSRC=		${WRKDIR}/${PORTNAME}-${DISTVERSION}
>
>This line is redundant. See DISTNAME and WRKSRC defaults in Mk/bsd.port.mk.

Indeed, that is a relic from when the port version was 0.14-rc1.
>
> PLIST_SUB=	OPSYS=${OPSYS:tl} ARCH=${ARCH:C/amd64/x86_64/}
>
>As you don't use regex :S would work fine as well.
>
Good to know; given sed syntax, I would not have guessed :S is the simpler, non-regex replacement. I copied that from elsewhere but do not recall where so I cannot go 'correct' that example now.

>  USE_GL=		yes
>
>Nothing in the code uses <GL/glu.h>. Probably cruft or an obscure workaround.

Perhaps cruft. I assumed one of the GL headers was needed but it appears not to be.
>
> USES=		cmake gmake localbase ncurses pathfix pkgconfig libtool:keepla
>
>- USES=cmake obsoletes at least USES=gmake and USES=libtool
>- Explicit POCL_INSTALL_PKGCONFIG_DIR obsoletes USES=pathfix

libtool was already dropped in the review but I hadn't updated the patch here assuming nobody was looking since there had been no activity.
INSTALL lists both GNU make and cmake under Requirements, so I assumed they really needed gmake and left it in there, but testing has confirmed the build does complete without gmake.
>
> # needed for the libltdl configure check
> LDFLAGS+=	-L${LOCALBASE}/lib
>
>Replace USES=localbase with USES=localbase:ldflags instead.
>
> DEBUG_CMAKE_BOOL_ON=	WITH_DEBUG
>
>- _BOOL_ON helper doesn't exist, see Mk/bsd.options.mk. Either use _BOOL or _ON.
>- WITH_DEBUG isn't used by pocl build system. Did you mean POCL_DEBUG_MESSAGES?

WITH_DEBUG was a mistranslated relic, probably from outdated docs prior to release. POCL_DEBUG_MESSAGES is default ON and uses a runtime environment check so there is no need for that to be connected to the DEBUG switch, which works as-is.
>
> OCLBOOK_ALL_TARGET=	test
>
> TEST_TARGET=	test
>
>One of these is redundant but as https://github.com/freebsd/poudriere/pull/355 workaround it looks inconsistent. Maybe try the following:

Most of the _TARGET stuff is obsolete. I had changed them to valid targets, but all doesn't need to be set to all.
>
>  TEST_TARGET=	test
>
>  OPTIONS_DEFINE=	TEST
>
>  TEST_CMAKE_OFF=	-DENABLE_TESTSUITES=""
>  TEST_CMAKE_ON=	-DENABLE_TESTSUITES="all"
>
>  pre-install-TEST-on: do-test

The group of one did seem odd; a single TEST option makes more sense.
>
> 	@${REINPLACE_CMD} -e 's/AMD64/amd64/' -e 's/i\.86/i386/' \
>
>Why do you need to replace i.86 ? . (dot) matches "any single character" according to re_format(7) while CMake docs say:
>
>  if(<variable|string> MATCHES regex)
>      True if the given string or variable's value matches the given
>      regular expression.

That seemed necessary for the release candidate so there may have been another instance that was the target of the replacement but I don't recall now.
Comment 7 Matthew Rezny freebsd_committer 2017-05-03 10:21:30 UTC
Created attachment 182267 [details]
update pocl to 0.14
Comment 8 commit-hook freebsd_committer 2017-05-12 17:09:08 UTC
A commit references this bug:

Author: rezny
Date: Fri May 12 17:08:27 UTC 2017
New revision: 440691
URL: https://svnweb.freebsd.org/changeset/ports/440691

Log:
  Update to 0.14 and switch to llvm40 by default

  PR:		218332
  Reviewed by:	jbeich
  Approved by:	swills (mentor), maintainer (timeout)
  Differential Revision:	https://reviews.freebsd.org/D10508

Changes:
  head/lang/pocl/Makefile
  head/lang/pocl/distinfo
  head/lang/pocl/files/patch-CMakeLists.txt
  head/lang/pocl/files/patch-Makefile.in
  head/lang/pocl/files/patch-config.h.in.cmake
  head/lang/pocl/files/patch-lib_CL_devices_cpuinfo.c
  head/lang/pocl/files/patch-lib_CL_pocl__binary.c
  head/lang/pocl/files/patch-scripts_Makefile.in
  head/lang/pocl/files/patch-tests_regression_test__issue__445.cpp
  head/lang/pocl/pkg-plist