Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
On 19/02/2018 14:02, Michal Vala wrote: 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. This looks okay to me. I've created JDK-8198380 to track/sponsor this. -Alan
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&page=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: 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 : > > 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
2018/2/11 23:10:46 -0800, Michal Vala : > 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. 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
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
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
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. -Alan
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
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. Mandy
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
On 9 February 2018 at 15:38, Alan Bateman 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 http://cr.openjdk.java.net/~shade/8170114/webrev.01/ FWIW, it looks like a pretty straight-forward fix to me. -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Web Site: http://fuseyism.com Twitter: https://twitter.com/gnu_andrew_java PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Re: RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory
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
