Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253149: Building an installer from invalid app image fails on Window… #271

Closed
wants to merge 5 commits into from

Conversation

@andyherrick
Copy link

@andyherrick andyherrick commented Sep 20, 2020

8253149: Building an installer from invalid app image fails on Windows and Linux
When jpackage builds a package from an app-image that was not generated by jpackage, the tool should give user a warning message, and then complete the package anyway.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253149: Building an installer from invalid app image fails on Windows and Linux ⚠️ Title mismatch between PR and JBS.

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/271/head:pull/271
$ git checkout pull/271

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 20, 2020

👋 Welcome back herrick! 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
Copy link

@openjdk openjdk bot commented Sep 20, 2020

@andyherrick 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 (add|remove) "label" command.

@openjdk openjdk bot added the core-libs label Sep 20, 2020
@andyherrick andyherrick marked this pull request as ready for review Sep 20, 2020
@openjdk openjdk bot added the rfr label Sep 20, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 20, 2020

public static void testEmpty() {
final String name = "EmptyAppImagePackageTest";
final String imageName = name + (TKit.isOSX() ? ".app" : "");
Path appImageDir = TKit.workDir().resolve(imageName);

Choose a reason for hiding this comment

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

I'd suggest to use Tkit.createTempDirectory() to create temp directory. The function will create unique directory every time it is called allowing to run the test multiple times and have fresh empty directory in every run.

Path libdir = Files.createDirectory(dir.resolve("lib"));
Path readme = Files.createFile(libdir.resolve("README"));
try (BufferedWriter bw = Files.newBufferedWriter(readme)) {
bw.write("This is some arbitrary ext for the README file\n");

Choose a reason for hiding this comment

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

All these lines can be replaced with a single call

TKit.createTextFile(List.of("This is some arbitrary text for the README file"));

This statement will add log statements, so no manual logging is required. Also there is no need for try/catch block

@@ -270,7 +272,9 @@ static void verifyPackageBundleEssential(JPackageCommand cmd) {
checkPrerequisites = expectedCriticalRuntimePaths.equals(
actualCriticalRuntimePaths);
} else {
checkPrerequisites = true;
// AppImagePackageTest.testEmpty() will no dependencied,

Choose a reason for hiding this comment

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

Looks like a typo

@@ -74,6 +74,8 @@ error.jlink.failed=jlink failed with: {0}
error.blocked.option=jlink option [{0}] is not permitted in --jlink-options

warning.no.jdk.modules.found=Warning: No JDK Modules found
warning.forign-app-image=Warning: app-image dir ({0}) not generated by jpackage.

Choose a reason for hiding this comment

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

Should the id be named "foreign" instead of "forign"?

@@ -22,9 +22,15 @@
*/

import java.nio.file.Path;
import java.nio.file.Files;
import java.io.BufferedWriter;

Choose a reason for hiding this comment

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

Seems like this import is not needed any more.

@@ -61,6 +61,8 @@ message.ldd-not-available=ldd command not found. Package dependencies will not b
message.deb-ldd-not-available.advice=Install "libc-bin" DEB package to get ldd.
message.rpm-ldd-not-available.advice=Install "glibc-common" RPM package to get ldd.

warning.forign-app-image=Warning: app-image dir not generated by jpackage.

Choose a reason for hiding this comment

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

Should it be "foreign" instead of "forign"?

@@ -61,6 +61,8 @@ message.ldd-not-available=ldd command not found. Package dependencies will not b
message.deb-ldd-not-available.advice=Install "libc-bin" DEB package to get ldd.
message.rpm-ldd-not-available.advice=Install "glibc-common" RPM package to get ldd.

warning.foerign-app-image=Warning: app-image dir not generated by jpackage.
Copy link
Member

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

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

I think it is misspelled again. Should we also change warning to "app-image is not generated by jpackage", since "app-image dir" is less clear that image itself is invalid. Also, same message in other resource file has parameter for app-image location.

cmd.removeArgumentWithValue("--input");

// on mac, with --app-image and without --mac-package-identifier,
// we will try to infer it from the image, so forign image needs it.
Copy link
Member

@sashamatveev sashamatveev Sep 22, 2020

Choose a reason for hiding this comment

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

forign -> foreign. Same at line 92.

@andyherrick
Copy link
Author

@andyherrick andyherrick commented Sep 22, 2020

OK - I spelled foreign wrong here too.
will fix them all in the morning.

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Sep 25, 2020

Looks fine to me.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@andyherrick 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 more details.

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

8253149: Building an installer from invalid app image fails on Window…

Reviewed-by: asemenyuk, almatvee, kizune

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 67 new commits pushed to the master branch:

  • 0e855fe: 8252377: Incorrect encoding for EC AlgorithmIdentifier
  • 9150b90: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
  • 0187567: 8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …
  • a75edc2: 8251188: Update LDAP tests not to use wildcard addresses
  • 1f5a033: 8253555: Make ByteSize and WordSize typed scoped enums
  • f62eefc: 8253469: ARM32 Zero: replace usages of __sync_synchronize() with OrderAccess::fence
  • 1b79326: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
  • dc1ef58: 8253631: Remove unimplemented CompileBroker methods after JEP-165
  • 27d0a70: 8253633: Remove unimplemented TieredThresholdPolicy::set_carry_if_neccessary
  • e12d94a: 8253594: Remove CollectedHeap::supports_tlab_allocation
  • ... and 57 more: https://git.openjdk.java.net/jdk/compare/43be5a3cb65d5c8165d9493f56b24c921c40cff3...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.

@openjdk openjdk bot added the ready label Sep 25, 2020
@andyherrick
Copy link
Author

@andyherrick andyherrick commented Sep 25, 2020

/integrate

@openjdk openjdk bot closed this Sep 25, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@andyherrick Since your change was applied there have been 67 commits pushed to the master branch:

  • 0e855fe: 8252377: Incorrect encoding for EC AlgorithmIdentifier
  • 9150b90: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64
  • 0187567: 8250984: Memory Docker tests fail on some Linux kernels w/o cgroupv1 …
  • a75edc2: 8251188: Update LDAP tests not to use wildcard addresses
  • 1f5a033: 8253555: Make ByteSize and WordSize typed scoped enums
  • f62eefc: 8253469: ARM32 Zero: replace usages of __sync_synchronize() with OrderAccess::fence
  • 1b79326: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
  • dc1ef58: 8253631: Remove unimplemented CompileBroker methods after JEP-165
  • 27d0a70: 8253633: Remove unimplemented TieredThresholdPolicy::set_carry_if_neccessary
  • e12d94a: 8253594: Remove CollectedHeap::supports_tlab_allocation
  • ... and 57 more: https://git.openjdk.java.net/jdk/compare/43be5a3cb65d5c8165d9493f56b24c921c40cff3...master

Your commit was automatically rebased without conflicts.

Pushed as commit b159e4e.

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

@andyherrick andyherrick deleted the JDK-8253149 branch Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants