On Thu, 31 Oct 2024 05:52:07 GMT, Alexey Semenyuk <asemen...@openjdk.org> wrote:
>> Support `win.norestart` application property in jpackage .cfg file. If found
>> in the .cfg file and set to `true`, it forces the Windows app launcher not
>> to restart the launcher process.
>>
>> Collateral changes to the tests:
>> - Added `WindowsHelper.killProcess()` to kill process by PID;
>> - Added `WindowsHelper.findAppLauncherPID()` that will return PIDs of the
>> running app launcher processes for the given app launcher name. The
>> implementation grabbed from `Win8301247Test` test;
>> - Rework `CfgFile` to make it R/W and capable of writing multiple instances
>> of the same key;
>> - Added `JPackageCommand.ignoreFakeRuntime()` to ignore external runtime if
>> it is a stub. Should speed up local test runs.
>
> Alexey Semenyuk has updated the pull request incrementally with four
> additional commits since the last revision:
>
> - Better log message
> - Rework findAppLauncherPID() into findAndKillAppLauncherProcess() that will
> kill detected running app launcher processes regardless if their number is as
> expected or not.
> - A comment added
> - Fix copyright year
Looks good overall with some additional comments.
src/jdk.jpackage/share/native/applauncher/AppLauncher.cpp line 41:
> 39: launcherPath = SysInfo::getProcessModulePath();
> 40: args = SysInfo::getCommandArgs();
> 41: externalCfgFile = 0;
I would prefer `NULL` instead of `0`.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/WindowsHelper.java line 235:
> 233:
> 234: public static void killProcess(long pid) {
> 235: Executor.of("taskkill", "/F", "/PID",
> Long.toString(pid)).dumpOutput(true).execute();
In case of two processes (parent and child) it will terminate only parent.
Based on `taskkill /?` `/T` needs to be specified to terminate child process as
well or call it for both processes.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/WindowsHelper.java line 311:
> 309: default -> {
> 310: TKit.assertUnexpected(String.format(
> 311: "Unexpected numer of running processes [%d]",
`numer` -> `number`
-------------
PR Review: https://git.openjdk.org/jdk/pull/21726#pullrequestreview-2409202951
PR Review Comment: https://git.openjdk.org/jdk/pull/21726#discussion_r1825242220
PR Review Comment: https://git.openjdk.org/jdk/pull/21726#discussion_r1825248460
PR Review Comment: https://git.openjdk.org/jdk/pull/21726#discussion_r1825247266
