Bug 94086 - Multiple conflicting libGL libraries installed
Summary: Multiple conflicting libGL libraries installed
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: GLX (show other bugs)
Version: git
Hardware: All All
: medium minor
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-10 23:28 UTC by Chuck Atkins
Modified: 2016-05-01 07:48 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch to add the --enable-gallium-xlib-glx option (3.63 KB, patch)
2016-04-14 15:16 UTC, Chuck Atkins
Details | Splinter Review
glx configure refactor: single-option verion (8.80 KB, patch)
2016-04-15 17:17 UTC, Chuck Atkins
Details | Splinter Review
glx config option refactor (v3) (9.34 KB, patch)
2016-04-19 15:32 UTC, Chuck Atkins
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chuck Atkins 2016-02-10 23:28:49 UTC
When enabling Gallium and glx-xlib with autotools, both a "classic mesa" libGL.so.1.6.0 and a gallium libGL.so.1.5.0 get built and installed.  Using the following configuration for software-only backends:

$ ./configure \
  --disable-va \
  --disable-gbm \
  --disable-xvmc \
  --disable-vdpau \
  --disable-dri \
  --with-dri-drivers= \
  --disable-egl \
  --with-egl-platforms= \
  --disable-gles1 \
  --disable-gles2 \
  --disable-shared-glapi \
  --disable-llvm-shared-libs \
  --enable-texture-float \
  --enable-glx \
  --enable-xlib-glx \
  --enable-gallium-llvm=yes \
  --enable-gallium-osmesa \
  --with-gallium-drivers=swrast \
  --prefix=/path/to/install
...
$ make
...
$ make install
..e
$ ls -l /path/to/install
libGL.la
libGL.so -> libGL.so.1.5.0
libGL.so.1 -> libGL.so.1.6.0
libGL.so.1.5.0
libGL.so.1.6.0
libOSMesa.la
libOSMesa.so -> libOSMesa.so.8.0.0
libOSMesa.so.8 -> libOSMesa.so.8.0.0
libOSMesa.so.8.0.0
pkgconfig

The following patch disables the 1.6.0 lib if gallium is enabled:

$ git diff
diff --git a/src/mesa/Makefile.am b/src/mesa/Makefile.am
index 8dc44fd..48d14ff 100644
--- a/src/mesa/Makefile.am
+++ b/src/mesa/Makefile.am
@@ -22,8 +22,10 @@
 SUBDIRS = . main/tests
 
 if HAVE_X11_DRIVER
+if !HAVE_GALLIUM
 SUBDIRS += drivers/x11
 endif
+endif
 
 if HAVE_DRICOMMON
 SUBDIRS += drivers/dri
Comment 1 Emil Velikov 2016-04-13 14:16:01 UTC
Hi Chuck,

Fwiw I'd rather have a configure error, if one tries to build conflicting (at install time) files.

Here is how we dealt it with osmesa - any chance you can send over to mesa-dev a port for xlib libGL ? 

2f142d596f6d950499d5e25d26e011a675c9670c
c8111904304a878a3d5831b453255f04e1ddcf56
Comment 2 Chuck Atkins 2016-04-13 20:16:07 UTC
> Fwiw I'd rather have a configure error, if one tries to build conflicting
> (at install time) files.

The glx options are implemented differently than the osmesa ones, even though they try to accomplish the same thing.  The osmesa options translate to "enable classic osmesa" or "enable gallium osmesa", but the glx options, on the other hand, translate to "enable glx period, (defaulting to dri-glx)" and "use xlib-glx instead of dri-glx".

After digging through the code a bit more, I believe my patch is perhaps not the right approach.  Is "classic" synonymous with dri?  There seems to currently be 3 GLX implementations: dri-glx in src/glx, classic-xlib-glx in src/mesa/x11, and gallium-xlib-glx in src/gallium/state_trackers/glx/xlib.  What is not clear to me now is under what situation should each of these implementations actually be built.  Currently both src/mesa/x11 and state_trackers/glx/xlib are getting built, which is the root of the problem I'm trying to address.  So what are the conditions then that src/mesa/x11 should be built vs when src/gallium/state_trackers/glx/xlib ?  Given that I can fix the patch accordingly.
Comment 3 Emil Velikov 2016-04-13 20:54:30 UTC
If you ignore the presence of dri powered libGL (one in src/glx) there're two libGL implementations - one 'classic' (src/mesa/foo) and one gallium (src/gallium/foo).

