[Mesa-dev] [PATCH] clover: add --with-icd-file-dir option

[Ilia Mirkin]

On Mon, May 4, 2015 at 12:47 PM, Tom Stellard <tom at stellard.net> wrote:
> On Mon, May 04, 2015 at 10:13:19AM -0400, Ilia Mirkin wrote:
>> On Mon, May 4, 2015 at 10:04 AM, Tom Stellard <tom at stellard.net> wrote:
>> > On Sat, May 02, 2015 at 01:31:41PM -0400, Ilia Mirkin wrote:
>> >> On Sat, May 2, 2015 at 1:19 PM, EdB <edb+mesa at sigluy.net> wrote:
>> >> > The standard ICD file path is /etc/OpenCL/vendor/.
>> >> > However it doesn't fit well with custom build.
>> >> > This option allow ICD vendor file installation path override
>> >> > ---
>> >> >  configure.ac                           | 6 ++++++
>> >> >  src/gallium/targets/opencl/Makefile.am | 2 +-
>> >> >  2 files changed, 7 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/configure.ac b/configure.ac
>> >> > index 095e23e..bf08d76 100644
>> >> > --- a/configure.ac
>> >> > +++ b/configure.ac
>> >> > @@ -2005,6 +2005,12 @@ AC_ARG_WITH([d3d-libdir],
>> >> >      [D3D_DRIVER_INSTALL_DIR="$withval"],
>> >> >      [D3D_DRIVER_INSTALL_DIR="${libdir}/d3d"])
>> >> >  AC_SUBST([D3D_DRIVER_INSTALL_DIR])
>> >> > +AC_ARG_WITH([icd-file-dir],
>> >> > +    [AS_HELP_STRING([--with-icd-file-dir=DIR],
>> >> > +        [directory for the OpenCL ICD vendor file @<:@/etc/OpenCL/vendors@:>@])],
>> >> > +    [ICD_FILE_INSTALL_DIR="$withval"],
>> >> > +    [ICD_FILE_INSTALL_DIR="/etc/OpenCL/vendors"])
>> >>
>> >> What about making this default to ${sysconfdir}/OpenCL/vendors ? That
>> >> way using --prefix should auto-make it go into the prefix instead of
>> >> unexpectedly installing things outside of the specified prefix? That
>> >> way a distro build which specifies --sysconfdir as /etc will get it in
>> >> the right place, while by default it'll go into /usr/local/etc and a
>> >> user can override the icd loader's default behaviour with
>> >> OPENCL_VENDOR_PATH?
>> >>
>> >
>> > I would prefer not to make this the default behavior, because it violates the spec
>> > and there could potentially be multiple icd implementations, which may or may not have
>> > the overrides.
>> >
>> > I think the best solution would be to rename the option to something like
>> > --enable-ocl-icd-respect-prefix (suggestions for other names encouraged).
>> > and have the option enable the behavior that Ilia is describing.
>> >
>> > This will give distros and advanced users a way to setup their system
>> > the way they want.
>>
>> It's just a very anti-autoconf thing to do to have "make install" fail
>> by default unless you specify some "hey, i actually want make install
>> to work" option.
>>
>> I think it's crazy to expect that, by default, people will want to
>> write over their system installs, and having things go outside of the
>> specified --prefix is very surprising (unless you force some other
>> option). And asking the user to run "make install" as root is even
>> crazier.
>>
>
> My expectation is that, by default, when people specify --enable-opencl-icd
> they want an implementation that conforms to the specification.
> Unfortunately, this means installing icd files to /etc.

Oh, does this only happen when I supply some option? I.e. if I just do
./configure --prefix=... it'll still work, I have to do
--enable-opencl-icd and only *then* does it install the other thing?
That might be more acceptable, although still not really.

>
> There is no good solution here, but I'd rather have users specify a flag
> to get a sane build system, than requiring them to set a flag and set
> an environment variable just to get working OpenCL with the ICD loader.

I know exceedingly little about OpenCL. I'm just coming at this from a
generic "build shouldn't try to install things outside the prefix"
perspective. Make install shouldn't fail, and users shouldn't be
tempted into accidentally overwriting their system installs even
though they set a prefix but then foolishly ran 'sudo make install'.

Autoconf is a pretty common build helper, nearly every sane package
with a unix build target uses it, and I think people are used to how
it works. One of the cool things is that you can do "./configure
--prefix=/home/user/install; make install" and expect it to work (as
in, things landing in /home/user/install/..., not in /). Or perhaps
you use a system like 'stow' which takes care of all the
symlinking/etc and allows you to easily have multiple versions of the
same software.

Having things go outside of the configured directories goes against
the autoconf standard... perhaps it'd be OK if you added some giant
warning at the end of configure output saying "DANGER DANGER DANGER
will do unexpected things by default!".

>
>> I guess I haven't hit this yet because there's no OpenCL support in
>> nouveau or freedreno, but I made the same stink about vdpau when Emil
>> tried to make it install to some system location by default. At least
>> a few people seemed to agree with me back then...
>>
>
> Does the vdpau spec also require installation to a specific system director
> (e.g. /etc/) ?

The vdpau loader (libvdpau) loads from a fixed (and arbitrarily
configurable at build-time, and practically speaking set differently
on different distributions) library directory, much like (the mesa)
libGL does. However there's (now) a VDPAU_DRIVER_PATH or something
along those lines that can be passed in.

My understanding was that the icd loader had a similar env var.

How about this -- get rid of --enable-opencl-icd and change it to
--with-opencl-icd-dir where you must supply a directory if you want to
use it. That way if you want the system one, you'd say
/etc/OpenCL/vendors, if you want your own custom dir, then put that
in.

  -ilia