[rfc][icedtea-web] dialogue for set jre dir
Adam Domurad
adomurad at redhat.com
Fri Apr 5 08:38:21 PDT 2013
[.. Everything snipped ..]
Looks good overall.
- Detecting Oracle JDK works. However I'm not sure this is a 'great'
result if nothing works. Also note:
http://i.imgur.com/OwmiVVq.png
This should be more clear that the warning is because we are not
selecting OpenJDK, and perhaps say why (ie, plugin won't work)
More issues:
Using javaws with Oracle JDK:
Your custom JRE /usr/java/jdk1.7.0_13 read from deployment.properties
under key deployment.jre.dir as /usr/java/jdk1.7.0_13 is not valid.
Using default (/usr/lib/jvm/java-openjdk/jre/bin/java,
/usr/lib/jvm/java-openjdk/jre/lib/rt.jar) in attempt to start. Please
fix this.
Note thate bin/javaws exists in this directory. Adding a / to the end of
the dir didn't help. It did at least fall back to IcedTea correctly.
Using plugin with Oracle JDK:
"Your custom jre (/lib/rt.jar check) /usr/java/jdk1.7.0_13/ is not
valid. Please fix deployment.jre.dir in your deployment.properties. In
attempt to run using default one.
Exception in thread "PluginMessageHandlerWorker1"
java.lang.IllegalAccessError: class net.sourceforge.jnlp.NetxPanel
cannot access its superclass sun.applet.AppletViewerPanel"
The exception is expected -- but the 'is not valid' message is not.
Additionally it does not seem to be falling back correctly.
Nits:
- It'd be nice if the explicit validate option were greyed out when
automatic validation is enabled
- There's a YES/NO/CANCEL option on the warning dialogue (the one in my
image), but it seems NO & CANCEL do the same thing. It'd be nice if one
of the options reverted to the default.
Comments on code:
> [.. snip ..]
> - File jreFile = new File(System.getProperty("java.home") +
> File.separator + "lib"
> - + File.separator + DEPLOYMENT_CONFIG);
> + String jrePath = null;
> + try {
> + Map<String, Setting<String>> tmpProperties =
> parsePropertiesFile(USER_DEPLOYMENT_PROPERTIES_FILE);
> + Setting<String> jreSetting = tmpProperties.get(KEY_JRE_DIR);
> + if (jreSetting != null) {
> + jrePath = jreSetting.getValue();
> + }
> + } catch (Exception ex) {
[nit] Rather explicitly catch exceptions we want to ignore, but OK.
> + ex.printStackTrace();
> + }
> +
> [..snip..]
> + try {
> + p = sb.start();
> + p.waitFor();
> + processErrorStream =
> StreamUtils.readStreamAsString(p.getErrorStream());
> + processStdOutStream =
> StreamUtils.readStreamAsString(p.getInputStream());
> + r = p.exitValue();
> + //System.err.println(processErrorStream);
> + //System.out.println(processStdOutStream);
Please remove these comments.
> + processErrorStream = processErrorStream.toLowerCase();
> + processStdOutStream = processStdOutStream.toLowerCase();
> + } catch (Exception ex) {;
[nit] Rather just catch any checked exceptions, but OK.
> + ex.printStackTrace();
> +
> + }
> + if (r == null) {
> + validationResult += "<span color=\"red\">" +
> Translator.R("CPJVMnotLaunched") + "</span>";
> + if (latestOne != JvmValidationResult.STATE.NOT_DIR) {
> + latestOne = JvmValidationResult.STATE.NOT_VALID_JDK;
> + }
> + return new JvmValidationResult(validationResult, latestOne);
> + }
> + if (r.intValue() != 0) {
> + validationResult += "<span color=\"red\">" +
> Translator.R("CPJVMnoSuccess") + "</span>";
> + if (latestOne != JvmValidationResult.STATE.NOT_DIR) {
> + latestOne = JvmValidationResult.STATE.NOT_VALID_JDK;
> + }
> + return new JvmValidationResult(validationResult, latestOne);
> + }
> + if (processErrorStream.contains("openjdk") ||
> processStdOutStream.contains("openjdk")) {
> + validationResult += "<span color=\"#00EE00\">" +
> Translator.R("CPJVMopenJdkFound") + "</span>";
> + return new JvmValidationResult(validationResult,
> JvmValidationResult.STATE.VALID_JDK);
> + }
> + if (processErrorStream.contains("ibm") ||
> processStdOutStream.contains("ibm")) {
> + validationResult += "<span color=\"green\">" +
> Translator.R("CPJVMibmFound") + "</span>";
> + if (latestOne != JvmValidationResult.STATE.NOT_DIR) {
> + latestOne = JvmValidationResult.STATE.NOT_VALID_JDK;
> + }
> + return new JvmValidationResult(validationResult, latestOne);
> + }
> + if (processErrorStream.contains("gij") ||
> processStdOutStream.contains("gij")) {
> + validationResult += "<span color=\"orange\">" +
> Translator.R("CPJVMgijFound") + "</span>";
> + if (latestOne != JvmValidationResult.STATE.NOT_DIR) {
> + latestOne = JvmValidationResult.STATE.NOT_VALID_JDK;
> + }
> + return new JvmValidationResult(validationResult, latestOne);
> + }
> + if (processErrorStream.contains("oracle") ||
> processStdOutStream.contains("oracle")
> + || processErrorStream.contains("java(tm)") ||
> processStdOutStream.contains("java(tm)")) {
> + validationResult += "<span color=\"green\">" +
> Translator.R("CPJVMoracleFound") + "</span>";
> + if (latestOne != JvmValidationResult.STATE.NOT_DIR) {
> + latestOne = JvmValidationResult.STATE.NOT_VALID_JDK;
> + }
> + return new JvmValidationResult(validationResult, latestOne);
> + }
> + validationResult += "<span color=\"orange\">" +
> Translator.R("CPJVMstrangeProcess") + "</span>";
> + return new JvmValidationResult(validationResult,
> JvmValidationResult.STATE.NOT_VALID_JDK);
> +
> + }
> +
> + private String resetValidationResult(final String value, String
> result, String headerKey) {
> + return "<html>" + Translator.R(headerKey) + ": <br />" +
> value + " <br />" + result + "<hr /></html>";
> + }
> }
> diff -r fbb6b3605538
> netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Thu Apr
> 04 11:21:04 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Thu Apr
> 04 12:09:43 2013 +0200
> @@ -310,6 +310,29 @@
> CPDebuggingDescription=Enable options here to help with debugging
> CPDesktopIntegrationDescription=Set whether or not to allow creation
> of desktop shortcut.
> CPJVMPluginArguments=Set JVM arguments for plugin.
> +CPJVMitwExec=Set JVM for icedtea-web - working best with OpenJDK
> +CPJVMitwExecValidation=Validate JVM for icedtea-web
> +CPJVMPluginSelectExec=Select JVM for icedtea-web
> +CPJVMnone=No validation result for
> +CPJVMvalidated=Validation result for
> +CPJVMvalueNotSet=Value is not set. Hardcoded JVM will be used.
> +CPJVMnotLaunched=Error, process was not launched, see console output
> for more info.
> +CPJVMnoSuccess=Error, process have not ended successfully, see output
> for details, but your java is not set correctly.
> +CPJVMopenJdkFound=Excellent, OpenJDK detected
> +CPJVMoracleFound=Great, Oracle java detected
> +CPJVMibmFound=Good, IBM java detected
> +CPJVMgijFound=Warning, gij detected
> +CPJVMstrangeProcess=Your path had an executable process, but it was
> not recognized. Verify the Java version in the console output.
> +CPJVMnotDir=Error, The path you chose is not a directory.
> +CPJVMisDir=Ok, the path you chose is a directory.
> +CPJVMnoJava=Error, the directory you chose does not contain bin/java.
> +CPJVMjava=Ok, the directory you chose contains bin/java.
> +CPJVMnoRtJar=Error, the directory you chose does not contain lib/rt.jar
> +CPJVMrtJar=Ok, the directory you chose contains lib/rt.jar.
> +CPJVMPluginAllowTTValidation=Allow type-time validation
> +CPJVMNotokMessage1=You have entered invalid JDK value <u>({0})</u>
> with following error message:
> +CPJVMNotokMessage2=With invalid JDK IcedTea-Web will probably not
> able to start.<br>You will have to modify or remove <u>{0}</u>
> property in your configuration file <u>{1}</u>. <br>You should try to
> search for OpenJDK in your system or be sure you know what you are doing.
> +
>
> # Control Panel - Buttons
> CPButAbout=About...
> diff -r fbb6b3605538 netx/net/sourceforge/jnlp/util/StreamUtils.java
> --- a/netx/net/sourceforge/jnlp/util/StreamUtils.java Thu Apr 04
> 11:21:04 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/util/StreamUtils.java Thu Apr 04
> 12:09:43 2013 +0200
> @@ -37,9 +37,11 @@
>
> package net.sourceforge.jnlp.util;
>
> +import java.io.BufferedReader;
> import java.io.Closeable;
> import java.io.IOException;
> import java.io.InputStream;
> +import java.io.InputStreamReader;
>
> public class StreamUtils {
>
> @@ -71,4 +73,21 @@
> }
> }
> }
> +
> +
> + public static String readStreamAsString(InputStream stream)
> throws IOException {
> + InputStreamReader is = new InputStreamReader(stream);
> + StringBuilder sb = new StringBuilder();
> + BufferedReader br = new BufferedReader(is);
> + while (true) {
> + String read = br.readLine();
> + if (read == null) {
> + break;
> + }
> + sb.append(read);
> +
> + }
> +
> + return sb.toString();
> + }
> }
Thanks for moving this method!
Nothing else stands out code-wise, thanks for sticking with it.
As far as I know, the bugs I mentioned above are related to
already-pushed patches, so they can (and should) be fixed separately.
To summarize before pushing, I view these as a must:
- Remove commented out println's
- Clarify the 'error message' that isn't an error in the image I showed.
Please explicitly mention that the warning concerns not choosing IcedTea.
And of course, consider the nits.
You may push without sending to list again, your call.
Happy hacking,
-Adam
More information about the distro-pkg-dev
mailing list