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

8347755: Support static library in jmod #46

Open
wants to merge 8 commits into
base: hermetic-java-runtime
Choose a base branch
from

Conversation

slowhog
Copy link
Contributor

@slowhog slowhog commented Mar 4, 2025

This PR add --static-libs option to the jmod tool, it's basically mirroring --libs option to support adding a new section of static archive into the jmod file under the new section static-lib.

The JMOD magic header contains a MAJOR and MINOR version, we bump up the MINOR version if the --static-libs option is specified; otherwise, keep the MINOR version as 1. This allow created JMOD file without the newly supported static-lib section to continue be consumed by earlier version of jmod tool.

Also fix the formatting for _hermetic_jdk_jimage_offset, which is a julong(uint64_t) as unsigned long long on MacOS, thus need to use %llu.


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-8347755: Support static library in jmod (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 46

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/leyden/pull/46.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 4, 2025

👋 Welcome back henryjen! A progress list of the required criteria for merging this PR into hermetic-java-runtime 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 bot commented Mar 4, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot changed the title JDK-8347755: Support static library in jmod 8347755: Support static library in jmod Mar 4, 2025
@openjdk
Copy link

openjdk bot commented Mar 4, 2025

⚠️ @slowhog a branch with the same name as the source branch for this pull request (hermetic-java-runtime) is present in the target repository. If you eventually integrate this pull request then the branch hermetic-java-runtime in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the hermetic-java-runtime branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f hermetic-java-runtime b0062eff411fb3d39a5cb909962c95158904b5af
$ git push -f origin hermetic-java-runtime

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 4, 2025
@mlbridge
Copy link

mlbridge bot commented Mar 4, 2025

Webrevs

Copy link
Collaborator

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

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

The updates to the JMOD format and jmod tool looks good, as is bumping the version when the JMOD includes a static-libs section.

Doesn't have to be this PR but jmod.md will need to be updated if the overall feature is eventually proposed for main line.

@slowhog
Copy link
Contributor Author

slowhog commented Mar 4, 2025

Doesn't have to be this PR but jmod.md will need to be updated if the overall feature is eventually proposed for main line.

Also CSR would be needed.

Comment on lines +190 to +195
public static void writeMagicNumber(OutputStream os, boolean withStaticLib) throws IOException {
if (withStaticLib) {
os.write(JMOD_MAGIC_NUMBER);
} else {
os.write(JMOD_MAGIC_NO_STATIC_LIB);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This conditionality seems a bit strange. Isn't the idea for the magic to be changed when the format got extended to do more things? I.e. shouldn't this be micro version 0x01 in both cases? Then when dealing with version JMOD 1.1 version you'd be checking if static-libs is there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is without static lib, it's essential the same as the old format, using the old version so that old tools can be used if static is not included.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the idea was that the jmod format was left intentionally undefined, to not think you could rely on anything else than the corresponding JDK tools to process it? That would mean backwards compatibility should not be a concern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Just to be clear, this is about the JMOD format, not the jimage format. The jimage format is JDK internal and we are free to change it from release to release, even build to build, without compatibility concerns.)

You are right that the JMOD format is not documented but care is required when rev'ing the format because there are JDK tools that produce and consume this format. It should be possible for a project to produce a module in JMOD format with the jmod tool from JDK N, and put the package module on the module path when creating a run-time image with JDK N+1 jlink. So what Henry has looks right. It means the jmod tool will produce a 1.0 format when creating a package module that doesn't have static libs, and a JMOD in the new format when the module has static libs. This seems preferable to adding a --release option.

Choose a reason for hiding this comment

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

I guess its worth asking how would the format evolve in the future if more than /static-lib were added? Just as a strawman: say /docs and /sources were added in some sequence. Would this grow to say

if (<nothing new>) {
   // 1.0
}
else if (<just static-lib>) {
   // 1.1
}
else {
   // 1.2
}

What if /sources (and corresponding generation of src.zip) were added first and /docs second?

if (<nothing new>) {
   // 1.0
}
else if (<just static-lib>) {
   // 1.1
}
else if (<just sources +/- static-lib>) {
   // 1.2
}
else {
   // 1.3
}

Or would, at that point, versioning move to be closer to what is done for classfiles.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess its worth asking how would the format evolve in the future if more than /static-lib were added?

Unlike the JAR format, the JMOD format requires an update when new sections are added. We don't need to get hung up on any of the details around this now, most of the work to support this will be in jlink, not the jmod tool.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with this for the leyden branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking just to support -1 version. So 1.2 jmod can produce 1.2 with latest feature and 1.1 for previous version.
Anyway, as Alan pointed out, we don't need to figure this out right now.

@slowhog
Copy link
Contributor Author

slowhog commented Mar 13, 2025

Latest commit to support creating of static-jmods with make static-jmods, thanks @magicus for help.
Result on MacOS,

ls -l build/macosx-x86_64-server-release/images/static-jmods   
total 184152
-rw-r--r--  1 hjen  staff  21778174 Mar 13 10:35 java.base.jmod
-rw-r--r--  1 hjen  staff    140500 Mar 13 10:35 java.compiler.jmod
-rw-r--r--  1 hjen  staff     58934 Mar 13 10:35 java.datatransfer.jmod
-rw-r--r--  1 hjen  staff  16688299 Mar 13 10:35 java.desktop.jmod
-rw-r--r--  1 hjen  staff     68824 Mar 13 10:35 java.instrument.jmod
...
-rw-r--r--  1 hjen  staff     49946 Mar 13 10:35 jdk.xml.dom.jmod
-rw-r--r--  1 hjen  staff    112583 Mar 13 10:35 jdk.zipfs.jmod
./build/macosx-x86_64-server-release/jdk/bin/jmod list build/macosx-x86_64-server-release/images/static-jmods/java.base.jmod | grep static-lib
static-lib/libjava.a
static-lib/libjimage.a
static-lib/libjli.a
static-lib/libjsig.a
static-lib/libnet.a
static-lib/libnio.a
static-lib/libosxsecurity.a
static-lib/libsyslookup.a
static-lib/libverify.a
static-lib/libzip.a
static-lib/module-included-libs.txt

@jianglizhou
Copy link
Collaborator

./build/macosx-x86_64-server-release/jdk/bin/jmod list build/macosx-x86_64-server-release/images/static-jmods/java.base.jmod | grep static-lib
static-lib/libjava.a
static-lib/libjimage.a
static-lib/libjli.a
static-lib/libjsig.a
static-lib/libnet.a
static-lib/libnio.a
static-lib/libosxsecurity.a
static-lib/libsyslookup.a
static-lib/libverify.a
static-lib/libzip.a
static-lib/module-included-libs.txt

I think libjvm.a should be included in java.base.jmod as well.

@slowhog
Copy link
Contributor Author

slowhog commented Mar 14, 2025

I think libjvm.a should be included in java.base.jmod as well.

You are right. Looks libjvm.a is not build as a dependency, and I didn't try to copy that file either.
Looks to me, it is only built for static launcher, I'll try to add that for java.base-static-jmod.

@slowhog
Copy link
Contributor Author

slowhog commented Mar 14, 2025

Verified with make java.base-static-jmod, and libjvm.a is included.

./build/macosx-x86_64-server-release/jdk/bin/jmod list build/macosx-x86_64-server-release/images/static-jmods/java.base.jmod | grep static-lib
static-lib/libjava.a
static-lib/libjimage.a
static-lib/libjli.a
static-lib/libjsig.a
static-lib/libjvm.a
static-lib/libnet.a
static-lib/libnio.a
static-lib/libosxsecurity.a
static-lib/libsyslookup.a
static-lib/libverify.a
static-lib/libzip.a
static-lib/module-included-libs.txt

@magicus
Copy link
Member

magicus commented Mar 14, 2025

static-lib/module-included-libs.txt should not be included. It is a built-time support file. I guess this happens since you re-used an existing directory structure that was only used for build support files before. The proper fix is to make sure this file ends up in a different directory.

@slowhog
Copy link
Contributor Author

slowhog commented Mar 14, 2025

static-lib/module-included-libs.txt should not be included. It is a built-time support file. I guess this happens since you re-used an existing directory structure that was only used for build support files before. The proper fix is to make sure this file ends up in a different directory.

I didn't particularly filter-out or move this file because I wonder if this file serve as a manifest. Otherwise, we could do something like how we process other folders like modules_libs. jmod --exclude could be a solution as well.

JMOD_FLAGS += --static-libs $(STATIC_LIBS_DIR)
endif

# Copy hotspot libs, currently only libjvm.a
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized that we also need the launcher's main.o in the static java.base.jmod. Creating a liblauncher.a with the main.o and including the liblauncher.a in java.base.jmod seems to be cleaner than including main.o directly in the jmod.

When jlinking the final image for the application, if user doesn't provide customized launcher, liblauncher.a is used with other needed JDK/VM static libraries for linking the executable using the native linker (e.g. ld). If customized launcher is used, the object file(s) or the static library for the customized launcher is then used when linking the executable. liblauncher.a is not used in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Should we really, though? That includes a main function. If the user wants to use the static libs from the jmod to create their own application, that will cause a symbol conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that worth a discussion, IIRC, the most needed processing should be in libjli already.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, main.c is basically a thin wrapper that parses command line arguments and calls JLI_Launch.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @jianglizhou, I did not properly read the second part of your suggestion, to create a liblauncher.o out of main.o. Well, tbh, this is just JDK-8351367 all over again. If we should include "optional" libraries in the jmods, we need to figure out first how to handle them. How should they be marked/classified as optional or required? How should the user tell jlink to include or exclude them? If we do get a good answer to that question, then we can introduce an optional liblauncher.a as well alongside libjsig.a. But until then, we should just stay clear of "optional" static libraries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for common cases, there would be no C/C++ main() function provided from user code with Java applications. The JDK java launcher's main() is the C/C++ entry point. So we do need to provide a static library with the C/C++ main function for linking the executable.

With the customized launcher cases, the C/C++ main() is implemented by the customized launcher. Then, the JDK provided main entry point is not needed.

@magicus, yes, this involves with the optional static library in the jmods, as you pointed out. I think for C/C++ main, supporting that is necessary.

An alternative is to require Java application implement C/C++ main(). That doesn't seem to be an applaudable approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer that thin wrapper is to be provided by the end tools(like jlink) which use the jmod to link an executable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer that thin wrapper is to be provided by the end tools(like jlink) which use the jmod to link an executable.

@slowhog Can you provide details on that? Do you suggest there would be a separate executable that provides the C/C++ main entry point as a thin wrapper? The binary that statically linked with the libjvm.a, libjli.a and etc would contain no C/C++ main? Is that binary linked as a shred library? Or, your proposal is to generate some C/C++ code with the main() function using jlink, then compile the C/C++ code and use that to link the executable?

The issue that we want to solve here is to provide the C/C++ main() entry point for the executable for the final image.

@magicus
Copy link
Member

magicus commented Mar 14, 2025

I didn't particularly filter-out or move this file because I wonder if this file serve as a manifest.

No, it doesn't, it's just an implementation specific detail of the build system.

Otherwise, we could do something like how we process other folders like modules_libs. jmod --exclude could be a solution as well.

No, no. We should just don't mix temporary support files with the proper output files.

@magicus
Copy link
Member

magicus commented Mar 14, 2025

However, regarding manifests, there is one thing that really should be included with the static libs, and that is a pkg-config file, that tells the end users what linker flags to use to be able to properly use the static libraries.

I have been planning to add this to the static-jdk-image but never gotten around to it. (Can't find the JBS number now either.) But I guess that there is no real conflict between that and this PR; if I just plop the file down alongside the .a files, it will be automatically picked up by jmod.

@slowhog
Copy link
Contributor Author

slowhog commented Mar 14, 2025

I didn't particularly filter-out or move this file because I wonder if this file serve as a manifest.

No, it doesn't, it's just an implementation specific detail of the build system.

I know the original intention of this is not for that. What I am asking is if we should have such a manifest or hints for user? Similar to pkg-config.

Otherwise, we could do something like how we process other folders like modules_libs. jmod --exclude could be a solution as well.

No, no. We should just don't mix temporary support files with the proper output files.

That I agree.

@magicus
Copy link
Member

magicus commented Mar 14, 2025

Similar to pkg-config.

Maybe we just wrote past each other here... As I said, my plan is to actually include a pkg-config file. This is a canonical way to describe exactly what is needed to utilize a library. There is no reason to do "something almost like pkg-config but not actually pkg-config". :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

6 participants