Re: Patch: install libgtkpeer.so and libjawt.so in versioned library directory

[Thomas Fitzsimmons <fitzsim at redhat dot com>]

Bryce McKinlay wrote:
> Thomas Fitzsimmons wrote:
> > One case that this patch does not handle is automatic LD_LIBRARY_PATH 
> > handling for GCJ-native-compiled libjawt.so applications.  Users 
> > running such binaries will have to manually point LD_LIBRARY_PATH at 
> > $(libdir)/gcj-$(gcc_version).
>
> This only effects applications that use libjawt (which are fairly rare), 
> right?

Right.

> > When this patch is accepted, I'll submit a corresponding patch to GNU 
> > Classpath.
> >
> > OK to commit?
>
> The only issue I see with this patch is that it makes us more dependent 
> on paths which are hard-coded at compile time - this means that tests 
> won't work (or the wrong gij will be used) when run from an uninstalled 
> tree, for example. These paths should be determined dynamically at 
> runtime. For the JNI library path, it is a fairly minor issue and we 
> have this problem already, but hardcoding the path to gij could be 
> problematic:
> > +    -DGIJ_EXECUTABLE="\"$(bindir)/gij$(EXEEXT)\"" \
> > +      int error_code = execv (GIJ_EXECUTABLE, (char* const*) argv);
> > +
>
> Wouldn't it be better to use argv[0] here? That should ensure that the 
> correct gij is always invoked.

Yes.

Attached patch committed.

Tom

2006-05-30  Thomas Fitzsimmons  <fitzsim@redhat.com>

	* scripts/makemake.tcl (emit_bc_rule): Do not skip
	gnu-java-awt-peer-gtk.lo.
	Include gnu/java/awt/peer/gtk Java objects in libgcj.so.  Use C++
	ABI for gnu/java/awt/peer/gtk package.
	* gnu/classpath/natSystemProperties.cc (PrependVersionedLibdir):
	New function.
	(insertSystemProperties): Only set java.ext.dirs if it is not
	already defined.  Prepend GCJ_VERSIONED_LIBDIR to module search
	path where necessary.
	* configure.ac (GTK_AWT): Remove automake conditional.
	Do not add gnu/java/awt/peer/gtk to standard.omit.
	* include/jvm.h (_Jv_PrependVersionedLibdir): New function
	declaration.
	* gij.cc (main): Prepend LD_LIBRARY_PATH with GCJ_VERSIONED_LIBDIR
	and re-exec self.
	* Makefile.am (AM_CXXFLAGS): Define GCJ_VERSIONED_LIBDIR and
	PATH_SEPARATOR macros.
	Remove lib-gnu-java-awt-peer-gtk.la and libgcjawt.la build logic.
	* prims.cc (_Jv_PrependVersionedLibdir): New function.

2006-05-30  Thomas Fitzsimmons  <fitzsim@redhat.com>

	* native/jni/gtk-peer/Makefile.am (gcc_version): New variable.
	(gcjversionedlibdir): Likewise.
	(libgtkpeer_la_LDFLAGS): Likewise.
	Install libgtkpeer.so in GCJ versioned library directory.
	* native/jawt/Makefile.am (gcc_version): New variable.
	(gcjversionedlibdir): Likewise.
	(libjawt_la_LDFLAGS): Likewise.
	Rename libjawtgnu.so libjawt.so.  Install libjawt.so in GCJ
	versioned library directory.
	* gnu/java/awt/peer/gtk/GdkFontPeer.java (static): Call
	System.loadLibrary unconditionally.
	* gnu/java/awt/peer/gtk/GdkPixbufDecoder.java: Likewise.
	* gnu/java/awt/peer/gtk/GdkGraphics2D.java: Likewise.
	* gnu/java/awt/peer/gtk/GdkGraphics.java: Likewise.
	* gnu/java/awt/peer/gtk/GtkToolkit.java: Likewise.
	* gnu/java/awt/peer/gtk/GdkTextLayout.java: Likewise.

