[icedtea-web] RFC: do not install netx.jar and plugin.jar into jdk dir
Omair Majid
omajid at redhat.com
Fri Feb 18 11:36:25 PST 2011
On 02/17/2011 07:23 PM, Dr Andrew John Hughes wrote:
> On 18:03 Thu 17 Feb , Omair Majid wrote:
>> On 02/17/2011 05:00 PM, Dr Andrew John Hughes wrote:
>>> On 16:39 Thu 17 Feb , Omair Majid wrote:
>>>> I have removed the pluginappletviewer binary. I had originally posted it
>>>> a separate patch, but since using pluginappletviewer would make this
>>>> patch more complex, I have removed it here.
>>>>
>>>
>>> Please do this in a separate patch prior to this. The two issues shouldn't
>>> be confused.
>>>
>>
>> I posted the patch already:
>> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-February/012010.html.
>> If you can approve it, I will remove pluginappletviewer before applying
>> this patch.
>>
Thanks for approving the pluginappletviewer patch. The updated patch now
assumes pluginappletviewer does not exist.
>>>> Currently, the jdk it uses to run is determined at build-time. In the
>>>> future, this will probably be changed so that icedtea-web will be able
>>>> to use any installed jdk.
>>>>
>>>
>>> Yeah I pondered this when reading the patch. What is wrong with just calling
>>> 'java'?
>>>
>>
>> Well, we do rely on certain API's that may not be present on all JDKs.
>> The configure checks are build time; so for now, the JRE to use is also
>> determined at build time.
>>
>> It's also in the interest of minimizing changes in this one patch. I am
>> quite sure most of the launcher code can be replaced by simple shell
>> scripts (or a simple call to java even). But that's for another patch.
>>
>
> Yes, I agree.
>
Great. I will avoid making large changes to the way the launcher works
for now then.
>>> I seem to remember there were some issues with libjava not being found if 1.0
>>> was installed in the wrong location and this was due to a relative LD_LIBRARY_PATH
>>> extension. I don't see this fixed. Do the new binaries work with just this
>>> patch?
>>>
>>
>> Hm.. no clue about the LD_LIBRARY_PATH issues. The binaries/plugin
>> produced after building icedtea-web with this patch applied (I used
>> ./configure --prefix=/home/omajid/code/icedtea-web-build/
>> --with-jdk-home=/home/omajid/code/icedtea6/openjdk.build/j2sdk-image/
>> --disable-docs) work perfectly fine for me.
>>
>> I did encounter an issue with libjava.so not being found. The method
>> GetIcedTeaWebJREPath fixes that. It uses the predefined path at build
>> time to locate the JRE rather than weird things like looking in the
>> directory above.
>
> Ok, that's the issue I was thinking of.
>
Ah, I see what you mean now. The launcher extends LD_LIBRAY_PATH using
the result of GetJREPath (now GetIcedTeaWebJREPath).
>>
>>>> Any thoughts or comments on the patch?
>>>>
>>>
>>> More inline.
>>>
>>>
>>>> diff -r 2289777074ae launcher/java_md.c
>>>> --- a/launcher/java_md.c Tue Feb 15 17:03:09 2011 -0500
>>>> +++ b/launcher/java_md.c Thu Feb 17 16:33:53 2011 -0500
>>>> @@ -158,6 +158,7 @@
>>>> static jboolean GetJVMPath(const char *jrepath, const char *jvmtype,
>>>> char *jvmpath, jint jvmpathsize, char * arch);
>>>> static jboolean GetJREPath(char *path, jint pathsize, char * arch, jboolean speculative);
>>>> +static jboolean GetIcedTeaWebJREPath(char *path, jint pathsize, char * arch, jboolean speculative);
>>>>
>>>> const char *
>>>> GetArch()
>>>> @@ -280,7 +281,9 @@
>>>> jvmpath does not exist */
>>>> if (wanted == running) {
>>>> /* Find out where the JRE is that we will be using. */
>>>> - if (!GetJREPath(jrepath, so_jrepath, arch, JNI_FALSE) ) {
>>>> +
>>>> + //if (!GetJREPath(jrepath, so_jrepath, arch, JNI_FALSE) ) {
>>>> + if (!GetIcedTeaWebJREPath(jrepath, so_jrepath, arch, JNI_FALSE) ) {
>>>
>>> Please just remove the line.
>>>
>>
>> Will do.
>>
Done.
>>>> fprintf(stderr, "Error: could not find Java 2 Runtime Environment.\n");
>>>> exit(2);
>>>> }
>>>> @@ -606,6 +609,33 @@
>>>> }
>>>>
>>>> /*
>>>> + * Find path to the JRE based on the the compile flag ICEDTEA_WEB_JRE
>>>> + */
>>>> +static jboolean
>>>> +GetIcedTeaWebJREPath(char* path, jint pathsize, char* arch, jboolean speculative)
>>>> +{
>>>> + char libjava[MAXPATHLEN];
>>>> + snprintf(libjava, MAXPATHLEN, ICEDTEA_WEB_JRE "/lib/%s/" JAVA_DLL, arch);
>>>> +
>>>> + if (_launcher_debug) {
>>>> + printf(ICEDTEA_WEB_JRE "/lib/%s/" JAVA_DLL "\n", arch);
>>>> + printf("libjava is %s\n", libjava);
>>>> + }
>>>> +
>>>
>>> Why is libjava being checked? Is this needed? I'm not sure it's standard and may
>>> be specified to Oracle JDKs.
>>>
>>
>> I wouldnt be surprised. The launcher code is fairly specific to OpenJDK.
>> Most of the code in this method is based on GetJREPath (which is the
>> method just below this one). The only significant change is where to
>> look for libjava - this method looks for it based on a compile time
>> constant; GetJREPath looks for it in the same directory as the binary.
>>
>
> I presume we could instead look for something more 'standard' like rt.jar?
>
Yes, but again, I would like to avoid modifying the launcher as much as
possible. The launcher needs to find libjava as it handles selection of
the appropriate JVM - it understands options like -d32/-d64 (and so
needs to check that the libjava.so file exists for the right
environment). As mentioned above, I would like to remove this but that
would be another patch.
>>>> + if (access(libjava, F_OK) == 0) {
>>>> + strncpy(path, ICEDTEA_WEB_JRE, pathsize);
>>>> + goto found;
>>>> + }
>>>> +
>>>> + return JNI_FALSE;
>>>> +
>>>> + found:
>>>> + if (_launcher_debug)
>>>> + printf("JRE path is %s\n", path);
>>>> + return JNI_TRUE;
>>>> +}
>>>> +
>>>> +/*
>>>> * Find path to JRE based on .exe's location or registry settings.
>>>> */
>>>> static jboolean
>>>> diff -r 2289777074ae plugin/icedteanp/IcedTeaNPPlugin.cc
>>>> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc Tue Feb 15 17:03:09 2011 -0500
>>>> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc Thu Feb 17 16:33:53 2011 -0500
>>>> @@ -1547,23 +1547,25 @@
>>>>
>>>> if (plugin_debug)
>>>> {
>>>> - command_line = (gchar**) malloc(sizeof(gchar*)*8);
>>>> + command_line = (gchar**) malloc(sizeof(gchar*)*9);
>>>> command_line[0] = g_strdup(appletviewer_executable);
>>>> - command_line[1] = g_strdup("-Xdebug");
>>>> - command_line[2] = g_strdup("-Xnoagent");
>>>> - command_line[3] = g_strdup("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n");
>>>> - command_line[4] = g_strdup("sun.applet.PluginMain");
>>>> - command_line[5] = g_strdup(out_pipe_name);
>>>> - command_line[6] = g_strdup(in_pipe_name);
>>>> - command_line[7] = NULL;
>>>> + command_line[1] = g_strdup(PLUGIN_BOOTCLASSPATH);
>>>> + command_line[2] = g_strdup("-Xdebug");
>>>> + command_line[3] = g_strdup("-Xnoagent");
>>>> + command_line[4] = g_strdup("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n");
>>>> + command_line[5] = g_strdup("sun.applet.PluginMain");
>>>> + command_line[6] = g_strdup(out_pipe_name);
>>>> + command_line[7] = g_strdup(in_pipe_name);
>>>> + command_line[8] = NULL;
>>>> } else
>>>> {
>>>> - command_line = (gchar**) malloc(sizeof(gchar*)*5);
>>>> + command_line = (gchar**) malloc(sizeof(gchar*)*6);
>>>> command_line[0] = g_strdup(appletviewer_executable);
>>>> - command_line[1] = g_strdup("sun.applet.PluginMain");
>>>> - command_line[2] = g_strdup(out_pipe_name);
>>>> - command_line[3] = g_strdup(in_pipe_name);
>>>> - command_line[4] = NULL;
>>>> + command_line[1] = g_strdup(PLUGIN_BOOTCLASSPATH);
>>>> + command_line[2] = g_strdup("sun.applet.PluginMain");
>>>> + command_line[3] = g_strdup(out_pipe_name);
>>>> + command_line[4] = g_strdup(in_pipe_name);
>>>> + command_line[5] = NULL;
>>>> }
>>>>
>>>> environment = plugin_filter_environment();
>>>> @@ -1590,17 +1592,21 @@
>>>> command_line[0] = NULL;
>>>> g_free (command_line[1]);
>>>> command_line[1] = NULL;
>>>> + g_free (command_line[2]);
>>>> + command_line[2] = NULL;
>>>> + g_free (command_line[3]);
>>>> + command_line[3] = NULL;
>>>> + g_free (command_line[4]);
>>>> + command_line[4] = NULL;
>>>>
>>>> if (plugin_debug)
>>>> {
>>>> - g_free (command_line[2]);
>>>> - command_line[2] = NULL;
>>>> - g_free (command_line[3]);
>>>> - command_line[3] = NULL;
>>>> - g_free (command_line[4]);
>>>> - command_line[4] = NULL;
>>>> g_free (command_line[5]);
>>>> command_line[5] = NULL;
>>>> + g_free (command_line[6]);
>>>> + command_line[6] = NULL;
>>>> + g_free (command_line[7]);
>>>> + command_line[7] = NULL;
>>>> }
>>>>
>>>> g_free(command_line);
>>>> @@ -2173,38 +2179,10 @@
>>>> }
>>>>
>>>> // Set appletviewer_executable.
>>>> - Dl_info info;
>>>> - int filename_size;
>>>> - if (dladdr ((const void*) ITNP_New,&info) == 0)
>>>> - {
>>>> - PLUGIN_ERROR_TWO ("Failed to determine plugin shared object filename",
>>>> - dlerror ());
>>>> - np_error = NPERR_GENERIC_ERROR;
>>>> - goto cleanup_data_directory;
>>>> - }
>>>> - filename = (gchar*) malloc(sizeof(gchar)*1024);
>>>> - filename_size = readlink(info.dli_fname, filename, 1023);
>>>> - if (filename_size>= 0)
>>>> - {
>>>> - filename[filename_size] = '\0';
>>>> - }
>>>> -
>>>> - if (!filename)
>>>> - {
>>>> - PLUGIN_ERROR ("Failed to create plugin shared object filename.");
>>>> - np_error = NPERR_OUT_OF_MEMORY_ERROR;
>>>> - goto cleanup_data_directory;
>>>> - }
>>>> -
>>>> - if (filename_size<= 0)
>>>> - {
>>>> - free(filename);
>>>> - filename = g_strdup(info.dli_fname);
>>>> - }
>>>> -
>>>> - appletviewer_executable = g_strdup_printf ("%s/../../bin/java",
>>>> - dirname (filename));
>>>> - PLUGIN_DEBUG(".so is located at: %s and the link points to: %s. Executing java from dir %s to run %s\n", info.dli_fname, filename, dirname (filename), appletviewer_executable);
>>>> + filename = g_strdup(ICEDTEA_WEB_JRE);
>>>> + appletviewer_executable = g_strdup_printf ("%s/bin/java",
>>>> + filename);
>>>> + PLUGIN_DEBUG("Executing java at %s\n", appletviewer_executable);
>>>> if (!appletviewer_executable)
>>>> {
>>>> PLUGIN_ERROR ("Failed to create appletviewer executable name.");
>>>
>>>
>>
>> Thanks for looking over the patch!
>>
Ok to commit?
Cheers,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: do-not-install-into-jdk-02.patch
Type: text/x-patch
Size: 11428 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110218/73fa3e28/do-not-install-into-jdk-02.patch
More information about the distro-pkg-dev
mailing list