On Tue, 13 Apr 2021 18:57:20 GMT, Andy Herrick <herr...@openjdk.org> wrote:

> two changes:
> One to jpackage, when recursively removing directory, when IOException 
> occurs, record it and continue (deleting as much as possible) before throwing 
> the exception.
> The other to tests, when running jpackage via ProcessBuilder.execute(), set 
> the "TMP" environment variable to the current value of System Property 
> "java.io.tmpdir".  This causes the sub-process (jpackage) to output tmp files 
> to the tmp file location used by the test. (So the test harness can clean up 
> after test exits).

Changes requested by asemenyuk (Reviewer).

src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:

> 57: 
> 58:     public static void deleteRecursive(Path directory) throws IOException 
> {
> 59:         final IOException [] exception = { (IOException) null };

Do we know `Files.walkFileTree()` synchronizes calls on callback object? If 
not, I'd use `AtomicReference` to store the first exception.

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/Executor.java line 92:

> 90:     }
> 91: 
> 92:     public Executor setTmpDir(String tmp) {

As this would work only on Windows, I'd throw `IllegalStateException` if the 
method is called on other platform.

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java line 646:

> 644:         } else {
> 645:             exec.setExecutable(JavaTool.JPACKAGE);
> 646:             exec.setTmpDir(System.getProperty("java.io.tmpdir"));

This would work only on Windows. I'd put corresponding `if` around this 
statement to avoid future confusion.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3473

Reply via email to