[RFC]: make IcedTea-web to be compatible with RHEL5 libraries
Jiri Vanek
jvanek at redhat.com
Mon Aug 29 04:03:24 PDT 2011
On 08/28/2011 06:10 AM, Dr Andrew John Hughes wrote:
> On 15:35 Fri 26 Aug , Jiri Vanek wrote:
>
> snip...
>
> Comments below.
>
>> <jvanek_home> dbhole, I can not reply your email now (I'm not on working environment) .. but to clarify : you wrote that iterator appeared in 2.15. It si not true - documentation said 2.16, but on 2.12.5 it is working also....
...
>> <dbhole> jvanek_home: This function is in glib
>
> I've already pointed this out once when you posted this patch in an earlier form.
Yes you did... I had "luckily" overlooked it and it caused me a lot of troubles:-/
>
>> <jvanek_home> omg
...
>> <dbhole> jvanek_home: doh! don't have a rhel5 vm :/ .. provisioning one on to-openjdk1 now
>> ....
>
> $ pkg-config --modversion glib-2.0
>
> as you probably found out. Surprised you even have glib 1.2...
yy;)
>
...>
>> diff -r 36270c76a533 Makefile.am
>> --- a/Makefile.am Wed Aug 24 15:17:46 2011 -0400
>> +++ b/Makefile.am Fri Aug 26 15:23:27 2011 +0200
>> @@ -210,6 +210,7 @@
>> mkdir -p $(PLUGIN_DIR)&& \
>> cd $(PLUGIN_DIR)&& \
>> $(CXX) $(CXXFLAGS) \
>> + $(GLIB214) \
>> -DJDK_UPDATE_VERSION="\"$(JDK_UPDATE_VERSION)\"" \
>> -DPLUGIN_NAME="\"IcedTea-Web Plugin\"" \
>> -DPLUGIN_VERSION="\"$(PLUGIN_VERSION)\"" \
>> @@ -225,6 +226,7 @@
>> $(PLUGIN_DIR)/IcedTeaPlugin.so: $(addprefix $(PLUGIN_DIR)/,$(PLUGIN_OBJECTS))
>> cd $(PLUGIN_DIR)&& \
>> $(CXX) $(CXXFLAGS) \
>> + $(GLIB214) \
>> $(PLUGIN_OBJECTS) \
>> $(GLIB_LIBS) \
>> $(GTK_LIBS) \
>
> This should use $(DEFS). See below.
refactored (see updated patch ad end of this email)
> Why do we need this twice? I think the second one is unnecessary, as we are
> just linking object files.
Right! Second call removed.
>
>> diff -r 36270c76a533 acinclude.m4
>> --- a/acinclude.m4 Wed Aug 24 15:17:46 2011 -0400
>> +++ b/acinclude.m4 Fri Aug 26 15:23:27 2011 +0200
>> @@ -484,6 +484,18 @@
>> AC_SUBST(PKGVERSION)
>> ])
>>
>> +AC_DEFUN_ONCE([IT_GET_GLIBVERSION],
>> +[
>> +PKG_CHECK_MODULES([GLIB2_V_216],[glib-2.0>= 2.16],
>> +[
>> +GLIB214=""
>> +],[
>> +GLIB214=" -DGLIB214 "
>> +])
>> +AC_SUBST([GLIB214])
>> +]
>> +)
>> +
>
> Use AC_DEFINE([LEGACY_GLIB],1); that's the standard way to do
> it and the naming isn't then tied to the test.
done
>
> Why are you testing for>= 2.16 when the discussion implied
> the functions appeared in 2.15?
As Deepak dig-out, it was committed into 2.15, but documentation is clear that those functions are available since 2.16, so I have used this officially announced version.
>
> You don't need the AC_SUBST as you don't add anything to the
> Makefile.am, although it doesn't hurt.
ok, removed.
>
>> AC_DEFUN([IT_CHECK_WITH_GCJ],
>> [
>> AC_MSG_CHECKING([whether to compile ecj natively])
>> diff -r 36270c76a533 configure.ac
>> --- a/configure.ac Wed Aug 24 15:17:46 2011 -0400
>> +++ b/configure.ac Fri Aug 26 15:23:27 2011 +0200
>> @@ -84,7 +84,7 @@
>> #
>> # Find optional depedencies
>> #
>> -
>> +IT_GET_GLIBVERSION
>> IT_FIND_OPTIONAL_JAR([rhino], RHINO,
>> [/usr/share/java/js.jar /usr/share/rhino-1.6/lib/js.jar])
>> IT_FIND_OPTIONAL_JAR([junit], JUNIT,
>
> This is the wrong place. These aren't optional dependencies.
> And that empty line shouldn't be removed.
>
>> diff -r 36270c76a533 plugin/icedteanp/IcedTeaNPPlugin.cc
>> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc Wed Aug 24 15:17:46 2011 -0400
>> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc Fri Aug 26 15:23:27 2011 +0200
>> @@ -47,6 +47,10 @@
>> #include<sys/types.h>
>> #include<unistd.h>
>>
>> +#ifdef GLIB214
>> +#include<glib.h>
>> +#endif
>> +
>
> This seems wrong. How does this work if glib.h is not included?
> Have you checked this still works without the define?
hmhm.. It is long time since I have added this include into this patch. Now I have try to compile both (defined and undefined GLIB214) and it is compiling fine, So i have removed this include utterly.
>
>> // Liveconnect extension
>> #include "IcedTeaScriptablePluginObject.h"
snip
>> + } else
>> + {
>> + return NPERR_GENERIC_ERROR;
>> + }
>> +#endif
>
> This would be a cleaner patch if the test was inverted i.e. #ifndef LEGACY_GLIB
Although I do not see the point why it is cleaner - done.
>
>>
>> #endif
>>
>> @@ -1363,21 +1391,38 @@
>>
>> #else
>>
>> +#ifdef GLIB214
>> if (browser_functions.getvalueforurl)
>> {
>>
>> - // As in get_cookie_info, we use the first active instance
>> - GHashTableIter iter;
>> + // As in get_cookie_info, we use the first active instance
>> gpointer id, instance;
>>
>> - g_hash_table_iter_init (&iter, instance_to_id_map);
>> - g_hash_table_iter_next (&iter,&instance,&id);
>> + g_hash_table_find(instance_to_id_map, (GHRFunc)find_first_item_in_hash_table,&instance);
>>
>> browser_functions.getvalueforurl((NPP) instance, NPNURLVProxy, siteAddr, proxy, len);
>> } else
>> {
>> return NPERR_GENERIC_ERROR;
>> }
>> +#else
>> + if (browser_functions.getvalueforurl)
>> + {
>> +
>> + // As in get_cookie_info, we use the first active instance
>> + GHashTableIter iter;
>> + gpointer id, instance;
>> +
>> + g_hash_table_iter_init (&iter, instance_to_id_map);
>> + g_hash_table_iter_next (&iter,&instance,&id);
>> +
>> + browser_functions.getvalueforurl((NPP) instance, NPNURLVProxy, siteAddr, proxy, len);
>> + } else
>> + {
>> + return NPERR_GENERIC_ERROR;
>> + }
>> +#endif
>> +
>
> Likewise.
> Could this not be factored out into a common function or define so we don't have this duplication?
I have refactored those two (four) duplicated blocks into state you can see in rhel5compatible-duplicateTableSearch.diff.
My intentions for this patch were to touch the code I do not understand full as seldom as possible, but as you are suggesting to remove this duplicity i have also extracted those 6lines into separatedmethod. (rhel5compatible.diff)
Although I have tested those binaries on rhel5,6 and f13 and everything seems to be working, I'm not 100% sure whether I did not broke something.
>
>> #endif
>>
>> return NPERR_NO_ERROR;
>> @@ -1403,6 +1448,17 @@
>> return FALSE;
>> }
>>
>> +#ifdef GLIB214
>> +int
>> +strcmp0(char *str1, char *str2)
>> +{
>> + if (str1 != NULL)
>> + return str2 != NULL ? strcmp(str1, str2) : 1;
>> + else // str1 == NULL
>> + return str2 != NULL ? 1 : 0;
>> +}
>> +#endif
>> +
>
> Just define g_strcmp0 here and the function below should work unchanged.
done. Thanx for this.
>
>> // remove all components from LD_LIBRARY_PATH, which start with
>> // MOZILLA_FIVE_HOME; firefox has its own NSS based security provider,
>> // which conflicts with the one configured in nss.cfg.
>> @@ -1424,7 +1480,11 @@
>> components = g_strsplit (path_old, ":", -1);
>> for (i1 = 0, i2 = 0; components[i1] != NULL; i1++)
>> {
>> +#ifdef GLIB214
>> + if (strcmp0 (components[i1], moz_home) == 0
>> +#else
>> if (g_strcmp0 (components[i1], moz_home) == 0
>> +#endif
>> || g_str_has_prefix (components[i1], moz_home))
>> components[i2] = components[i1];
>> else
>
>
The patch have shrunk a lot, and now I'm not absolutely sure that I did not broke something. My intentions for this were to touch the code I do not understand full as seldom as possible, but due to reviewing issues i made much more changes.
Please note that I have touched and reused $DEFS, and moved block of code in IcedTeaNPPlugin.cc into separate method. Although I have tested those binaries on rhel5,6 and f13 and everything seems to be working, I'm really not 100% sure.
Best Regards
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rhel5compatible.diff
Type: text/x-patch
Size: 3885 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110829/edd698f7/rhel5compatible.diff
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rhel5compatible-duplicateTableSearch.diff
Type: text/x-patch
Size: 3938 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110829/edd698f7/rhel5compatible-duplicateTableSearch.diff
More information about the distro-pkg-dev
mailing list