OSMesa is, was actually, in a similar boat. Thus to avoid the issue we added an error if one requests both of them at configure time. As miscommunication does happen, I've listed the sha(s) to illustrate what I was talking about :-)
Comment 4 Chuck Atkins 2016-04-14 15:16:50 UTC
Created attachment 122938 [details] [review]
Patch to add the --enable-gallium-xlib-glx option
Comment 5 Chuck Atkins 2016-04-14 15:53:59 UTC
So, based on our discussion, I believe I've addressed this in the attached patch now by adding an --enable-gallium-xlib-glx option to specify gallium or classic xlib-glx implementation.  I gave it this behavior instead of the osmesa approach of --enable-xlib-glx and --enable-gallium-xlib-glx being competing options in order to stay consistent with how the other GLX options work; i.e. --enable-foo turns on foo with default bar1 implementation and --enable-bar2-foo changes the implementation to bar2.  Invalid configurations for enabling the gallium-xlib-glx implementation will produce configure errors but there is no conflicting way to specify both classic and gallium xlib-glx implementations.
Comment 6 Emil Velikov 2016-04-14 16:04:19 UTC
(In reply to Chuck Atkins from comment #5)
> So, based on our discussion, I believe I've addressed this in the attached
> patch now by adding an --enable-gallium-xlib-glx option to specify gallium
> or classic xlib-glx implementation.  I gave it this behavior instead of the
> osmesa approach of --enable-xlib-glx and --enable-gallium-xlib-glx being
> competing options in order to stay consistent with how the other GLX options
> work; i.e. --enable-foo turns on foo with default bar1 implementation and
> --enable-bar2-foo changes the implementation to bar2.  Invalid
> configurations for enabling the gallium-xlib-glx implementation will produce
> configure errors but there is no conflicting way to specify both classic and
> gallium xlib-glx implementations.

I'd rather not do that, as the current "enable foo when bar is on" feels quite magic. Sometimes it gives you a warning although in all honestly, I doubt (m)any people really look at those. I would really want to nuke them altogether and make the configure shorter and cleaner.

A bunch of comments coming in a second, but for next round please send the patch to the mesa-dev mailing list (ideally via git send-email).

Thanks
Comment 7 Emil Velikov 2016-04-14 16:06:50 UTC
> --- a/configure.ac
> +++ b/configure.ac
> @@ -930,10 +930,9 @@ AC_ARG_ENABLE([xlib-glx],
>         [make GLX library Xlib-based instead of DRI-based @<:@default=disabled@:>@])],
>     [enable_xlib_glx="$enableval"],
>     [enable_xlib_glx=no])
>-
> AC_ARG_ENABLE([gallium-tests],
>     [AS_HELP_STRING([--enable-gallium-tests],
>-        [Enable optional Gallium tests) @<:@default=disabled@:>@])],
>+        [Enable optional Gallium tests @<:@default=disabled@:>@])],
Please, split this to a separate patch.

>@@ -957,6 +956,17 @@ case "$with_gallium_drivers" in
>     no) with_gallium_drivers='' ;;
> esac
> 
>+if test -n "$with_gallium_drivers" -a "x$enable_glx$enable_xlib_glx" = xyesyes; then
>+    GALLIUM_XLIB_GLX_DEFAULT="yes"
>+else
>+    GALLIUM_XLIB_GLX_DEFAULT="no"
>+fi
>+AC_ARG_ENABLE([gallium-xlib-glx],
>+    [AS_HELP_STRING([--enable-gallium-xlib-glx],
>+        [enable Gallium implementation of the Xlib-based GLX library @<:@default=enabled if using gallium and xlib-glx@:>@])],
>+    [enable_gallium_xlib_glx="$enableval"],
>+    [enable_gallium_xlib_glx="$GALLIUM_XLIB_GLX_DEFAULT"])
>+
I'd keep it disabled by default. Similar to its classic counterpart.

> if test "x$enable_opengl" = xno -a \
>         "x$enable_gles1" = xno -a \
>         "x$enable_gles2" = xno -a \
>@@ -1000,6 +1010,15 @@ fi
> if test "x$enable_opengl$enable_xlib_glx" = xnoyes; then
>     AC_MSG_ERROR([Xlib-GLX cannot be built without OpenGL])
> fi
>+if test -z "$with_gallium_drivers" -a \
>+           "x$enable_gallium_xlib_glx" = xyes; then
>+    AC_MSG_ERROR([Gallium Xlib-GLX cannot be built without Gallium])
Strictly speaking one should be able to build it without explicitly requiring any gallium drivers. Although that can happen once we fix a few bugs.