Index: scripts/makemake.tcl
===================================================================
--- scripts/makemake.tcl	(revision 114136)
+++ scripts/makemake.tcl	(working copy)
@@ -258,8 +258,7 @@
 
   # We skip these because they are built into their own libraries and
   # are handled specially in Makefile.am.
-  if {$loname != "gnu-java-awt-peer-gtk.lo"
-      && $loname != "gnu-java-awt-peer-qt.lo"} {
+  if {$loname != "gnu-java-awt-peer-qt.lo"} {
     lappend bc_objects $loname
   }
 }
Index: gnu/classpath/natSystemProperties.cc
===================================================================
--- gnu/classpath/natSystemProperties.cc	(revision 114136)
+++ gnu/classpath/natSystemProperties.cc	(working copy)
@@ -121,6 +121,26 @@
 }
 #endif
 
+// Prepend GCJ_VERSIONED_LIBDIR to a module search path stored in a
+// Java string, if the path is not already prefixed by
+// GCJ_VERSIONED_LIBDIR.  Return a newly JvMalloc'd char buffer.  The
+// result should be freed using JvFree.  See
+// _Jv_PrependVersionedLibdir in prims.cc.
+static char*
+PrependVersionedLibdir (::java::lang::String* libpath)
+{
+  char* retval = 0;
+
+  // Extract a C char array from libpath.
+  char* val = (char*) _Jv_Malloc (JvGetStringUTFLength (libpath) + 1);
+  jsize total = JvGetStringUTFRegion (libpath, 0, libpath->length(), val);
+  val[total] = '\0';
+  retval = _Jv_PrependVersionedLibdir (val);
+  JvFree (val);
+
+  return retval;
+}
+
 void
 gnu::classpath::SystemProperties::insertSystemProperties (java::util::Properties *newprops)
 {
@@ -288,8 +308,11 @@
       SET ("user.region", "US");
     }  
 
-  // The java extensions directory.
-  SET ("java.ext.dirs", JAVA_EXT_DIRS);
+  // Set the java extension directories property if it has not yet been
+  // specified.
+  ::java::lang::String *extdirs = newprops->getProperty(JvNewStringLatin1("java.ext.dirs"));
+  if (! extdirs)
+    SET ("java.ext.dirs", JAVA_EXT_DIRS);
 
   // The endorsed directories that libgcj knows about by default.
   // This is a way to get other jars into the boot class loader
@@ -343,9 +366,9 @@
   ::java::lang::String *path = newprops->getProperty(JvNewStringLatin1("java.library.path"));
   if (path)
     {
-      char *val = (char *) _Jv_Malloc (JvGetStringUTFLength (path) + 1);
-      jsize total = JvGetStringUTFRegion (path, 0, path->length(), val);
-      val[total] = '\0';
+      // Prepend GCJ_VERSIONED_LIBDIR to the module load path so that
+      // libgcj will find its own JNI libraries, like libgtkpeer.so.
+      char* val = PrependVersionedLibdir (path);
       _Jv_SetDLLSearchPath (val);
       _Jv_Free (val);
     }
@@ -354,11 +377,10 @@
       // Set a value for user code to see.
 #ifdef USE_LTDL
       char *libpath = getenv (LTDL_SHLIBPATH_VAR);
-      if (libpath)
-        newprops->put(JvNewStringLatin1 ("java.library.path"),
-                      JvNewStringLatin1 (libpath));
-      else
-        SET ("java.library.path", "");
+      char* val = _Jv_PrependVersionedLibdir (libpath);
+      SET ("java.library.path", val);
+      _Jv_SetDLLSearchPath (val);
+      _Jv_Free (val);
 #else
       SET ("java.library.path", "");
 #endif
Index: configure.ac
===================================================================
--- configure.ac	(revision 114136)
+++ configure.ac	(working copy)
@@ -227,7 +227,6 @@
 done
 
 AM_CONDITIONAL(XLIB_AWT, test "$use_xlib_awt" = yes)
