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

[Jason Ekstrand]

On Aug 20, 2016 5:00 AM, "Emil Velikov" <emil.l.velikov at gmail.com> wrote:
>
> On 20 August 2016 at 07:25, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > On Aug 19, 2016 18:05, "Eric Engestrom" <eric.engestrom at imgtec.com>
wrote:
> >>
> >> On Fri, Aug 19, 2016 at 09:04:14AM -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.
> >> >
> Not sure I understand what this patch fixes that the Vulkan spec does
> not have a recommendation or two for. Can anyone elaborate step by
> step what we're having here, please ?
>
> Furthermore this sounds like a developer only thing, so perhaps we
> should restrict it there ? As-is we're effectively annoying some/most
> distros.

That a sticky question.  I believe including the full path is the "correct"
thing to do.  That is what the ICD file is there for after all.  Distros,
however, may choose to do differently for a packaged system-wide install
due to multi-arch issues.

The problem with a full path is because of an oversight in the defining of
the ICD file format, namely that they provided no way of specifying the
architecture of the .so.  This makes it very difficult to install both 32
and 64-bit versions side-by-side.  The solution that ajax chose to use in
his early fedora packages was to place libvulkan_Intel.so in the regular
library install path and not specify any path beyond the filename.  This
way the loader will call dlopen("libvulkan_Intel.so") and regular search
paths will be used.  Since the operating system sets those paths up
correctly, the correct version of libvulkan_Intel.so is loaded and off you
go.

Personally, I think the more correct way to handle this would be with an
architecture field in the ICD json file.  Proposing that and fixing up the
loader to handle has been on my to-do list for some time but has been
eclipsed by other things.

> >> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> >> > Cc: Mark Janes <mark.a.janes at intel.com>
> >>
> >> I have one question (below), but this patch is good regardless:
> >> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
> >>
> >> > ---
> >> >  src/intel/vulkan/.gitignore                            | 1 +
> >> >  src/intel/vulkan/Makefile.am                           | 7 ++++++-
> >> >  src/intel/vulkan/{intel_icd.json => intel_icd.json.in} | 2 +-
> >> >  3 files changed, 8 insertions(+), 2 deletions(-)
> >> >  rename src/intel/vulkan/{intel_icd.json => intel_icd.json.in} (59%)
> >> >
> >> > diff --git a/src/intel/vulkan/.gitignore
b/src/intel/vulkan/.gitignore
> >> > index bde5cd8..a099ff6 100644
> >> > --- a/src/intel/vulkan/.gitignore
> >> > +++ b/src/intel/vulkan/.gitignore
> >> > @@ -3,3 +3,4 @@
> >> >  /anv_entrypoints.h
> >> >  /anv_timestamp.h
> >> >  /dev_icd.json
> >> > +/intel_icd.json
> >> > diff --git a/src/intel/vulkan/Makefile.am
b/src/intel/vulkan/Makefile.am
> >> > index ad0148d..9fef960 100644
> >> > --- a/src/intel/vulkan/Makefile.am
> >> > +++ b/src/intel/vulkan/Makefile.am
> >> > @@ -141,7 +141,7 @@ anv_timestamp.h:
> >> >       $(AM_V_GEN) echo "#define ANV_TIMESTAMP \"$(TIMESTAMP_CMD)\""
> $@
> >> >
> >> >  BUILT_SOURCES = $(VULKAN_GENERATED_FILES)
> >> > -CLEANFILES = $(BUILT_SOURCES) dev_icd.json
> >> > +CLEANFILES = $(BUILT_SOURCES) dev_icd.json intel_icd.json
> >> >  EXTRA_DIST = \
> >> >       $(top_srcdir)/include/vulkan/vk_icd.h \
> >> >       anv_entrypoints_gen.py \
> >> > @@ -170,6 +170,11 @@ dev_icd.json : dev_icd.json.in
> >> >               -e "s#@build_libdir@#${abs_top_builddir}/${LIB_DIR}#" \
> >> >               < $(srcdir)/dev_icd.json.in > $@
> >> >
> >> > +intel_icd.json : intel_icd.json.in
> >> > +     $(AM_V_GEN) $(SED) \
> >> > +             -e "s#@install_libdir@#${libdir}#" \
> >> > +             < $(srcdir)/intel_icd.json.in > $@
> >>
> >> I think I may have already asked when dev_icd.json was added, but why
> >> use a relative path for the dependency and a full path when reading it?
> >> Why not use the full path for the dep and read `< $<` instead?
> >
> > No good reason that I know of other than copy+paste of the one above. We
> > could probably make that change.  Emil?
> >
> IIRC '< $<' was only applicable in some cases according to POSIX make.
> One the relative/full path, I think that non-gnumake implementations
> were having issues with the proposed solution. If anyone can confirm
> that it works I'm ok with changing it.
>
> Thanks
> Emil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160820/edb9c00e/attachment.html>