8287971: Throw exception for missing values in .jpackage.xml #9

sashamatveev · 2022-06-13T17:01:48Z

Error will be thrown if app image is generated with another JDK version or has malformed .jpackage.xml.
Re-fixed as Alexey suggested in 8287971: Throw exception for missing values in .jpackage.xml jdk#9098.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8287971: Throw exception for missing values in .jpackage.xml

Reviewers

Alexey Semenyuk (@alexeysemenyukoracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk19 pull/9/head:pull/9
$ git checkout pull/9

Update a local copy of the PR:
$ git checkout pull/9
$ git pull https://git.openjdk.org/jdk19 pull/9/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9

View PR using the GUI difftool:
$ git pr show -t 9

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/9.diff

bridgekeeper · 2022-06-13T17:02:08Z

👋 Welcome back almatvee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-06-13T17:04:15Z

@sashamatveev The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-06-13T17:08:41Z

Webrevs

alexeysemenyukoracle · 2022-06-14T18:07:12Z

src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageFile.java

-    private final boolean signed;
-    private final boolean appStore;
+    private final String signedStr;
+    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.

alexeysemenyukoracle · 2022-06-14T18:12:30Z

src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/MainResources_de.properties

-warning.foreign-app-image=Warnung: app-image-Verzeichnis ({0}) wurde von jpackage nicht generiert.
-warning.invalid-app-image=Warnung: .jpackage.xml kann in app-image-Verzeichnis ({0}) nicht geparst werden
+
+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})."

alexeysemenyukoracle · 2022-06-14T18:26:35Z

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/TKit.java

+    public static void createJPackageXMLFile(Path appImageDir,
+            String mainLauncher, String mainClass) throws IOException {
+        String version = System.getProperty("java.version");
+        String platform = "";
+        Path appDir = appImageDir;
+        if (TKit.isWindows()) {
+            platform = "windows";
+            appDir = Files.createDirectories(
+                         appImageDir.resolve("app"));
+        } else if (TKit.isLinux()) {
+            platform = "linux";
+            appDir = Files.createDirectories(
+                         appImageDir.resolve("lib").resolve("app"));
+        } else if (TKit.isOSX()) {
+            platform = "macOS";
+            appDir = Files.createDirectories(
+                         appImageDir.resolve("Contents").resolve("app"));
+        }
+
+        TKit.createTextFile(appDir.resolve(".jpackage.xml"), List.of(
+                            "<?xml version=\"1.0\" encoding=\"utf-8\"?>",
+                            String.format(
+                                "<jpackage-state platform=\"%s\" version=\"%s\">",
+                                platform, version),
+                            "<app-version>1.0</app-version>",
+                            String.format(
+                                "<main-launcher>%s</main-launcher>",
+                                mainLauncher),
+                            String.format(
+                                "<main-class>%s</main-class>",
+                                mainClass),
+                            "<signed>false</signed>",
+                            "<app-store>false</app-store>",
+                            "</jpackage-state>"
+            ));
+    }
+


This is jpackage specific, so it should belong to JPackageCommand class.

Please use proper XML API to create XML output.

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

Right! My bad, sorry for the confusion.

alexeysemenyukoracle · 2022-06-14T18:32:12Z

