Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.
/ jdk19 Public archive

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,9 @@ protected void validateAppImageAndBundeler(
SIGN_BUNDLE.fetchFrom(params)).orElse(Boolean.FALSE)) {
// if signing bundle with app-image, warn user if app-image
// is not already signed.
try {
if (!(AppImageFile.load(applicationImage).isSigned())) {
Log.info(MessageFormat.format(I18N.getString(
"warning.unsigned.app.image"), getID()));
}
} catch (IOException ioe) {
// Ignore - In case of a forign or tampered with app-image,
// user is notified of this when the name is extracted.
if (!(AppImageFile.load(applicationImage).isSigned())) {
Log.info(MessageFormat.format(I18N.getString(
"warning.unsigned.app.image"), getID()));
}
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,15 @@ public final class AppImageFile {
private final String launcherName;
private final String mainClass;
private final List<LauncherInfo> addLauncherInfos;
private final boolean signed;
private final boolean appStore;
private final String signedStr;
private final String appStoreStr;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the idea of this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private static final String FILENAME = ".jpackage.xml";

private static final Map<Platform, String> PLATFORM_LABELS = Map.of(
Platform.LINUX, "linux", Platform.WINDOWS, "windows", Platform.MAC,
"macOS");


private AppImageFile() {
this(null, null, null, null, null, null, null);
}

private AppImageFile(String launcherName, String mainClass,
List<LauncherInfo> launcherInfos, String creatorVersion,
String creatorPlatform, String signedStr, String appStoreStr) {
Expand All @@ -86,8 +81,8 @@ private AppImageFile(String launcherName, String mainClass,
this.addLauncherInfos = launcherInfos;
this.creatorVersion = creatorVersion;
this.creatorPlatform = creatorPlatform;
this.signed = "true".equals(signedStr);
this.appStore = "true".equals(appStoreStr);
this.signedStr = signedStr;
this.appStoreStr = appStoreStr;
}

/**
Expand All @@ -114,15 +109,11 @@ String getMainClass() {
}

boolean isSigned() {
return signed;
return "true".equals(signedStr);
}

boolean isAppStore() {
return appStore;
}

void verifyCompatible() throws ConfigException {
// Just do nothing for now.
return "true".equals(appStoreStr);
}

/**
Expand Down Expand Up @@ -189,25 +180,17 @@ static void save(Path appImageDir, Map<String, Object> params)
* @return valid info about application image or null
* @throws IOException
*/
static AppImageFile load(Path appImageDir) throws IOException {
static AppImageFile load(Path appImageDir) {
try {
Document doc = readXml(appImageDir);

XPath xPath = XPathFactory.newInstance().newXPath();

String mainLauncher = xpathQueryNullable(xPath,
"/jpackage-state/main-launcher/text()", doc);
if (mainLauncher == null) {
// No main launcher, this is fatal.
return new AppImageFile();
}

String mainClass = xpathQueryNullable(xPath,
"/jpackage-state/main-class/text()", doc);
if (mainClass == null) {
// No main class, this is fatal.
return new AppImageFile();
}

List<LauncherInfo> launcherInfos = new ArrayList<>();

Expand All @@ -234,12 +217,19 @@ static AppImageFile load(Path appImageDir) throws IOException {
AppImageFile file = new AppImageFile(mainLauncher, mainClass,
launcherInfos, version, platform, signedStr, appStoreStr);
if (!file.isValid()) {
file = new AppImageFile();
throw new RuntimeException(MessageFormat.format(I18N.getString(
"error.invalid-app-image"), appImageDir));
}
return file;
} catch (XPathExpressionException ex) {
// This should never happen as XPath expressions should be correct
throw new RuntimeException(ex);
} catch (NoSuchFileException nsfe) {
// non jpackage generated app-image (no app/.jpackage.xml)
throw new RuntimeException(MessageFormat.format(I18N.getString(
"error.foreign-app-image"), appImageDir));
} catch (IOException ioe) {
throw new RuntimeException(ioe);
}
}

Expand Down Expand Up @@ -275,23 +265,11 @@ static List<LauncherInfo> getLaunchers(Path appImageDir,
Map<String, Object> params) {
List<LauncherInfo> launchers = new ArrayList<>();
if (appImageDir != null) {
try {
AppImageFile appImageInfo = AppImageFile.load(appImageDir);
if (appImageInfo != null) {
launchers.add(new LauncherInfo(
appImageInfo.getLauncherName(), params));
AppImageFile appImageInfo = AppImageFile.load(appImageDir);
launchers.add(new LauncherInfo(
appImageInfo.getLauncherName(), params));
launchers.addAll(appImageInfo.getAddLaunchers());
return launchers;
}
} catch (NoSuchFileException nsfe) {
// non jpackage generated app-image (no app/.jpackage.xml)
Log.info(MessageFormat.format(I18N.getString(
"warning.foreign-app-image"), appImageDir));
} catch (IOException ioe) {
Log.verbose(ioe);
Log.info(MessageFormat.format(I18N.getString(
"warning.invalid-app-image"), appImageDir));
}
return launchers;
}

launchers.add(new LauncherInfo(params));
Expand All @@ -302,23 +280,11 @@ static List<LauncherInfo> getLaunchers(Path appImageDir,
}

public static String extractAppName(Path appImageDir) {
try {
return AppImageFile.load(appImageDir).getLauncherName();
} catch (IOException ioe) {
Log.verbose(MessageFormat.format(I18N.getString(
"warning.foreign-app-image"), appImageDir));
return null;
}
return AppImageFile.load(appImageDir).getLauncherName();
}

public static String extractMainClass(Path appImageDir) {
try {
return AppImageFile.load(appImageDir).getMainClass();
} catch (IOException ioe) {
Log.verbose(MessageFormat.format(I18N.getString(
"warning.foreign-app-image"), appImageDir));
return null;
}
return AppImageFile.load(appImageDir).getMainClass();
}

private static String xpathQueryNullable(XPath xPath, String xpathExpr,
Expand All @@ -332,29 +298,43 @@ private static String xpathQueryNullable(XPath xPath, String xpathExpr,
}

static String getVersion() {
return "1.0";
return System.getProperty("java.version");
}

static String getPlatform() {
return PLATFORM_LABELS.get(Platform.getPlatform());
}

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;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,12 +531,8 @@ private static Path findPathOfModule( List<Path> modulePath, String moduleName)
Boolean.class,
params -> {
if (hasPredefinedAppImage(params)) {
try {
return AppImageFile.load(getPredefinedAppImage(params))
.isAppStore();
} catch (IOException ex) {
return false;
}
return AppImageFile.load(getPredefinedAppImage(params))
.isAppStore();
}
return false;
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ error.blocked.option=jlink option [{0}] is not permitted in --jlink-options
error.no.name=Name not specified with --name and cannot infer one from app-image

warning.no.jdk.modules.found=Warning: No JDK Modules found
warning.foreign-app-image=Warning: app-image dir ({0}) not generated by jpackage.
warning.invalid-app-image=Warning: cannot parse .jpackage.xml in app-image dir ({0})

error.foreign-app-image=Error: app-image dir ({0}) not generated by jpackage. Missing .jpackage.xml.
error.invalid-app-image=Error: app-image dir ({0}) generated by another jpackage version or malformed .jpackage.xml

MSG_BundlerFailed=Error: Bundler "{1}" ({0}) failed to produce a package
MSG_BundlerConfigException=Bundler {0} skipped because of a configuration problem: {1} \n\
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ error.blocked.option=jlink-Option [{0}] ist in --jlink-options nicht zul\u00E4ss
error.no.name=Name nicht mit --name angegeben. Es kann auch kein Name aus app-image abgeleitet werden

warning.no.jdk.modules.found=Warnung: Keine JDK-Module gefunden
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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

error.invalid-app-image=Error: app-image dir ({0}) generated by another jpackage version or malformed .jpackage.xml

MSG_BundlerFailed=Fehler: Bundler "{1}" ({0}) konnte kein Package generieren
MSG_BundlerConfigException=Bundler {0} aufgrund eines Konfigurationsproblems \u00FCbersprungen: {1} \nEmpfehlung zur Behebung: {2}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ error.blocked.option=jlink\u30AA\u30D7\u30B7\u30E7\u30F3[{0}]\u306F--jlink-optio
error.no.name=\u540D\u524D\u304C--name\u3067\u6307\u5B9A\u3055\u308C\u3066\u304A\u3089\u305A\u3001app-image\u304B\u3089\u63A8\u8AD6\u3067\u304D\u307E\u305B\u3093

warning.no.jdk.modules.found=\u8B66\u544A: JDK\u30E2\u30B8\u30E5\u30FC\u30EB\u304C\u898B\u3064\u304B\u308A\u307E\u305B\u3093
warning.foreign-app-image=\u8B66\u544A: app-image\u30C7\u30A3\u30EC\u30AF\u30C8\u30EA({0})\u306Fjpackage\u3067\u751F\u6210\u3055\u308C\u307E\u305B\u3093\u3002
warning.invalid-app-image=\u8B66\u544A: app-image\u30C7\u30A3\u30EC\u30AF\u30C8\u30EA({0})\u306E.jpackage.xml\u3092\u89E3\u6790\u3067\u304D\u307E\u305B\u3093

error.foreign-app-image=Error: app-image dir ({0}) not generated by jpackage. Missing .jpackage.xml.
error.invalid-app-image=Error: app-image dir ({0}) generated by another jpackage version or malformed .jpackage.xml

MSG_BundlerFailed=\u30A8\u30E9\u30FC: \u30D0\u30F3\u30C9\u30E9"{1}" ({0})\u304C\u30D1\u30C3\u30B1\u30FC\u30B8\u306E\u751F\u6210\u306B\u5931\u6557\u3057\u307E\u3057\u305F
MSG_BundlerConfigException=\u69CB\u6210\u306E\u554F\u984C\u306E\u305F\u3081\u3001\u30D0\u30F3\u30C9\u30E9{0}\u304C\u30B9\u30AD\u30C3\u30D7\u3055\u308C\u307E\u3057\u305F: {1} \n\u6B21\u306E\u4FEE\u6B63\u3092\u884C\u3063\u3066\u304F\u3060\u3055\u3044: {2}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ error.blocked.option=\u4E0D\u5141\u8BB8\u5728 --jlink-options \u4E2D\u4F7F\u7528
error.no.name=\u672A\u4F7F\u7528 --name \u6307\u5B9A\u540D\u79F0\uFF0C\u65E0\u6CD5\u4ECE app-image \u63A8\u65AD\u540D\u79F0

warning.no.jdk.modules.found=\u8B66\u544A: \u672A\u627E\u5230 JDK \u6A21\u5757
warning.foreign-app-image=\u8B66\u544A\uFF1Ajpackage \u672A\u751F\u6210 app-image \u76EE\u5F55 ({0})\u3002
warning.invalid-app-image=\u8B66\u544A\uFF1A\u65E0\u6CD5\u89E3\u6790 app-image \u76EE\u5F55 ({0}) \u4E2D\u7684 .jpackage.xml

error.foreign-app-image=Error: app-image dir ({0}) not generated by jpackage. Missing .jpackage.xml.
error.invalid-app-image=Error: app-image dir ({0}) generated by another jpackage version or malformed .jpackage.xml

MSG_BundlerFailed=\u9519\u8BEF\uFF1A\u6253\u5305\u7A0B\u5E8F "{1}" ({0}) \u65E0\u6CD5\u751F\u6210\u7A0B\u5E8F\u5305
MSG_BundlerConfigException=\u7531\u4E8E\u914D\u7F6E\u95EE\u9898, \u8DF3\u8FC7\u4E86\u6253\u5305\u7A0B\u5E8F{0}: {1} \n\u4FEE\u590D\u5EFA\u8BAE: {2}
Expand Down
37 changes: 37 additions & 0 deletions test/jdk/tools/jpackage/helpers/jdk/jpackage/test/TKit.java
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,43 @@ public static void createTextFile(Path filename, Stream<String> lines) {
trace("Done");
}

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>"
));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! My bad, sorry for the confusion.

public static void createPropertiesFile(Path propsFilename,
Collection<Map.Entry<String, String>> props) {
trace(String.format("Create [%s] properties file...",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.junit.Test;
import org.junit.Rule;
import org.junit.rules.TemporaryFolder;
import org.junit.function.ThrowingRunnable;

public class AppImageFileTest {

Expand Down Expand Up @@ -74,19 +75,19 @@ public void testInvalidCommandLine() throws IOException {

@Test
public void testInavlidXml() throws IOException {
assertInvalid(createFromXml("<foo/>"));
assertInvalid(createFromXml("<jpackage-state/>"));
assertInvalid(createFromXml(JPACKAGE_STATE_OPEN, "</jpackage-state>"));
assertInvalid(createFromXml(
assertInvalid(() -> createFromXml("<foo/>"));
assertInvalid(() -> createFromXml("<jpackage-state/>"));
assertInvalid(() -> createFromXml(JPACKAGE_STATE_OPEN, "</jpackage-state>"));
assertInvalid(() -> createFromXml(
JPACKAGE_STATE_OPEN,
"<main-launcher></main-launcher>",
"</jpackage-state>"));
assertInvalid(createFromXml(
assertInvalid(() -> createFromXml(
JPACKAGE_STATE_OPEN,
"<main-launcher>Foo</main-launcher>",
"<main-class></main-class>",
"</jpackage-state>"));
assertInvalid(createFromXml(
assertInvalid(() -> createFromXml(
JPACKAGE_STATE_OPEN,
"<launcher>A</launcher>",
"<launcher>B</launcher>",
Expand All @@ -99,19 +100,25 @@ public void testValidXml() throws IOException {
JPACKAGE_STATE_OPEN,
"<main-launcher>Foo</main-launcher>",
"<main-class>main.Class</main-class>",
"<signed>false</signed>",
"<app-store>false</app-store>",
"</jpackage-state>")).getLauncherName());

Assert.assertEquals("Boo", (createFromXml(
JPACKAGE_STATE_OPEN,
"<main-launcher>Boo</main-launcher>",
"<main-launcher>Bar</main-launcher>",
"<main-class>main.Class</main-class>",
"<signed>false</signed>",
"<app-store>false</app-store>",
"</jpackage-state>")).getLauncherName());

var file = createFromXml(
JPACKAGE_STATE_OPEN,
"<main-launcher>Foo</main-launcher>",
"<main-class>main.Class</main-class>",
"<signed>false</signed>",
"<app-store>false</app-store>",
"<launcher></launcher>",
"</jpackage-state>");
Assert.assertEquals("Foo", file.getLauncherName());
Expand Down Expand Up @@ -199,9 +206,10 @@ private AppImageFile create(Map<String, Object> params) throws IOException {
return AppImageFile.load(tempFolder.getRoot().toPath());
}

private void assertInvalid(AppImageFile file) {
Assert.assertNull(file.getLauncherName());
Assert.assertNull(file.getAddLaunchers());
private void assertInvalid(ThrowingRunnable action) {
Exception ex = Assert.assertThrows(RuntimeException.class, action);
Assert.assertTrue(ex instanceof RuntimeException);
Assert.assertTrue(ex.getMessage().contains("malformed .jpackage.xml"));
}

private AppImageFile createFromXml(String... xmlData) throws IOException {
Expand Down