-AM_CONDITIONAL(GTK_AWT, test "$use_gtk_awt" = yes)
 AM_CONDITIONAL(QT_AWT, test "$use_qt_awt" = yes)
 
 # Create standard.omit based on decisions we just made.
@@ -236,9 +235,6 @@
    echo gnu/awt/xlib >> standard.omit
    echo gnu/gcj/xlib >> standard.omit
 fi
-if test "$use_gtk_awt" != yes; then
-   echo gnu/java/awt/peer/gtk >> standard.omit
-fi
 if test "$use_qt_awt" != yes; then
    echo gnu/java/awt/peer/qt >> standard.omit
 fi
Index: classpath/native/jni/gtk-peer/Makefile.am
===================================================================
--- classpath/native/jni/gtk-peer/Makefile.am	(revision 114136)
+++ classpath/native/jni/gtk-peer/Makefile.am	(working copy)
@@ -1,5 +1,8 @@
-## GCJ LOCAL: don't install this library
-noinst_LTLIBRARIES = libgtkpeer.la
+## GCJ LOCAL: install this library in GCJ's versioned library
+## directory
+gcc_version := $(shell cat $(top_srcdir)/../../gcc/BASE-VER)
+gcjversionedlibdir = $(libdir)/gcj-$(gcc_version)
+gcjversionedlib_LTLIBRARIES = libgtkpeer.la
 
 # Gtk/Cairo JNI sources.
 if GTK_CAIRO
@@ -52,6 +55,10 @@
 			gtk_jawt.c \
 			gtkpeer.h
 
+## GCJ LOCAL: encode the library path and  use GCJ's library version
+libgtkpeer_la_LDFLAGS = -rpath $(gcjversionedlibdir) \
+	-version-info `grep -v '^\#' $(top_srcdir)/../libtool-version`
+
 libgtkpeer_la_LIBADD = $(top_builddir)/native/jni/classpath/native_state.lo \
 		       $(top_builddir)/native/jni/classpath/jcl.lo
 
Index: classpath/native/jawt/Makefile.am
===================================================================
--- classpath/native/jawt/Makefile.am	(revision 114136)
+++ classpath/native/jawt/Makefile.am	(working copy)
@@ -1,13 +1,16 @@
-## GCJ LOCAL: don't install this library
-noinst_LTLIBRARIES = libjawtgnu.la
+## GCJ LOCAL: install this library in GCJ's versioned library
+## directory
+gcc_version := $(shell cat $(top_srcdir)/../../gcc/BASE-VER)
+gcjversionedlibdir = $(libdir)/gcj-$(gcc_version)
+gcjversionedlib_LTLIBRARIES = libjawt.la
 
-libjawtgnu_la_SOURCES = jawt.c
-libjawtgnu_la_LIBADD = $(top_builddir)/native/jni/gtk-peer/libgtkpeer.la
-# FIXME: libtool doesn't allow overriding SONAME, but we need to set
-# it to libjawt.so for binary compatibility.
-#
-# libjawtgnu_la_LDFLAGS = -Wl,-soname -Wl,libjawt.so
+libjawt_la_SOURCES = jawt.c
+libjawt_la_LIBADD = $(top_builddir)/native/jni/gtk-peer/libgtkpeer.la
 
+## GCJ LOCAL: encode the library path and  use GCJ's library version
+libjawt_la_LDFLAGS = -rpath $(gcjversionedlibdir) \
+	-version-info `grep -v '^\#' $(top_srcdir)/../libtool-version`
+
 AM_LDFLAGS = @CLASSPATH_MODULE@ @GTK_LIBS@ @CAIRO_LIBS@ @PANGOFT2_LIBS@ @X_LIBS@ -lXtst
 AM_CPPFLAGS = @CLASSPATH_INCLUDES@
 
Index: classpath/gnu/java/awt/peer/gtk/GdkFontPeer.java
===================================================================
--- classpath/gnu/java/awt/peer/gtk/GdkFontPeer.java	(revision 114136)
+++ classpath/gnu/java/awt/peer/gtk/GdkFontPeer.java	(working copy)
@@ -62,10 +62,7 @@
   
   static 
   {
-    if (Configuration.INIT_LOAD_LIBRARY)
-      {
-        System.loadLibrary("gtkpeer");
-      }
+    System.loadLibrary("gtkpeer");
 
     initStaticState ();
 
Index: classpath/gnu/java/awt/peer/gtk/GdkPixbufDecoder.java
===================================================================
--- classpath/gnu/java/awt/peer/gtk/GdkPixbufDecoder.java	(revision 114136)
+++ classpath/gnu/java/awt/peer/gtk/GdkPixbufDecoder.java	(working copy)
@@ -75,10 +75,8 @@
 {
   static 
   {
-    if (Configuration.INIT_LOAD_LIBRARY)
-      {
-        System.loadLibrary("gtkpeer");
-      }
+    System.loadLibrary("gtkpeer");
+
     initStaticState ();
   }
   
Index: classpath/gnu/java/awt/peer/gtk/GdkGraphics2D.java
===================================================================
--- classpath/gnu/java/awt/peer/gtk/GdkGraphics2D.java	(revision 114136)
+++ classpath/gnu/java/awt/peer/gtk/GdkGraphics2D.java	(working copy)
@@ -104,8 +104,7 @@
       throw new Error("Graphics2D not implemented. "
 		      + "Cairo was not found or disabled at configure time");
 
-    if (Configuration.INIT_LOAD_LIBRARY)
-      System.loadLibrary("gtkpeer");
+    System.loadLibrary("gtkpeer");
 
     initStaticState();
   }
Index: classpath/gnu/java/awt/peer/gtk/GdkGraphics.java
===================================================================
--- classpath/gnu/java/awt/peer/gtk/GdkGraphics.java	(revision 114136)
+++ classpath/gnu/java/awt/peer/gtk/GdkGraphics.java	(working copy)
@@ -56,10 +56,8 @@
 {
   static 
   {
-    if (Configuration.INIT_LOAD_LIBRARY)
-      {
-        System.loadLibrary("gtkpeer");
-      }
+    System.loadLibrary("gtkpeer");
+
     initStaticState ();
   }
   
Index: classpath/gnu/java/awt/peer/gtk/GtkToolkit.java
===================================================================
--- classpath/gnu/java/awt/peer/gtk/GtkToolkit.java	(revision 114136)
+++ classpath/gnu/java/awt/peer/gtk/GtkToolkit.java	(working copy)
@@ -107,8 +107,7 @@
 
   static
   {
-    if (Configuration.INIT_LOAD_LIBRARY)
-      System.loadLibrary("gtkpeer");
+    System.loadLibrary("gtkpeer");
 
     int portableNativeSync;     
     String portNatSyncProp = 
Index: classpath/gnu/java/awt/peer/gtk/GdkTextLayout.java
===================================================================
--- classpath/gnu/java/awt/peer/gtk/GdkTextLayout.java	(revision 114136)
+++ classpath/gnu/java/awt/peer/gtk/GdkTextLayout.java	(working copy)
@@ -70,10 +70,8 @@
   // native side, plumbing, etc.
   static 
   {
-    if (Configuration.INIT_LOAD_LIBRARY)
-      {
-        System.loadLibrary("gtkpeer");
-      }
+    System.loadLibrary("gtkpeer");
+
     initStaticState ();
   }
   private native void setText(String str);
Index: include/jvm.h
===================================================================
--- include/jvm.h	(revision 114136)
+++ include/jvm.h	(working copy)
@@ -660,4 +660,7 @@
   return c->state == JV_STATE_PHANTOM;
 }
 
+// A helper function defined in prims.cc.
+char* _Jv_PrependVersionedLibdir (char* libpath);
+
 #endif /* __JAVA_JVM_H__ */
Index: gij.cc
===================================================================
--- gij.cc	(revision 114136)
+++ gij.cc	(working copy)
@@ -14,6 +14,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
+#include <unistd.h>
 
 static void
 help ()
@@ -71,6 +72,47 @@
 int
 main (int argc, char const** argv)
 {
+  // libjawt.so must be installed in GCJ's versioned directory and not
+  // the main library directory so that it doesn't override other
+  // libjawt.so implementations.  Programs that use the AWT Native
+  // Interface contain a JNI library that links to libjawt.so.  We do
+  // not want to require that users explicitly add GCJ's versioned
+  // directory to LD_LIBRARY_PATH when running such programs.
+
+  // Simply adding GCJ's versioned directory to the module load path
+  // does not solve this problem since libltdl searches its module
+  // load path only for object that it will dlopen; dependencies of
+  // these dynamically loaded objects are searched for in
+  // LD_LIBRARY_PATH.
+
+  // In addition, setting LD_LIBRARY_PATH from within the current
+  // process does not alter the dependency search path, since it is
+  // computed on startup.  This behaviour makes sense since
+  // LD_LIBRARY_PATH is designed to allow users to override the path
+  // set by a program.  This re-spawning trick makes it impossible to
+  // override, using LD_LIBRARY_PATH, the versioned directories
+  // searched by gij.
+
+  // Check if LD_LIBRARY_PATH is already prefixed with
+  // GCJ_VERSIONED_LIBDIR.  If not, export LD_LIBRARY_PATH prefixed
+  // with GCJ_VERSIONED_LIBDIR and re-spawn gij.
+  char *libpath = getenv (LTDL_SHLIBPATH_VAR);
+  char *newpath = _Jv_PrependVersionedLibdir (libpath);
+
+  if (! libpath || strcmp (libpath, newpath))
+    {
+      setenv (LTDL_SHLIBPATH_VAR, newpath, 1);
+      JvFree (newpath);
+
+      int error_code = execvp (argv[0], (char* const*) argv);
+
+      fprintf (stderr, "error re-spawning gij with new "
+               LTDL_SHLIBPATH_VAR " value: %s\n", strerror (error_code));
+
+      return error_code;
+    }
+  JvFree (newpath);
+
   JvVMInitArgs vm_args;
   bool jar_mode = false;
 
Index: Makefile.am
===================================================================
--- Makefile.am	(revision 114136)
+++ Makefile.am	(working copy)
@@ -40,10 +40,6 @@
 toolexeclib_LTLIBRARIES += lib-gnu-awt-xlib.la
 endif
 
-if GTK_AWT
-toolexeclib_LTLIBRARIES += lib-gnu-java-awt-peer-gtk.la libgcjawt.la
-endif
-
 if QT_AWT
 toolexeclib_LTLIBRARIES += lib-gnu-java-awt-peer-qt.la
 endif
@@ -128,6 +124,8 @@
         -DBOOT_CLASS_PATH="\"$(BOOT_CLASS_PATH_DIR)\"" \
 	-DJAVA_EXT_DIRS="\"$(jardir)/ext\"" \
 	-DGCJ_ENDORSED_DIRS="\"$(jardir)/gcj-endorsed\"" \
+	-DGCJ_VERSIONED_LIBDIR="\"$(libdir)/gcj-$(gcc_version)\"" \
+	-DPATH_SEPARATOR="\"$(CLASSPATH_SEPARATOR)\"" \
 	-DLIBGCJ_DEFAULT_DATABASE="\"$(dbexecdir)/$(db_name)\"" \
 	-DLIBGCJ_DEFAULT_DATABASE_PATH_TAIL="\"$(db_pathtail)\"" \
 	-DTOOLEXECLIBDIR="\"$(toolexeclibdir)\""
@@ -247,33 +245,6 @@
 ## not available.
 	./gcj-dbtool -n $(db_name) || touch $(db_name)
 
-## For the peer library, DEPENDENCIES need to come before OBJECTS so
-## that JNI headers are built before JNI C files.
-$(lib_gnu_java_awt_peer_gtk_la_OBJECTS): $(lib_gnu_java_awt_peer_gtk_la_DEPENDENCIES)
-
-lib_gnu_java_awt_peer_gtk_la_SOURCES =
-lib_gnu_java_awt_peer_gtk_la_LIBADD = \
-	gnu-java-awt-peer-gtk.lo \
-	classpath/native/jni/gtk-peer/libgtkpeer.la \
-	$(GTK_LIBS) $(GLIB_LIBS) $(LIBART_LIBS) $(CAIRO_LIBS) $(PANGOFT2_LIBS)
-lib_gnu_java_awt_peer_gtk_la_DEPENDENCIES = gnu-java-awt-peer-gtk.lo \
-	classpath/native/jni/gtk-peer/libgtkpeer.la \
-	libgcj-$(gcc_version).jar libgcj.la libgcj.spec
-## The mysterious backslash in the grep pattern is consumed by make.
-lib_gnu_java_awt_peer_gtk_la_LDFLAGS = \
-        -version-info `grep -v '^\#' $(srcdir)/libtool-version` $(LIBGCJ_LD_SYMBOLIC) $(X_LIBS) -lXtst
-lib_gnu_java_awt_peer_gtk_la_LINK = $(LIBLINK)
-
-libgcjawt_la_SOURCES = classpath/native/jawt/jawt.c
-libgcjawt_la_CFLAGS = -I$(srcdir)/classpath/native/jni/classpath \
-	$(PEDANTIC_CFLAGS) $(X_CFLAGS)
-## See jv_convert_LDADD.
-libgcjawt_la_LIBADD = -L$(here)/.libs lib-gnu-java-awt-peer-gtk.la
-libgcjawt_la_LDFLAGS = \
-	-version-info `grep -v '^\#' $(srcdir)/libtool-version` \
-	$(LIBGCJ_LD_SYMBOLIC)
-libgcjawt_la_LINK = $(LIBLINK)
-
 lib_gnu_java_awt_peer_qt_la_SOURCES =
 lib_gnu_java_awt_peer_qt_la_LIBADD = \
 	gnu-java-awt-peer-qt.lo \
Index: prims.cc
===================================================================
--- prims.cc	(revision 114136)
+++ prims.cc	(working copy)
@@ -1723,3 +1723,45 @@
 	      && _Jv_ClassNameSamePackage (self_klass->name,
 					   other_klass->name)));
 }