src/jdk.jpackage/share/classes/jdk/jpackage/internal/AppImageFile.java

    private boolean isValid() {
+        if (!Objects.equals(getVersion(), creatorVersion)) {
+            return false;
+        }
+
+        if (!Objects.equals(getPlatform(), creatorPlatform)) {
+            return false;
+        }
+
        if (launcherName == null || launcherName.length() == 0) {
            return false;
        }

+        if (mainClass == null || mainClass.length() == 0) {
+            return false;
+        }
+
        for (var launcher : addLauncherInfos) {
            if ("".equals(launcher.getName())) {
                return false;
            }
        }

-        if (!Objects.equals(getVersion(), creatorVersion)) {
+        if (signedStr == null ||
+                !("true".equals(signedStr) || "false".equals(signedStr))) {
            return false;
        }

-        if (!Objects.equals(getPlatform(), creatorPlatform)) {
+        if (appStoreStr == null ||
+                !("true".equals(appStoreStr) || "false".equals(appStoreStr))) {
            return false;
        }


It makes sense to unfold this function in the ctor as we don't allow empty AppImageFile instances.

alexeysemenyukoracle

alexeysemenyukoracle

No need to copy/paste xml authoring code from IOUtils. This API is accessible outside of jpackage. Check out https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/test/jdk/tools/jpackage/windows/WinScriptTest.java#L151

sashamatveev · 2022-06-17T04:04:35Z

8287971: Throw exception for missing values in .jpackage.xml [v3]

Removed copy/paste code from TKit.java.

alexeysemenyukoracle · 2022-06-17T17:01:10Z

test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java

+        String appImage = getArgumentValue("--app-image");
+        if (appImage == null) {
+            throw new RuntimeException("Error: --app-image expected");
+        }


Can be simplified to:

Path jpackageXMLFile = AppImageFile.getPathInAppImage( Optional.ofNullable(getArgumentValue("--app-image")).map( Path::of).orElseThrow(() -> { return new RuntimeException( "Error: --app-image expected"); }));

And you don't need copy/paste code from AppImageFile.getPathInAppImage() below.

Done. I also switched to use AppImageFile.getVersion() and AppImageFile.getPlatform().

openjdk · 2022-06-18T00:02:47Z

@sashamatveev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8287971: Throw exception for missing values in .jpackage.xml

Reviewed-by: asemenyuk

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 21 new commits pushed to the master branch:

f12d044: 8288692: jdk/javadoc/doclet/testTagMisuse/TestTagMisuse.java fails after JDK-8288545
97544be: 8268398: 15% increase in JFR footprint in Noop-Base
983f75c: 8288545: Missing space in error message
53bf1bf: 8286176: Add JNI_VERSION_19 to jni.h and JNI spec
c254c9d: 8287401: jpackage tests failing on Windows due to powershell issue
ff3db52: 8288534: Out of bound errors for memory segment access mentions wrong values
729164f: 8288533: Missing @param tags in com.sun.source classes
9254e12: 8288526: ProblemList gc/stress/TestStressG1Humongous.java on windows-x64
395aea3: 8288414: Long::compress/expand samples are not correct
f3b1f60: 8288289: Preview APIs in jdk.jdi, jdk.management, and jdk.jfr should be reflective preview APIs
... and 11 more: https://git.openjdk.org/jdk19/compare/59b0de6bc7064b39cdc51517dee4f4d96af3efaf...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

sashamatveev · 2022-06-21T21:10:37Z

/integrate

openjdk · 2022-06-21T21:12:33Z

Going to push as commit 70008da.
Since your change was applied there have been 38 commits pushed to the master branch:

d7b43af: 8288761: SegmentAllocator:allocate(long bytesSize) not throwing IAEx when bytesSize < 0
834d92d: 8288754: GCC 12 fails to build zReferenceProcessor.cpp
198cec9: 8286103: VThreadMonitorTest fails "assert(!current->cont_fastpath() || (current->cont_fastpath_thread_state() && !interpreted_native_or_deoptimized_on_stack(current))) failed"
97200a7: 8278053: serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java failing in loom repo with Xcomp
31d981e: 8288532: additional review changes for JDK-8286830
a144988: 8288139: JavaThread touches oop after GC barrier is detached
e26d3b3: 8288497: add support for JavaThread::is_oop_safe()
c74a923: 8288531: Empty spans in mobile navigation markup
af05139: 8288467: remove memory_operand assert for spilled instructions
b9c3966: 8288671: Problematic fix for font boosting
... and 28 more: https://git.openjdk.org/jdk19/compare/59b0de6bc7064b39cdc51517dee4f4d96af3efaf...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-21T21:12:53Z

@sashamatveev Pushed as commit 70008da.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8287971: Throw exception for missing values in .jpackage.xml

daee617

sashamatveev mentioned this pull request Jun 13, 2022

8287971: Throw exception for missing values in .jpackage.xml openjdk/jdk#9098

Closed

3 tasks

openjdk bot added the rfr Pull request is ready for review label Jun 13, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 13, 2022

alexeysemenyukoracle reviewed Jun 14, 2022

View reviewed changes

alexeysemenyukoracle suggested changes Jun 14, 2022

View reviewed changes

8287971: Throw exception for missing values in .jpackage.xml [v2]

c06af06

alexeysemenyukoracle suggested changes Jun 16, 2022

View reviewed changes

8287971: Throw exception for missing values in .jpackage.xml [v3]

2199f4e

alexeysemenyukoracle suggested changes Jun 17, 2022

View reviewed changes

8287971: Throw exception for missing values in .jpackage.xml [v4]

d489ee6

alexeysemenyukoracle approved these changes Jun 18, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 18, 2022

openjdk bot added the integrated Pull request has been integrated label Jun 21, 2022

openjdk bot closed this Jun 21, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 21, 2022

sureshg mentioned this pull request Sep 17, 2022

Packaging fails on JDK-19 JetBrains/compose-multiplatform#2328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8287971: Throw exception for missing values in .jpackage.xml #9

8287971: Throw exception for missing values in .jpackage.xml #9

sashamatveev commented Jun 13, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jun 13, 2022

openjdk bot commented Jun 13, 2022

mlbridge bot commented Jun 13, 2022 •

edited

alexeysemenyukoracle Jun 14, 2022

sashamatveev Jun 16, 2022

alexeysemenyukoracle Jun 14, 2022

sashamatveev Jun 16, 2022

alexeysemenyukoracle Jun 14, 2022 •

edited

sashamatveev Jun 16, 2022

alexeysemenyukoracle Jun 16, 2022

alexeysemenyukoracle Jun 14, 2022

sashamatveev Jun 16, 2022

alexeysemenyukoracle left a comment

alexeysemenyukoracle left a comment

sashamatveev commented Jun 17, 2022

alexeysemenyukoracle Jun 17, 2022 •

edited

sashamatveev Jun 17, 2022

openjdk bot commented Jun 18, 2022 •

edited

sashamatveev commented Jun 21, 2022

openjdk bot commented Jun 21, 2022

openjdk bot commented Jun 21, 2022

8287971: Throw exception for missing values in .jpackage.xml #9

8287971: Throw exception for missing values in .jpackage.xml #9

Conversation

sashamatveev commented Jun 13, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 13, 2022

openjdk bot commented Jun 13, 2022

mlbridge bot commented Jun 13, 2022 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeysemenyukoracle Jun 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeysemenyukoracle left a comment

Choose a reason for hiding this comment

alexeysemenyukoracle left a comment

Choose a reason for hiding this comment

sashamatveev commented Jun 17, 2022

alexeysemenyukoracle Jun 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jun 18, 2022 • edited

sashamatveev commented Jun 21, 2022

openjdk bot commented Jun 21, 2022

openjdk bot commented Jun 21, 2022

sashamatveev commented Jun 13, 2022 •

edited by openjdk bot

mlbridge bot commented Jun 13, 2022 •

edited

alexeysemenyukoracle Jun 14, 2022 •

edited

alexeysemenyukoracle Jun 17, 2022 •

edited

openjdk bot commented Jun 18, 2022 •

edited