svn commit: r437953 - in head/lang/beignet: . files

[Matthew Rezny]

On Friday 07 April 2017 23:34:03 Jan Beich wrote:
> Matthew Rezny <rezny at freebsd.org> writes:
> > On Friday 07 April 2017 22:23:18 Jan Beich wrote:
> >> Matthew Rezny <rezny at FreeBSD.org> writes:
> >> >   - Enable OpenCL 2.0 on AMD64
> >> 
> >> Isn't this premature before 1.3.2 or bug 217635 fix?
> > 
> > There was no clear answer given in that PR as to whether or not version
> > 1.3.1 addressed the issue, but as I understood it the issue in that PR
> > was caused by a lack of OpenCL 2.0 so I do not see how enabling it would
> > be premature.
> DRM_IOCTL_I915_GEM_USERPTR is only available on drm-next but even there is
> half broken. Beignet *requires* it for OpenCL 2.0 but, looking more, the
> default is still 1.2. The patch just limits the ioctl to when an application
> explicitly requests 2.0.
> 
> If you want a clear answer look no further than comment 0. OpenCL 2.0
> support upstream *is* the source of the regression. The patch just have a
> different effect on 1.3.0 and 1.3.1 but still required for both on FreeBSD.
> 
Given that comments 0 and 1 in 217635 are the reporting of the original issue, 
I assume you are referring to comment 0 of 217771, which is not the correct 
place to give an answer about 217635. However, that was just a summary of your 
testing without any statement of impact on 217635. Clear as mud.

Not long after creating 217771, you posted comment 4 to 217635 that reads 
"With bug 217771 in scope comment 0 is limited to OPENCL20=off while comment 1 
no longer happens." To me, that says both issues in 217635 are fixed if Beignet 
is updated to 1.3.1 and OpenCL 2.0 support is enabled.

I wanted to be absolutely clear on that, hence comment 5; "I was going to 
suggest trying 1.3.1 since it was released today but it appears you are ahead 
of me. If I understand your last reply correctly, there is no problem and no 
need for the patch if beignet is built with OpenCL 2.0 support enabled, 
correct?"

That only needed a simple yes or no answer, but more than 3 weeks have passed 
without any answer to it in either PR. You seemed to have time to question my 
work in other areas while remaining silent on that detail, so I could only 
assume that the issue was resolved and you did not feel the need to give 
confirmation. Most often I get no direct confirmation when I fix something so the 
the silence serves as confirmation.

> >> > +USE_LDCONFIG=	${LOCALBASE}/lib/${PORTNAME}
> >> 
> >> Why do you need ldconfig hints for dlopen(3)? libcl.so embeds absolute
> >> paths for libgbe.so and libgbeinterp.so + absolutepath via ocl-icd
> >> config in /usr/local/etc/OpenCL/vendors/intel-beignet.icd
> >> 
> >> Please, don't turn portlint into a cargo cult.
> > 
> > Nothing to do with portlint, just following what is written in the
> > Porter's
> > Handbook about ports that install shared libraries and USE_LDCONFIG.
> 
> Porter's Handook says "Please double-check, often this is not necessary at
> all". For an X11-related example, dri, libva, libvdpau, xorg-server load
> modules under /usr/local/lib/foo without adding it to USE_LDCONFIG e.g.,
> 
>   /usr/local/lib/xorg/modules/libglamoregl.so
> 
I am aware X.org does things it's own way, including module loading and symbol 
resolution, but this is not Xorg nor is ocl-icd.

The section on shared libraries starts with "If the port installs one or more 
shared libraries, define a USE_LDCONFIG make variable, which will instruct a 
bsd.port.mk to run ${LDCONFIG} -m on the directory where the new library is 
installed (usually PREFIX/lib) during post-install target to register it into 
the shared library cache." which makes it sound like the default should be to 
USE_LDCONFIG is there are any .so files installed. There is no statement about 
when to install shared libraries without USE_LDCONFIG.

As your incomplete quote follows "The default directory can be overridden by 
setting USE_LDCONFIG to a list of directories into which shared libraries are 
to be installed. " and ends with " or can be avoided through -rpath or setting 
LD_RUN_PATH during linking (see lang/moscow_ml for an example), or through a 
shell-wrapper which sets LD_LIBRARY_PATH before invoking the binary, like 
www/seamonkey does.", the "not necessary at all" is clearly in reference to 
overriding the location with USE_LDCONFIG and not simply the use of 
USE_LDCONFIG.

So, the PHB suggests methods to keep it as a simple USE_LDCONFIG=yes, but does 
not suggest conditions in which USE_LDCONFIG would be superfluous. That was 
your opportunity to fill the void with a better explanation, but instead of 
imparting any useful information, you just put a negative spin on a well 
intended attempt to follow the written directions.

Fortunately, someone else has followed up with a more useful response; 
"Blindly following the PHB (or portlint) is still merely cargo-culting.
USE_LDCONFIG is bogus for *.so's that are not being normally linked by
consumers, but dlopen(3)ed during runtime instead."
Sentence 1 I'm ignoring as the product of this thread.
Sentence 2 is what would have helped to see in the PHB or earlier in this 
thread.

> >> > +LLVMVER=	${MESA_LLVM_VER:U39}
> >> 
> >> Why not drop LLVMVER variable and use MESA_LLVM_VER directly?
> >> 
> >>   BUILD_DEPENDS=	clang${MESA_LLVM_VER}:devel/llvm${MESA_LLVM_VER} \
> >>   [...]
> >>   MESA_LLVM_VER?=	39
> > 
> > Conversely, why do it that way? I see no benefit, just more lines changed.
> 
> ${LLVMVER} != ${MESA_LLVM_VER} cannot happen, so an extra variable may
> confuse the next maintainer.

Using MESA_LLVM_VER throughout would seem more confusing to me than to have 
one place where Mesa's variable influences the local LLVMVER. So, this is just 
another bit of style nit-picking like where to add whitespace.

Given that your interaction thus far has consisted primarily of useless 
criticism, where half might be constructive if some reasoning was provided but 
half is just personal opinion/style, it feels like your aim is to cause grief 
rather than improve or educate, so I may just begin ignoring you in favor of 
those who can provide useful feedback.