On Tue, 14 Jun 2022 18:07:12 GMT, Alexey Semenyuk <asemen...@openjdk.org> wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8287971: Throw exception for missing values in .jpackage.xml [v2]
>
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageFile.java line
> 68:
>
>> 66: private final List<LauncherInfo> addLauncherInfos;
>> 67: private final String signedStr;
>> 68: private final String appStoreStr;
>
> What is the idea of this change?
To store string values, so it can be validated to make sure it is true or false
only. I changed it back, since I moved validation to ctor.
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageFile.java line
> 339:
>
>> 337: !("true".equals(appStoreStr) ||
>> "false".equals(appStoreStr))) {
>> 338: return false;
>> 339: }
>
> It makes sense to unfold this function in the ctor as we don't allow empty
> AppImageFile instances.
Done.
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/MainResources_de.properties
> line 79:
>
>> 77: warning.no.jdk.modules.found=Warnung: Keine JDK-Module gefunden
>> 78:
>> 79: error.foreign-app-image=Error: app-image dir ({0}) not generated by
>> jpackage. Missing .jpackage.xml.
>
> This error message will be misleading in case app image was generated by
> jpackage and adjusted after.
> I'd put it as "Missing .jpackage.xml file in app-image dir ({0})."
Done.
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/TKit.java line 272:
>
>> 270: ));
>> 271: }
>> 272:
>
> 1. This is jpackage specific, so it should belong to JPackageCommand class.
> 2. Please use proper XML API to create XML output.
> 3. Having `<app-version>1.0</app-version>` in .jpackage.xml file will make
> `AppImageFile.isValid()` return `false` and `AppImageFile.load()` to throw
> exception. This seems to be equivalent to not having .jpackage.xml file in
> app image at all raising a question if this new function is needed at all.
1 and 2 done. I think you mistaken `<app-version>1.0</app-version>` with
`<jpackage-state version="%s"`. We never read <app-version> and I do put
correct value for `<jpackage-state version="%s"`. It is required for several
tests which uses empty/dummy app images and since we started required
.jpackage.xml file we need it for empty app images as well.
-------------
PR: https://git.openjdk.org/jdk19/pull/9
