On Tue, 6 Jan 2026 22:32:33 GMT, Alexey Semenyuk <[email protected]> wrote:
>> - Move code shared between jpackage's Executor and jpackage's test lib >> Executor into `jdk.jpackage.internal.util.CommandOutputControl` class using >> the latter one as the baseline for the new class; [CommandOutputControl >> class >> javadoc](https://alexeysemenyukoracle.github.io/jpackage-javadoc/jdk.jpackage/jdk/jpackage/internal/util/CommandOutputControl.html). >> - Place "execute with retries" logic into >> `jdk.jpackage.internal.util.RetryExecutor` class and use it from both >> jpackage and jpackage's test lib. Use `jdk.jpackage.internal.RetryExecutor` >> class as the baseline. >> - Add `ObjectFactory`, `ExecutorFactory`, and `RetryExecutorFactory` >> interfaces to the "jdk.jpackage.internal" package. They enable the use of >> command mocks with jpackage. >> - Add `jdk.jpackage.test.mock` package. It facilitates creating command >> mocks. Use it to test LibProvidersLookup, LinuxSystemEnvironment, >> LinuxPackageArch, MacDmgSystemEnvironment, and MacDmgPackager classes. >> >> Supplementary changes: >> - Make `jdk.jpackage.internal.SystemEnvironment` and all implementing >> classes package private > > Alexey Semenyuk has updated the pull request incrementally with one > additional commit since the last revision: > > test/Executor: minor fix Looks good with some comments. src/jdk.jpackage/macosx/classes/jdk/jpackage/internal/MacDmgPackager.java line 274: > 272: .setMaxAttemptsCount(10) > 273: .setAttemptTimeoutMillis(3000) > 274: .setWriteOutputToFile(true) Do we always write output to file with new implementation? I see that you call `storeOutputInFiles()` for `hdiutil`. src/jdk.jpackage/share/classes/jdk/jpackage/internal/util/CommandOutputControl.java line 7: > 5: * This code is free software; you can redistribute it and/or modify it > 6: * under the terms of the GNU General Public License version 2 only, as > 7: * published by the Free Software Foundation. Missing "Classpath" exception. test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/util/CommandOutputControlTest.java line 129: > 127: var desc = spec.create().description(); > 128: assertFalse(desc.isBlank()); > 129: // System.out.println(desc); Remove if not needed. test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/util/CommandOutputControlTest.java line 698: > 696: private static List<OutputTestSpec> testSomeSavedOutput() { > 697: var testIds = List.<Integer>of(/* 10, 67, 456 */); > 698: if (testIds.isEmpty()) { Can you explain what this method do? `testIds` is always empty, so not sure what it tries to do. test/jdk/tools/jpackage/junit/share/jdk.jpackage/jdk/jpackage/internal/util/CommandOutputControlTest.java line 1003: > 1001: } > 1002: case CatCommandAction _ -> { > 1003: // Not used, no pint to implement. `pint` -> `point` ------------- PR Review: https://git.openjdk.org/jdk/pull/28733#pullrequestreview-3629203357 PR Review Comment: https://git.openjdk.org/jdk/pull/28733#discussion_r2663456818 PR Review Comment: https://git.openjdk.org/jdk/pull/28733#discussion_r2666734358 PR Review Comment: https://git.openjdk.org/jdk/pull/28733#discussion_r2666938204 PR Review Comment: https://git.openjdk.org/jdk/pull/28733#discussion_r2666955055 PR Review Comment: https://git.openjdk.org/jdk/pull/28733#discussion_r2666960201
