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

Tue May 5 07:48:29 PDT 2015

On 4 May 2015 at 18:11, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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.
>
Fwiw I second Ilia's suggestion. Why ?
 - Overwriting system files (by default), when you want to install to
a custom location is not a good idea.
 - Package maintainers have sufficient knowledge which file should be
placed where on the specific distribution. For example distro A may
have a policy that the file must be in /usr/foo, with a symlink to it
in /etc.
 - This is not an OpenCL only "problem". DRI, VAAPI, VDPAU, XvMC and
OMX exhibit similar behaviour. Ilia's suggestion brings OpenCL
alongside the rest.

>>
>> 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.
>
If one does not use the distribution provided package then they either
install to a custom prefix or system wide. Either of which has its
caveats.
In the case of a custom prefix they can use an env var. Alternatively
the file will be installed in the correct place. If one changes
sysconfdir, then they should be fully aware that things are bound to
explode at any moment.

> 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...
>>>
The "issue" was about consistency & hand-holding people with
broken/buggy setups (omx-wize). I was looking for the former and
no-one was keen on the latter. Luckily Christian found a nice
workaround, although that one is not applicable here.

>>
>> 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.
>
/me waves

> My understanding was that the icd loader had a similar env var.
>
The variable name is OPENCL_VENDOR_PATH but not every loader honours
it :-( I'm assuming that's the case because it isn't part of the
specification. Although people who lack write access to /etc/ (or
prefer to keep the system one as is) make extensive use of it, when
possible.

> 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.
>
This sounds like a very clever solution, but it is not intuitive if
you lack prior knowledge about its workings. I can add "For
distribution maintainers" note as 10.6 comes out, though not too sure
how many people actually read these :-\
I'm fine with it either way, so I'll be up-to Tom and others to make the call.

Cheers,
Emil