[Mesa-dev] [PATCH] anv: Give the installed intel_icd.json file an absolute path

[Emil Velikov]

On 23 August 2016 at 04:33, Nicholas Miell <nmiell at gmail.com> wrote:
> On 08/22/2016 04:10 PM, Julien Cristau wrote:
>>
>> On Mon, Aug 22, 2016 at 14:18:51 -0700, Jason Ekstrand wrote:
>>
>>> On Mon, Aug 22, 2016 at 2:06 PM, Julien Cristau <jcristau at debian.org>
>>> wrote:
>>>
>>>> On Fri, Aug 19, 2016 at 09:04:14 -0700, Jason Ekstrand wrote:
>>>>
>>>>> Not providing a path allows the ICD to work on multi-arch systems but
>>>>> breaks it if you install anywhere other than /usr/lib.  Given that
>>>>> users
>>>>> may be installing locally in .local or similar, we probably do want to
>>>>> provide a filename.  Distros can carry a revert of this commit if they
>>>>
>>>> want
>>>>>
>>>>> an intel_icd.json file without the path.
>>>>>
>>>> If a user is going to install stuff in .local, don't they have
>>>> LD_LIBRARY_PATH pointing there too?
>>>>
>>>
>>> Actually, no.  The loader will look for ICD files in
>>> .local/share/vulkan/icd.d and the ICD file will point to the right .so.
>>> It
>>> should work out-of-the-box unless you either have a broken loader or
>>> we're
>>> installing something wrong.
My understanding of the following section was that the user is
responsible of placing the ICD in such a way that dlopen finds it.
Thus it was rather confusing to me how/why this patch was needed since
an initial assumption of using LD_LIBRARY_PATH (as you would if you
install anything into the non-standard prefix) was taken as granted.

Although I see your point - asking people to fiddle with
LD_LIBRARY_PATH is a  bit annoying.


"If the ICD is specified via a filename, the loader will attempt to
open that file as a shared object using dlopen(), and the file must be
in a directory that dlopen is _configured_ to look in (Note: various
distributions are configured differently)"

* My emphasis on configured

>>
>>
>> So somehow they're only building the vulkan driver but not libGL or
>> anything else?  Still, I guess a bunch of people will need both a 32bit
>> and a 64bit version of the driver.  How is the 64-bit
>> ~/.local/share/vulkan/icd.d/intel_icd.json not going to clash with the
>> 32-bit ~/.local/share/vulkan/icd.d/intel_icd.json?  I'm just not seeing
>> how this solves the problem...
>>
>
> To recap the IRC discussion:
>
> ld-linux.so will expand $LIB to lib or lib64 as appropriate.
>
> Unfortunately, Debian patches glibc to make $LIB expand to the target triple
> and there's no convenient way to figure out how $LIB expands at build time.
>
> So you can just put $LIB in your architecture-independent ICD file and
> everything will work just fine, you just have to manually put the shared
> libraries in the appropriate location for your distribution.
>
Skimmed through the discussion and I'm not sure the above will be enough.

Since the user is free to place json files in $HOME/.local ... this
implies that they may _not_ have access to /usr or /etc. Thus as they
install the file (to say $HOME/foo/lib) the Vulkan loader will not be
able to pick it up.
In theory one should be able to have a varying .json (one with and one
without the path) although the heuristics will be fragile due to the
varying $LIB (like the case of Debian and derivatives) and $DESTDIR.

So I see the following options:
 - new configure option - have to agree with Dave, please don't.
Furthermore we already have more than 50! of them.
 - fold the "w or w/o full path" under and existing option
(--enable-debug ?) - somewhat picky/fragile as well. Kind of OK for
short term solution.
 - use LD_LIBRARY_PATH - slightly annoying yet add once and forget
about it. OK-ish short term solution.
 - json update - the better long term solution imho.

Json update:
 - the same file _cannot_ be provided by multiple packages
 - thus we should use differently named files, but we should not rely
on the filename to determine arch/triple.
There has been no such recommendations/restrictions about the name so
starting now feels wrong.
 - so let's bump "file_format_version" - 1.0.1 or 1.1.0 ?
 - add ICD::arch tag and teach the loader to honour it. For json files
missing it we'll assume a "can be used on this platform" behaviour.
 - should we bother with the other parts of the triple or add it only
as need arise ?

With the above in place, one can have full path + filename for the
mutlilib setups. Old loaders will just get an error upon dlopen of
incompatible/wrong arch ICDs.

How do the above short/long term solutions sound ?

Thanks
Emil