+
+// Prepend GCJ_VERSIONED_LIBDIR to a module search path stored in a C
+// char array, if the path is not already prefixed by
+// GCJ_VERSIONED_LIBDIR.  Return a newly JvMalloc'd char buffer.  The
+// result should be freed using JvFree.
+char*
+_Jv_PrependVersionedLibdir (char* libpath)
+{
+  char* retval = 0;
+
+  if (libpath && libpath[0] != '\0')
+    {
+      if (! strncmp (libpath,
+                     GCJ_VERSIONED_LIBDIR,
+                     sizeof (GCJ_VERSIONED_LIBDIR) - 1))
+        {
+          // LD_LIBRARY_PATH is already prefixed with
+          // GCJ_VERSIONED_LIBDIR.
+          retval = (char*) _Jv_Malloc (strlen (libpath) + 1);
+          strcpy (retval, libpath);
+        }
+      else
+        {
+          // LD_LIBRARY_PATH is not prefixed with
+          // GCJ_VERSIONED_LIBDIR.
+          jsize total = (sizeof (GCJ_VERSIONED_LIBDIR) - 1)
+            + (sizeof (PATH_SEPARATOR) - 1) + strlen (libpath) + 1;
+          retval = (char*) _Jv_Malloc (total);
+          strcpy (retval, GCJ_VERSIONED_LIBDIR);
+          strcat (retval, PATH_SEPARATOR);
+          strcat (retval, libpath);
+        }
+    }
+  else
+    {
+      // LD_LIBRARY_PATH was not specified or is empty.
+      retval = (char*) _Jv_Malloc (sizeof (GCJ_VERSIONED_LIBDIR));
+      strcpy (retval, GCJ_VERSIONED_LIBDIR);
+    }
+
+  return retval;
+}