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

JDK-8258755: jpackage: Invalid 32-bit exe when building app-image #2030

Closed
wants to merge 3 commits into from

Conversation

@andyherrick
Copy link

@andyherrick andyherrick commented Jan 11, 2021


Progress

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

Issue

  • JDK-8258755: jpackage: Invalid 32-bit exe when building app-image

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 11, 2021

👋 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 Jan 11, 2021

@andyherrick To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing
@andyherrick
Copy link
Author

@andyherrick andyherrick commented Jan 11, 2021

JDK-8258755: jpackage: Invalid 32-bit exe when building app-image

@openjdk openjdk bot added the rfr label Jan 11, 2021
@@ -33,6 +33,11 @@
#include "Toolbox.h"
#include "ErrorHandling.h"

#if defined(_WIN32) && !defined(_WIN64)
#define LAUNCH_FUNC "_JLI_Launch@56"

This comment has been minimized.

@sashamatveev

sashamatveev Jan 12, 2021
Member

Why JLI_Launch does not work for 32-bit? Do you know why @56 is being used?

This comment has been minimized.

@andyherrick

andyherrick Jan 12, 2021
Author

not so much that is doesn't work, but that it doesn't exist,
The 32 bit version of jli.dll does not have a "JLI_LAUNCH" entry point, but does have a "_JLI_Launch@56" which works perfectly well.

static boolean is64Bit() {
return is64b;
}

Comment on lines 124 to 127

This comment has been minimized.

@alexeysemenyukoracle

alexeysemenyukoracle Jan 13, 2021
Member

This is Windows specific thing. There is no point to keep it in shared code. I'd suggest to move this function in WixSourcesBuilder class.

I think it is not correct to check for arch of the OS where jpackage runs. Jpackage from 32bit JDK (if somebody would build 32bit OpenJDK) would bake in 32bit Java runtime in app image regardless Windows is 64bit or 32bit. So the arch of installer is determined by arch of Java runtime baked in the app image. For simplicity we can assume that arch of Java runtime is the same as the arch of jpackage (this might not be the case for external Java runtime case though, but let's leave this possibility aside as it is not supported anyways). If so, checking of sun.arch.data.model system property would be better alternative to os.arch.

This comment has been minimized.

@alexeysemenyukoracle

alexeysemenyukoracle Jan 13, 2021
Member

Sorry, I left this comment earlier, but it was lost.

This comment has been minimized.

@andyherrick

andyherrick Jan 13, 2021
Author

I agree is better to have the check in windows specific code (not Platform) and will move it.
The "os.arch" System property in 32 java is "x86" regardless of whether running on 64 or 32 bit version of Windows, It reflects the arch of the running JVM (not the arch of the os as the name implies), so checking if system property "os.arch" is "x86" is the same as checking if "sun,arch.data.model" is "32". Since "os.arch" is the more documented property I think we should continue to use that.

This comment has been minimized.

@andyherrick

andyherrick Jan 13, 2021
Author

(note:) I do build 32 bit java on windows to test this, I can "make CONF=windows-x86 images", but building installers is not supported and currently building test has problems in hotspot.

This comment has been minimized.

@alexeysemenyukoracle

alexeysemenyukoracle Jan 13, 2021
Member

Agree on os.arch

@andyherrick
Copy link
Author

@andyherrick andyherrick commented Jan 13, 2021

/label add core-libs

@openjdk openjdk bot added the core-libs label Jan 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@andyherrick
The core-libs label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 13, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Jan 16, 2021

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

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

8258755: jpackage: Invalid 32-bit exe when building app-image

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

  • d63388c: 8259266: com/sun/jdi/JdbOptions.java failed with "RuntimeException: 'prop[boo] = >foo 2<' missing from stdout/stderr"
  • d7d34dd: 8259799: vmTestbase/nsk/jvmti/Breakpoint/breakpoint001 is incorrect
  • fe84ecd: 8259048: (tz) Upgrade time-zone data to tzdata2020f
  • 9aa5672: 8259068: Streamline class loader locking
  • 27a39c8: 8193031: Collections.addAll is likely to perform worse than Collection.addAll
  • 5855d52: 8259651: [macOS] Replace JNF_COCOA_ENTER/EXIT macros
  • 360c722: 8259729: Missed JNFInstanceOf -> IsInstanceOf conversion
  • b78cd63: 8259846: [BACKOUT] JDK-8259278 Optimize Vector API slice and unslice operations
  • eb7fa00: 8259216: javadoc omits method receiver for any nested type annotation
  • bcf20a0: 8259777: Incorrect predication condition generated by ADLC
  • ... and 184 more: https://git.openjdk.java.net/jdk/compare/722f23610e128991e21ba1b56ddf2b7eec99e5c2...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 Jan 16, 2021
@andyherrick
Copy link
Author

@andyherrick andyherrick commented Jan 16, 2021

/integrate

@openjdk openjdk bot closed this Jan 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 16, 2021

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

  • c3bdbf9: 8259238: Clean up Log.java and remove usage of non-final static variables.
  • 6d6a23e: 8259062: Remove MacAppStoreBundler
  • afd3f78: 8030048: (fs) Support UserDefinedFileAttributeView/extended attributes on OS X / HFS+
  • bbb93ca: 8256126: Create implementation for NSAccessibilityImage protocol peer
  • 90c73d0: 8259569: gtest os.dll_address_to_function_and_library_name_vm fails
  • 536082d: Merge
  • e85892b: 8258396: SIGILL in jdk.jfr.internal.PlatformRecorder.rotateDisk()
  • d63388c: 8259266: com/sun/jdi/JdbOptions.java failed with "RuntimeException: 'prop[boo] = >foo 2<' missing from stdout/stderr"
  • d7d34dd: 8259799: vmTestbase/nsk/jvmti/Breakpoint/breakpoint001 is incorrect
  • fe84ecd: 8259048: (tz) Upgrade time-zone data to tzdata2020f
  • ... and 191 more: https://git.openjdk.java.net/jdk/compare/722f23610e128991e21ba1b56ddf2b7eec99e5c2...master

Your commit was automatically rebased without conflicts.

Pushed as commit da4cf05.

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

@andyherrick andyherrick deleted the andyherrick:JDK-8258755 branch Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants