On Tue, 13 Apr 2021 19:48:24 GMT, Alexey Semenyuk <asemen...@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).
>
> 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.

That seems like overkill.  walkFileTree must call visitFile, preVisitDirectory, 
and postVisitDirectory synchronously, because their return value tells 
walkFileTree where to go next.

> 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.

That makes sense, should this be here in JPackageCommand.CreateExecutor() or in 
Executor.setTempDir() (maybe renamed setWinTempDir ?)

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

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

Reply via email to