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

[Emil Velikov]

On 5 May 2015 at 16:42, Tom Stellard <tom at stellard.net> wrote:
> On Tue, May 05, 2015 at 03:48:29PM +0100, Emil Velikov wrote:
>> 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.
>>
>
> Can you explain why the DRI, VAAPI, VDPAU, XvMC, and OMX situation is
> the same as OpenCL.  My understanding the install locations  for these
> are configurable.  Whereas the OpenCL spec forces you to install to /etc.
>
All of these have hardcoded path into the loader library. Mostly the
path is fixed, but in some cases one can change it at build time (via
a configure option). The loaders also provide (officially or not) a
way of overriding the said path.

I guess the difference here is that not all OpenCL loaders honour
OPENCL_VENDOR_PATH :-\


>> > 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.
>>
>
> I am in favor of some variant of this solution.  I personally prefer
> having two options one to use /etc and one to use $sysconfdif, which I
> think is more intuitive.
>
Having two options differs from the rest of mesa/gallium. Fwiw I would
vote for consistency (be that for the actual handling and configure
which name), as imho despite not being perfect it will be more
intuitive. Having X configure switches named/using behaviour 1, and Y
using 2 is perhaps something we can spare onto people ?

Cheers
Emil