>+fi
>+if test "x$enable_xlib_glx$enable_gallium_xlib_glx" = xnoyes; then
wrong check - should be xyesyes

>+    AC_MSG_ERROR([Gallium Xlib-GLX cannot be built without Xlib-GLX])
and here "classic and gallium Xlib-GLX cannot be built together"

The remaining places that check enable_xlib_glx should be updated (or just nuked with preparatory patch) to attribute the gallium one as well.

The check around NEED_WINSYS_XLIB=foo should be the gallium one.

>@@ -2604,11 +2623,14 @@ if test "x$enable_dri" != xno; then
>         echo "        DRI driver dir:  $DRI_DRIVER_INSTALL_DIR"
> fi
> 
>-case "x$enable_glx$enable_xlib_glx" in
>-xyesyes)
>+case "x$enable_glx$enable_xlib_glx$enable_gallium_xlib_glx" in
>+xyesyesyes)
>+    echo "        GLX:             Xlib-based (Gallium)"
>+    ;;
>+xyesyesno)
>     echo "        GLX:             Xlib-based"
>     ;;
>-xyesno)
>+xyesno*)
With the above suggestions this can only be xyesnono.

>     echo "        GLX:             DRI-based"
>     ;;
> *)
>diff --git a/src/gallium/Makefile.am b/src/gallium/Makefile.am
>index 086e170..c4dc224 100644
>--- a/src/gallium/Makefile.am
>+++ b/src/gallium/Makefile.am
>@@ -141,8 +141,10 @@ SUBDIRS += state_trackers/dri targets/dri
> endif
> 
> if HAVE_X11_DRIVER
Drop this conditional.

>+if HAVE_GALLIUM_X11_DRIVER
> SUBDIRS += state_trackers/glx/xlib targets/libgl-xlib
> endif
>+endif
> 
> if HAVE_ST_OMX
> SUBDIRS += state_trackers/omx targets/omx
>diff --git a/src/mesa/Makefile.am b/src/mesa/Makefile.am
>index 3903818..7abc7b0 100644
>--- a/src/mesa/Makefile.am
>+++ b/src/mesa/Makefile.am
>@@ -22,8 +22,10 @@
> SUBDIRS = . main/tests
> 
> if HAVE_X11_DRIVER
>+if !HAVE_GALLIUM_X11_DRIVER
You don't need the extra conditional here.

Thanks for bringing this topic and working on the patch. I can realise it's not the easiest/prettiest of things ;-)
Comment 8 Chuck Atkins 2016-04-14 17:29:13 UTC
(In reply to Emil Velikov from comment #6)
> I'd rather not do that, as the current "enable foo when bar is on" feels
> quite magic. Sometimes it gives you a warning although in all honestly, I
> doubt (m)any people really look at those. I would really want to nuke them
> altogether and make the configure shorter and cleaner.

Just to be clear, before I re-work the patch, instead of enabling behavior as a set of cascading options, you'd rather see the three options as mutually exclusive at the top level, correct?

--enable-glx = enable DRI GLX
--enable-xlib-glx = enable classic Xlib GLX
--enable-gallium-xlib-glx = enable gallium Xlib GLX


> 
> A bunch of comments coming in a second, but for next round please send the
> patch to the mesa-dev mailing list (ideally via git send-email).

I'll push the next one to the list


(In reply to Emil Velikov from comment #7)
> Thanks for bringing this topic and working on the patch. I can realise it's
> not the easiest/prettiest of things

No it's not but it's far less frustrating to fix and then not have to deal with than to always have a hacked patch that has to get applied.  I'd much rather tell a customer who asks "How do I build Mesa to work with ParaView" to just download the mesa source, configure this way, and build than to tell them download mesa, apply this weird patch to the build system that you have to get from us, then configure this way and build.

As a developer at a company that produces open source software, I consider it just being a good citizen in the eco-system.
Comment 9 Chuck Atkins 2016-04-15 17:17:57 UTC
Created attachment 122974 [details] [review]
glx configure refactor: single-option verion
Comment 10 Chuck Atkins 2016-04-19 15:32:28 UTC
Created attachment 123058 [details] [review]
glx config option refactor (v3)

v3: Rebased to current master and added changelog in commit msg
Comment 11 Emil Velikov 2016-05-01 07:48:36 UTC
Thanks a million Chuck for untangling this and sticking through the process. The fix is in master now.


bug/show.html.tmpl processed on Jan 16, 2017 at 17:22:15.
(provided by the Example extension).