Re: JDK-8170120 jimage IOException solution?
On 02/19/2018 02:07 PM, Alan Bateman wrote:
On 19/02/2018 12:09, Michal Vala wrote:
Yes, true. Attached.
This looks good except the "does not exist" case which needs this change:
- throw TASK_HELPER.newBadArgs("err.not.a.jimage",
file.getName());
+ throw TASK_HELPER.newBadArgs("err.not.a.jimage", file);
If you agree then I will include it in your patch and push it.
yes please. thanks
--
Michal Vala
OpenJDK QE
Red Hat Czech
Re: JDK-8170120 jimage IOException solution?
On 19/02/2018 12:09, Michal Vala wrote:
Yes, true. Attached.
This looks good except the "does not exist" case which needs this change:
- throw TASK_HELPER.newBadArgs("err.not.a.jimage",
file.getName());
+ throw TASK_HELPER.newBadArgs("err.not.a.jimage", file);
If you agree then I will include it in your patch and push it.
-Alan
Re: JDK-8170120 jimage IOException solution?
On 02/17/2018 08:17 AM, Alan Bateman wrote:
On 16/02/2018 14:39, Michal Vala wrote:
:
That sounds reasonable. Here's the webrev:
http://cr.openjdk.java.net/~shade/8170120/webrev.01/
It fixes JImageVerifyTest so I've removed it from ProblemList.
note: JImageExtractTest still failing on not-empty-dir(JDK-8170114) and
JImageListTest fails on simplest list test. I'll take a look at it later.
The changes look good, I just wonder if it would be better to specify {0} as
file rather than file.getName(). Just thinking about a script running this tool
failing because the file path is wrong, I think it could be useful to have it in
error.
Yes, true. Attached.
-Alan
--
Michal Vala
OpenJDK QE
Red Hat Czech
--- old/src/jdk.jlink/share/classes/jdk/tools/jimage/JImageTask.java 2018-02-19 12:59:49.727985281 +0100
+++ new/src/jdk.jlink/share/classes/jdk/tools/jimage/JImageTask.java 2018-02-19 12:59:49.440984670 +0100
@@ -431,6 +431,8 @@
}
}
}
+} catch (IOException ioe) {
+throw TASK_HELPER.newBadArgs("err.invalid.jimage", file, ioe.getMessage());
}
}
}
--- old/src/jdk.jlink/share/classes/jdk/tools/jimage/resources/jimage.properties 2018-02-19 12:59:50.279986455 +0100
+++ new/src/jdk.jlink/share/classes/jdk/tools/jimage/resources/jimage.properties 2018-02-19 12:59:50.018985900 +0100
@@ -95,6 +95,7 @@
err.missing.arg=no value given for {0}
err.not.a.dir=not a directory: {0}
err.not.a.jimage=not a jimage file: {0}
+err.invalid.jimage=Unable to open {0}: {1}
err.no.jimage=no jimage provided
err.option.unsupported={0} not supported: {1}
err.unknown.option=unknown option: {0}
--- old/test/jdk/ProblemList.txt 2018-02-19 12:59:50.813987591 +0100
+++ new/test/jdk/ProblemList.txt 2018-02-19 12:59:50.561987055 +0100
@@ -466,7 +466,6 @@
tools/jimage/JImageExtractTest.java 8170120 generic-all
tools/jimage/JImageListTest.java8170120 generic-all
-tools/jimage/JImageVerifyTest.java 8170120 generic-all
Re: JDK-8170120 jimage IOException solution?
On 16/02/2018 14:39, Michal Vala wrote:
:
That sounds reasonable. Here's the webrev:
http://cr.openjdk.java.net/~shade/8170120/webrev.01/
It fixes JImageVerifyTest so I've removed it from ProblemList.
note: JImageExtractTest still failing on not-empty-dir(JDK-8170114)
and JImageListTest fails on simplest list test. I'll take a look at it
later.
The changes look good, I just wonder if it would be better to specify
{0} as file rather than file.getName(). Just thinking about a script
running this tool failing because the file path is wrong, I think it
could be useful to have it in error.
-Alan
Re: JDK-8170120 jimage IOException solution?
On 02/16/2018 02:45 PM, Alan Bateman wrote:
On 16/02/2018 13:35, Michal Vala wrote:
:
Sure I can do that. However, all output messages are defined at
jimage.properties (jdk.jlink module) file and this won't be possible for this
case. I can't tell whether IOExcaption is caused by wrong jimage file or
something else so all I can do is really just print an exception message.
The jimage tool is for troubleshooting purposes, it's not a tool that we expect
many developers to every use directly. It should be okay if the resource is
something like "Unable to open {0}: {1}" where {0} is the file path specified to
the tool, and {1} is the exception message.
That sounds reasonable. Here's the webrev:
http://cr.openjdk.java.net/~shade/8170120/webrev.01/
It fixes JImageVerifyTest so I've removed it from ProblemList.
note: JImageExtractTest still failing on not-empty-dir(JDK-8170114) and
JImageListTest fails on simplest list test. I'll take a look at it later.
--
Michal Vala
OpenJDK QE
Red Hat Czech
Re: JDK-8170120 jimage IOException solution?
On 16/02/2018 13:35, Michal Vala wrote:
:
Sure I can do that. However, all output messages are defined at
jimage.properties (jdk.jlink module) file and this won't be possible
for this case. I can't tell whether IOExcaption is caused by wrong
jimage file or something else so all I can do is really just print an
exception message.
The jimage tool is for troubleshooting purposes, it's not a tool that we
expect many developers to every use directly. It should be okay if the
resource is something like "Unable to open {0}: {1}" where {0} is the
file path specified to the tool, and {1} is the exception message.
-Alan
Re: JDK-8170120 jimage IOException solution?
On 02/16/2018 02:17 PM, Alan Bateman wrote: On 16/02/2018 13:03, Michal Vala wrote: Hi, I'm working on JDK-8170120[1]. I have 2 working solutions, but I'm not happy with neither one. That IOException is thrown from jdk.internal.jimage.BasicImageReader, which is in java.base module. Jimage is implemented in jdk.jlink module (jdk.tools.jimage.JImageTask). One solution is new exception NotValidJimageException which can be thrown from BasicImageReader and catch and handled at JImageTask. Then it's easy to return proper error message to the output. Issue is that this new exception has to be public in java.base so de-facto defining new jdk core api, which of course I don't want to. Next option is leave there IOException, but give it some known message and then handle this message in JImageTask. This work also well, but "parsing" some message from exception is a bit clumsy. Why can't you just catch the IOException and adds the exception message to the error message that jimage prints? Sure I can do that. However, all output messages are defined at jimage.properties (jdk.jlink module) file and this won't be possible for this case. I can't tell whether IOExcaption is caused by wrong jimage file or something else so all I can do is really just print an exception message. It also affects behavior of other cases that throws IOException (not sure if bad or not) that are now catch with Exception and return EXIT_ABNORMAL error code. So it sounds like another 'wrong' solution to me. Question is what solution is least wrong and what is desired behavior. I wanted to fix that one specific case with minimal side-effects. -- Michal Vala OpenJDK QE Red Hat Czech
