Re: JDK-8198405 - JImageExtractTest.java & JImageListTest.java failed in Windows
On 02/21/2018 02:44 PM, Alan Bateman wrote: On 21/02/2018 12:23, Michal Vala wrote: Hi, I'm trying to investigate failure of JImageExtractTest on Windows. Failing method is testExtractToReadOnlyDir, issue is simple. It is trying to extract jimage to read-only directory and it must fail. AFAIK it is not possible to create read-only directory. I tried set rights programaticaly from java, manually from windows explorer and windows cmd. Without any success. It effectively just deny access from explorer and cmd. Is there any Windows expert who can confirm this or suggest solution? Only solution I've found is to write something to some mounted ISO image, which is not an option here. In case this will be confirmed I suggest to skip this single test on Windows. I don't have time to detail this just now but you can specify a DACL (as a FileAttribute<List>) to Files.createDirectory so that the directory is created with an initial DACL that denies access. Alternatively, you can change the DACL of an existing directory. Yes, a bit complicated to setup. I don't think it would be too much of a surprise for this sub-test to only execute on platforms that support POSIX file permissions (I think some of the tests check this already). Yes, I've tried this, but I'm still able to create a file in that directory. It prevents just from accessing the directory from explorer or cmd. However, I can freely access it from cygwin or java. -- Michal Vala OpenJDK QE Red Hat Czech
JDK-8198405 - JImageExtractTest.java & JImageListTest.java failed in Windows
Hi, I'm trying to investigate failure of JImageExtractTest on Windows. Failing method is testExtractToReadOnlyDir, issue is simple. It is trying to extract jimage to read-only directory and it must fail. AFAIK it is not possible to create read-only directory. I tried set rights programaticaly from java, manually from windows explorer and windows cmd. Without any success. It effectively just deny access from explorer and cmd. Is there any Windows expert who can confirm this or suggest solution? Only solution I've found is to write something to some mounted ISO image, which is not an option here. In case this will be confirmed I suggest to skip this single test on Windows. -- Michal Vala OpenJDK QE Red Hat Czech
Re: RFR - JImageListTest test fix
On 02/20/2018 10:47 AM, Alan Bateman wrote: On 19/02/2018 14:51, Michal Vala wrote: Hi, sending patch fixing JImageListTest failing tests. It was in ProblemList and this fix greens the test case, so the patch also removes it from the list. These tests are failing on Windows now. Nothing to do with your changes, it seems the original tests added via JDK-8167240 weren't run enough to shake out issues with the tests. So we need to re-add them to the exclude list, Sundar has already reviewed. ok, I can take a look at them -Alan diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt +++ b/test/jdk/ProblemList.txt @@ -464,6 +464,9 @@ tools/launcher/FXLauncherTest.java 8068049 linux-all,macosx-all +tools/jimage/JImageExtractTest.java 8198405 windows-all +tools/jimage/JImageListTest.java 8198405 windows-all + -- Michal Vala OpenJDK QE Red Hat Czech
RFR - JImageListTest test fix
Hi,
sending patch fixing JImageListTest failing tests. It was in ProblemList and
this fix greens the test case, so the patch also removes it from the list.
It will probably need new bug id and I will need a sponsor for this.
Fix itself is simple. There was incorrect assert for list of images contains
java.base image as it was testing index of "java.base" > 0 and it's usually at
first place, thus index 0. I've changed it to List#contains.
Thanks!
--
Michal Vala
OpenJDK QE
Red Hat Czech
--- old/test/jdk/ProblemList.txt 2018-02-19 15:40:56.429753232 +0100
+++ new/test/jdk/ProblemList.txt 2018-02-19 15:40:56.215752710 +0100
@@ -465,7 +465,6 @@
tools/launcher/FXLauncherTest.java 8068049 linux-all,macosx-all
tools/jimage/JImageExtractTest.java 8170120 generic-all
-tools/jimage/JImageListTest.java8170120 generic-all
--- old/test/jdk/tools/jimage/JImageListTest.java 2018-02-19 15:40:56.869754306 +0100
+++ new/test/jdk/tools/jimage/JImageListTest.java 2018-02-19 15:40:56.657753788 +0100
@@ -57,7 +57,7 @@
.map(s -> s.substring(s.indexOf(':') + 1).trim())
.collect(Collectors.toList());
assertTrue(modules.size() > 0, "Image contains at least one module.");
-assertTrue(modules.indexOf("java.base") > 0, "Module java.base found.");
+assertTrue(modules.contains("java.base"), "Module java.base found.");
});
}
@@ -88,7 +88,7 @@
.map(s -> s.substring(s.indexOf(':') + 1).trim())
.collect(Collectors.toList());
assertTrue(modules.size() > 0, "Image contains at least one module.");
-assertTrue(modules.indexOf("java.base") > 0, "Module java.base found.");
+assertTrue(modules.contains("java.base"), "Module java.base found.");
Set entries = Stream.of(lines)
.filter(s -> { return !s.startsWith("Module: ") && !s.startsWith("Offset"); })
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
Hi,
On 02/09/2018 04:23 PM, Michal Vala wrote:
Hi,
sending fix for jimage bug JDK-8170114[1].
as this was resolved with current behavior is correct[1], I've updated test case
according to this behavior.
Patch attached. This also greens JImageExtractTest so I've removed it from
ProblemList.
[1] -
https://bugs.openjdk.java.net/browse/JDK-8170114?focusedCommentId=14157816=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14157816
--
Michal Vala
OpenJDK QE
Red Hat Czech
--- old/test/jdk/ProblemList.txt 2018-02-19 14:59:41.645575388 +0100
+++ new/test/jdk/ProblemList.txt 2018-02-19 14:59:41.342574590 +0100
@@ -464,7 +464,6 @@
tools/launcher/FXLauncherTest.java 8068049 linux-all,macosx-all
-tools/jimage/JImageExtractTest.java 8170120 generic-all
tools/jimage/JImageListTest.java8170120 generic-all
--- old/test/jdk/tools/jimage/JImageExtractTest.java 2018-02-19 14:59:42.387577342 +0100
+++ new/test/jdk/tools/jimage/JImageExtractTest.java 2018-02-19 14:59:42.067576499 +0100
@@ -154,8 +154,10 @@
Path tmp = Files.createTempDirectory(Paths.get("."), getClass().getName());
Files.createFile(Paths.get(tmp.toString(), ".not_empty"));
jimage("extract", "--dir", tmp.toString(), getImagePath())
-.assertFailure()
-.assertShowsError();
+.assertSuccess()
+.resultChecker(r -> {
+assertTrue(r.output.isEmpty(), "Output is not expected");
+});
}
public void testExtractToFile() throws IOException {
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 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 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 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
JDK-8170120 jimage IOException solution?
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. Opinions or other ideas? [1] - https://bugs.openjdk.java.net/browse/JDK-8170120 -- Michal Vala OpenJDK QE Red Hat Czech
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
On Mon, 2018-02-12 at 09:39 -0800, mark.reinh...@oracle.com wrote: > 2018/2/11 23:10:46 -0800, Michal Vala <mv...@redhat.com>: > > On 02/10/2018 11:25 AM, Alan Bateman wrote: > > > ... > > > > > > I think the `jimage extract --dir ` > > > scenario needs > > > discussion. If is a non-directory file then > > > jimage has to fail, > > > I don't expect disagreement on that. For the case where it is an > > > existing > > > directory then the options seem to be: > > > > > > 1. Extract into the existing directory (existing JDK 9 and JDK 10 > > > behavior) > > > 2. Fail if it's not empty (your patch) > > > 3. Fail if it exists (Mandy's mail, the motivation being to keep > > > it consistent > > > with jlink) > > > > > > I view `jimage extract --dir ` as being similar to `unzip -d > > > ` so I > > > don't think existing behavior (#1) is incorrect, the only issue > > > is that it > > > silently overrides files whereas unzip will prompt before > > > overriding (unless you > > > specify -o). The `jar` tool, and legacy `tar` tool side with > > > `jimage` and are > > > happy to silently replace existing files. > > > > > > What would you think about focusing on the override case instead > > > of disallowing > > > extracting into an existing non-empty directory? I realize this > > > is more work as > > > it means deciding on whether to prompt, warn or fail. It also > > > means thinking > > > about the equivalent of unzip -o to allowing existing files be > > > replaced. > > > > Unzip prompts user for individual files and I'm not sure whether > > it's a good > > option here. Maybe prompt user at the beginning that directory is > > not empty and > > there is a risk that files there might be overwritten continue y/n? > > CLI tools that prompt the user are difficult to use in scripts, > so I advise against that. Usability in scripts is a valid point, but I still think that silently overwriting files is a bad practice. I can imagine better behavior without prompting the user. Like error when non-empty dir and 'force- overwrite' parameter. Or at least print a warning message that some files were overwritten. > > jimage is a diagnostic tool meant for rare and specialized use. > I think its current behavior (extract into an existing directory) > is fine, and I can imagine use cases where that might actually be > desired. > > - Mark -- Michal Vala OpenJDK QE Red Hat Czech
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
On 02/10/2018 11:25 AM, Alan Bateman wrote: On 10/02/2018 00:04, mandy chung wrote: On 2/9/18 7:23 AM, Michal Vala wrote: Patch validates output directory before any jimage extracting happen. I've moved validation to extra private method as it is few lines of code. I've also added proper error message for case when output path is not a directory (JImageTask.java#449). Thanks for looking at JDK-8170114 and JDK-8170120. I took a look at http://cr.openjdk.java.net/~shade/8170114/webrev.01/ Yes, thanks for looking at these issues. For `jimage info ` issue then jimage correctly fails (with a non-zero status) but the IOException isn't converted into an error message as it does with other errors. Yes, it would be good to fix that. I think the `jimage extract --dir ` scenario needs discussion. If is a non-directory file then jimage has to fail, I don't expect disagreement on that. For the case where it is an existing directory then the options seem to be: 1. Extract into the existing directory (existing JDK 9 and JDK 10 behavior) 2. Fail if it's not empty (your patch) 3. Fail if it exists (Mandy's mail, the motivation being to keep it consistent with jlink) I view `jimage extract --dir ` as being similar to `unzip -d ` so I don't think existing behavior (#1) is incorrect, the only issue is that it silently overrides files whereas unzip will prompt before overriding (unless you specify -o). The `jar` tool, and legacy `tar` tool side with `jimage` and are happy to silently replace existing files. What would you think about focusing on the override case instead of disallowing extracting into an existing non-empty directory? I realize this is more work as it means deciding on whether to prompt, warn or fail. It also means thinking about the equivalent of unzip -o to allowing existing files be replaced. Unzip prompts user for individual files and I'm not sure whether it's a good option here. Maybe prompt user at the beginning that directory is not empty and there is a risk that files there might be overwritten continue y/n? -- Michal Vala OpenJDK QE Red Hat Czech
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
On 02/10/2018 01:04 AM, mandy chung wrote: On 2/9/18 7:23 AM, Michal Vala wrote: Patch validates output directory before any jimage extracting happen. I've moved validation to extra private method as it is few lines of code. I've also added proper error message for case when output path is not a directory (JImageTask.java#449). Thanks for looking at JDK-8170114 and JDK-8170120. I took a look at http://cr.openjdk.java.net/~shade/8170114/webrev.01/ Alternatively, jimage extract can behave as jlink and it fails if the specified output directory exists including empty directory. It'd be easy to delete the directory in the command-line before running jimage. You extend JImageCliTest to handle the beforeTest method to be invoked before running each test case. Is that necessary? The test itself is creating temp file/directory for each test case. I think it'd be good to update the test to create a named file/dir under the scratch area as jtreg will take care of cleaning the scratch area. It's because of extract tests without specifying --dir (extract to cwd). AFAIK you can't correctly change cwd in runtime, so calling a test method in context of different cwd is not an option. Thus we need an empty scratch area for those. Alternatively we can clean cwd explicitly just before these tests, instead of before each test method. Mandy -- Michal Vala OpenJDK QE Red Hat Czech
Fwd: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
ah, accidentally replied just to Alan :/ Here it is -- Forwarded message -- From: Michal Vala <mv...@redhat.com> Date: Fri, Feb 9, 2018 at 6:21 PM Subject: Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory To: Alan Bateman <alan.bate...@oracle.com> sure, here it is http://cr.openjdk.java.net/~shade/8170114/webrev.01/ On Fri, Feb 9, 2018 at 4:38 PM, Alan Bateman <alan.bate...@oracle.com> wrote: > On 09/02/2018 15:23, Michal Vala wrote: >> >> Hi, >> >> sending fix for jimage bug JDK-8170114[1]. I'm not sure whether this is >> correct list. If it is not, please direct me somewhere else. >> >> I don't have an openjdk account, so webrev is on my fedora public space. I >> will need a sponsor for this. >> >> webrev: >> https://michalvala.fedorapeople.org/webrevs/JDK-8170114/webrev.00/webrev/ > > Do you have any buddies in Java team in Red Hat that you publish this on > cr.openjdk.java.net? We can't look at/accept contributions that are > published on non-OpenJDK infrastructure. If you don't then can you include > the patch in a mail. > > -Alan
RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
Hi, sending fix for jimage bug JDK-8170114[1]. I'm not sure whether this is correct list. If it is not, please direct me somewhere else. I don't have an openjdk account, so webrev is on my fedora public space. I will need a sponsor for this. webrev: https://michalvala.fedorapeople.org/webrevs/JDK-8170114/webrev.00/webrev/ Patch validates output directory before any jimage extracting happen. I've moved validation to extra private method as it is few lines of code. I've also added proper error message for case when output path is not a directory (JImageTask.java#449). I've implemented beforeTest method, that cleans cwd. This must be done because files were extracted to cwd and constantly overwriting and tests were non-deterministic (order did matter). Plus some tests added. I want to fix JDK-8170120 with IOException when file is not an jimage. It is blocked by this. This will fix 3 now problematic and excluded tests. [1] - https://bugs.openjdk.java.net/browse/JDK-8170114 -- Michal Vala OpenJDK QE Red Hat Czech
