[rfc][icedtea-web] replacing of hardcoded jre path by function

[Adam Domurad]

On 02/22/2013 08:03 AM, Jiri Vanek wrote:
> Hi!
>
> This is replacing of hardoced path part 2. This is replacing the 
> constants ICEDTEA_WEB_JRE by functions. However those functions are 
> just returning the (ICEDTEA_WEB_JRE) consatnts now :) But they should 
> prepare path for another parts...
>
> The goal is to have logic in those functions, which will provide the 
> value from deploy.propertie file(s) (or use default (As is as now) if 
> property will not exists....
>
> J
>
>
> ps: this is part of make-jredir-configurable after install effort

> diff -r 5501859497c2 plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc    Fri Feb 22 11:43:19 2013 
> +0100
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc    Fri Feb 22 13:51:39 2013 
> +0100
> @@ -156,8 +156,14 @@
>  // Data directory for plugin.
>  static std::string data_directory;
>
> -// Fully-qualified appletviewer executable.
> -static const char* appletviewer_executable = ICEDTEA_WEB_JRE "/bin/java";
> +// Fully-qualified appletviewer default  executable and rt.jar
> +static const char* appletviewer_default_executable = ICEDTEA_WEB_JRE 
> "/bin/java";
> +static const char* appletviewer_default_rtjar = ICEDTEA_WEB_JRE 
> "/lib/rt.jar";
> +// Fully-qualified appletviewer default  executable and rt.jar
> +// resolved by methods in runtime. It is expected they will be the 
> same during
> +//whole runtime
> +static char appletviewer_executable[256];
> +static char appletviewer_rtjar[256];

Java user afraid of the heap ? :-) These static buffers are not needed.

>
>  // Applet viewer input channel (needs to be static because it is used 
> in plugin_in_pipe_callback)
>  static GIOChannel* in_from_appletviewer = NULL;
> @@ -284,6 +290,16 @@
>
>  #endif
>
> +static char* getPluginExecutbale(char* dest){

s/Executbale/Executable/


> +      strcpy(dest, appletviewer_default_executable);
> +      return dest;
> +}
> +
> +static char* getPluginRtJar(char* dest){
> +      strcpy(dest, appletviewer_default_rtjar);
> +      return dest;
> +}

I prefer get_plugin_java_executable() & get_plugin_java_runtime_jar(). 
No big deal though, but I would use underscores for consistency however 
you name it.

Also please use the signature:

static std::string whatever_name() {
       return ICEDTEA_WEB_JRE "/bin/java"; // or lib/rt.jar
}

You can get rid of the constant this way, too. Note that char* is valid 
wherever std::string is taken (but not the other way around).
Note that for now the signature *could* be const char* -- but I strongly 
encourage std::string for when this becomes read from the properties.


> +
>
>  /*
>   * Find first member in GHashTable* depending on version of glib
> @@ -1517,13 +1533,14 @@
>  static NPError
>  plugin_test_appletviewer ()
>  {
> -  PLUGIN_DEBUG ("plugin_test_appletviewer: %s\n", 
> appletviewer_executable);
> +
> +  PLUGIN_DEBUG ("plugin_test_appletviewer: %s\n", 
> getPluginExecutbale(appletviewer_executable));

Just a hint, this will need a c_str() here when changed to std::string

>    NPError error = NPERR_NO_ERROR;
>
>    gchar* command_line[3] = { NULL, NULL, NULL };
>    gchar** environment;
>
> -  command_line[0] = g_strdup (appletviewer_executable);
> +  command_line[0] = g_strdup 
> (getPluginExecutbale(appletviewer_executable));

Same

>    command_line[1] = g_strdup("-version");
>    command_line[2] = NULL;
>
> @@ -1570,7 +1587,7 @@
>
>    // Construct command line parameters
>
> -  command_line.push_back(appletviewer_executable);
> + command_line.push_back(getPluginExecutbale(appletviewer_executable));

Same

>
>    //Add JVM args to command_line
>    for (int i = 0; i < jvm_args->size(); i++)
> @@ -1581,7 +1598,7 @@
>    command_line.push_back(PLUGIN_BOOTCLASSPATH);
>    // set the classpath to avoid using the default (cwd).
>    command_line.push_back("-classpath");
> -  command_line.push_back(ICEDTEA_WEB_JRE "/lib/rt.jar");
> +  command_line.push_back(getPluginRtJar(appletviewer_rtjar));
>
>    // Enable coverage agent if we are running instrumented plugin
>  #ifdef COVERAGE_AGENT
> @@ -1652,10 +1669,10 @@
>    gchar *output = NULL;
>    std::vector<std::string*>* tokenOutput = NULL;
>
> -  commands.push_back(appletviewer_executable);
> + commands.push_back(getPluginExecutbale(appletviewer_executable));
>    commands.push_back(PLUGIN_BOOTCLASSPATH);
>    commands.push_back("-classpath");
> -  commands.push_back(ICEDTEA_WEB_JRE "/lib/rt.jar");
> +  commands.push_back(getPluginRtJar(appletviewer_rtjar));
> commands.push_back("net.sourceforge.jnlp.controlpanel.CommandLine");
>    commands.push_back("get");
>    commands.push_back("deployment.plugin.jvm.arguments");
> @@ -2175,11 +2192,11 @@
>      }
>
>    // Set appletviewer_executable.
> -  PLUGIN_DEBUG("Executing java at %s\n", appletviewer_executable);
> +  PLUGIN_DEBUG("Executing java at %s\n", 
> getPluginExecutbale(appletviewer_executable));

Same

>    np_error = plugin_test_appletviewer ();
>    if (np_error != NPERR_NO_ERROR)
>      {
> -      fprintf(stderr, "Unable to find java executable %s\n", 
> appletviewer_executable);
> +      fprintf(stderr, "Unable to find java executable %s\n", 
> getPluginExecutbale(appletviewer_executable));

Same

>        return np_error;
>      }
>
> @@ -2191,7 +2208,7 @@
>
>    plugin_instance_mutex = g_mutex_new ();
>
> -  PLUGIN_DEBUG ("NP_Initialize: using %s\n", appletviewer_executable);
> +  PLUGIN_DEBUG ("NP_Initialize: using %s\n", 
> getPluginExecutbale(appletviewer_executable));

Same

>
>    plugin_req_proc = new PluginRequestProcessor();
>    java_req_proc = new JavaMessageSender();

The commands vector will not need the c_str(), because it is a vector of 
std::string (things are much nicer when everything uses std::string, we 
should move towards that!)

Thanks for looking into it, looks fine otherwise.
-Adam