8307194: Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries

[jianglizhou]

This PR is branched from the makefile changes for https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for handling the JDK/hotspot static libraries:

• Build hotspot libjvm.a and JDK static libraries for static-libs-image/static-libs-bundles targets; This change does not affect the graal-builder-image target

• For libjvm.a specifically, exclude operator_new.o

• Filter out "external" .o files (those are the .o files included from a different JDK library and needed when creating the .so shared library only) from JDK .a libraries; That's to avoid linker failures caused by duplicate symbols

  • For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled

• For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since it's provided in libawt.a

• Handle long arguments case for static build in make/common/NativeCompilation.gmk

• Address @erikj79's comment in 8303796: Optionally build fully statically linked JDK image #13709 (comment) for LIBJLI_STATIC_EXCLUDE_OBJS

---

Progress

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

Issue

• JDK-8307194: Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)
• Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13768

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13768.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jiangli! 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]

@jianglizhou The following labels will be automatically applied to this pull request:

• build
• client
• core-libs

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

[mlbridge]

Webrevs

• 11: Full - Incremental (ba2e9ce6)
• 10: Full - Incremental (36526d4f)
• 09: Full - Incremental (45dd2a00)
• 08: Full - Incremental (d30b3e34)
• 07: Full - Incremental (cf317d36)
• 06: Full - Incremental (2cb786ab)
• 05: Full - Incremental (e49d7b4e)
• 04: Full - Incremental (84861990)
• 03: Full - Incremental (e65aee25)
• 02: Full - Incremental (1f62842e)
• 01: Full - Incremental (f74b576d)
• 00: Full (f8be9e95)

[erikj79]

The current target user of the .a libraries is GraalVM, so we should check with them that the changes to the contents of the .a files isn't impacting them in a bad way. @dougxc what do you think?

[dougxc]

The current target user of the .a libraries is GraalVM, so we should check with them that the changes to the contents of the .a files isn't impacting them in a bad way. @dougxc what do you think?

Thanks for the heads up - I've asked the GraalVM Native Image team to look at this PR.

[jerboaa]

@jianglizhou How does the produced image look like after this patch? I.e. what's the contents of build/*/images/static-libs?

[jerboaa]

Could we decouple hotspot-static-libs from static-libs-image somehow, please? static-libs-image is used by the graal-builder-image target and it would be good if it didn't include hotspot static libs as they are not needed for it.

Would it be sufficient to just use hotspot-static-libs directly? Like: make static-libs-image hotspot-static-libs? Failing that, could we introduce a new target that produces both?

[erikj79]

I'm hoping to get input from the graal team on the impact of this change. The exact usage of the new libjvm.a file is still under discussion so I share you concern about changing things for the current static libs usecase before we fully understand where this is going.

[jianglizhou]

Could we decouple hotspot-static-libs from static-libs-image somehow, please? static-libs-image is used by the graal-builder-image target and it would be good if it didn't include hotspot static libs as they are not needed for it.

Would it be sufficient to just use hotspot-static-libs directly? Like: make static-libs-image hotspot-static-libs? Failing that, could we introduce a new target that produces both?

Good questions. I had similar thoughts when making the makefile changes. Here's my reasoning with the current approach in this PR:

The images/static-libs/lib would provide a super set of the JDK/VM static libraries (in a JDK binary/release) for downstream developers to produce their desired final static image. With the addition of the libjvm.a and potentially bundled libzlib.a and libfreetype.a included in static-libs-image output, users could select the needed subset of the static libraries for their linking step (e.g. via jlink based on the needed modules) to produce the final image.

If these changes are cumbersome for graal-builder-image usages, using hotspot-static-libs directly for producing libjvm.a as you suggested could be doable. Or, we could try using a new make target for producing the .a superset. There might be potential concerns/issues with the differences between graal-builder-image support and Java static image support, i.e. it might be a good idea to explore unified solution for both if possible. @dougxc and others related to GraalVM Native Image are on this review thread. It's a good idea to collect the thoughts together.

[jerboaa]

GraalVM native-image has it's own libjvm.a shim which would likely conflict or produce undesirable results. So I'd prefer the approach where static-libs-image wouldn't produce hotspot libjvm.a as part of it. For new uses-cases needing that, we could add a new top-level target (like graal-builder-image) which would produce such an image. As one of the Mandrel maintainers, I'm not sure any post-build filtering via jlink or the like would be ideal for us. I'll see if I can test this on a mandrel build tomorrow...

[jianglizhou]

@jerboaa, thanks for the info.

[jianglizhou]

@jerboaa Thanks very much for testing with mandrel. Based on yours and @olpaw's feedback, I'll refactor this PR to not use the existing static-libs-image target for including hotspot libjvm.a. I'll also reflect that in the bug & PR titles as well.

[jianglizhou]

As @jerboaa mentioned, for GraalVM native-image we produce our own libjvm.a as part of building GraalVM (every native image gets statically linked to that library). See https://github.com/oracle/graal/blob/f1c1d710625ac84559a6ef69c4068c9d8c2c9f8b/substratevm/mx.substratevm/mx_substratevm.py#L1378 and com.oracle.svm.native.jvm.{posix,windows} in https://github.com/oracle/graal/blob/f1c1d710625ac84559a6ef69c4068c9d8c2c9f8b/substratevm/mx.substratevm/suite.py#L736.

Having a hot-spot variant of libjvm.a next to the other static libraries might complicate things for us. Ideally the output files produced by target static-libs-image should remain the same.

@olpaw Thanks for the input. Could you also please take a look of the changes that excludes the object files from the following static libraries from Graal native image usage perspective?

• For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled.
• For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since it's provided in libawt.a.

[AlanBateman]

At first glance, static-libs-image create libjvm.a in the same directory as the other lib .a files make sense but I don't know how that works if --with-jvm-variants is used to create more more than one libjvm. Might have to think whether this combination make sense or not.

[jianglizhou]

@jerboaa Thanks very much for testing with mandrel. Based on yours and @olpaw's feedback, I'll refactor this PR to not use the existing static-libs-image target for including hotspot libjvm.a.

Updated PR to decouple building libjvm.a from static-libs-image target. Add a java-static-libs-image target for building the static library super set, which includes JDK .a static libraries and hotspot libjvm.a.

[jianglizhou]

At first glance, static-libs-image create libjvm.a in the same directory as the other lib .a files make sense but I don't know how that works if --with-jvm-variants is used to create more more than one libjvm. Might have to think whether this combination make sense or not.

We now use a java-static-libs-image target for building all JDK and hotspot .a static libraries.

@erikj79 Suggested building libjvm.a for $(JVM_VARIANT_MAIN) for now. I think that's a good starting point. The PR has been updated accordingly.

[jianglizhou]

@jianglizhou How does the produced image look like after this patch? I.e. what's the contents of build/*/images/static-libs?

With the changes in this patch, build/*/images/static-libs will contain the libjvm.a in addition to the JDK .a static libraries.

For the following JDK static libraries (.a), the duplicate .o files are excluded to avoid linking failures when the the JDK static libraries are linked with:

• For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled. When zlib is built as bundled, the extra source files from it are compiled and the produced object files are linked into libjli. That's intended for the shared library, libjli.so, but that's problematic for libjli.a. It's not an issue when zlib is not bundled, as the extra sources are not included for libjli in that case.
• For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since it's provided in libawt.a.

I'll add these details in the PR description as well.

[jianglizhou]

Changed to define/use the libjvm.a rule for the main variant only as suggested, thanks.

[erikj79]

I'm hoping to get input from the graal team on the impact of this change. The exact usage of the new libjvm.a file is still under discussion so I share you concern about changing things for the current static libs usecase before we fully understand where this is going.

[erikj79]

With this solution, the build will produce a different static-libs bundle and image depending on the make target called, but the output name is exactly the same. That isn't ok. If you run one target after the other, the behavior becomes strange and unpredictable. If these are different deliverables, they need to be that all the way through, with a separate image and bundle with their own make targets.

If we go with two separate deliverables, then to me it makes more sense that "static-libs-image" is the complete image with all the static libs and if graal needs a specialized subset of this, then that's something graal specific. The graal-builder-image is already meant just for their consumption (which is a combination of the jdk-image and the current static-libs-image). Having graal builds having to change the target they call is inconvenient for them, but for us to be forced to maintain unintuitive build targets is also bad.

All of that said, I think we can get away with a smaller subset of targets and deliverables. AFAIK, graal needs the combined graal-builder-image as input to their build anyway, so they should not have any dependency on what the target static-libs-image produces. Given that I propose the following behavior:

make static-libs-image produces images/static-libs with all .a (including libjvm.a).
make static-libs-graal-image produces images/static-libs-graal with all .a except libjvm.a.
make graal-builder-image produces images/graal-builder-image like today, but depends on and uses static-libs-graal-image instead of static-libs-image.
make static-libs-bundles depends on and uses static-libs-image like today, so will contain libjvm.a, which is new behavior.

Further I would like to suggest that libjvm.a gets put in the variant subdir under lib, just like libjvm.so does today (e.g. lib/server/libjvm.a). That way you can support building libjvm.a for all variants without worry. It will also get libjvm.a out of the way to cause less trouble for a graal build that uses the static-libs-bundles as input. This goes against what I suggested before, to just use JVM_VARIANT_MAIN, but I think this makes for a cleaner long term solution given the goals of the hermetic java effort.

[jerboaa]

Seems fine to me, but I'm not an expert in on the build files. Thanks for the implementing the suggestion.

[openjdk]

@jianglizhou 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:

8307194: Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries

Reviewed-by: erikj, sgehwolf

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[jianglizhou]

All of that said, I think we can get away with a smaller subset of targets and deliverables. AFAIK, graal needs the combined graal-builder-image as input to their build anyway, so they should not have any dependency on what the target static-libs-image produces. Given that I propose the following behavior:
make static-libs-image produces images/static-libs with all .a (including libjvm.a). make static-libs-graal-image produces images/static-libs-graal with all .a except libjvm.a. make graal-builder-image produces images/graal-builder-image like today, but depends on and uses static-libs-graal-image instead of static-libs-image. make static-libs-bundles depends on and uses static-libs-image like today, so will contain libjvm.a, which is new behavior.

Sure, that should work too as long as there is a way to a) build the static libs only needed for graal some way b) keep graal-builder-image working as it does today. FWIW, we use a) at adoptium so as to be able to have a combination to build mandrel from. Not all users will want to have JDK + static libs so only the ones needing them should need to download them.

Thanks @erikj79 @jerboaa. We can go with what @erikj79 suggested then. I'll revise the PR.

[jianglizhou]

Further I would like to suggest that libjvm.a gets put in the variant subdir under lib, just like libjvm.so does today (e.g. lib/server/libjvm.a). That way you can support building libjvm.a for all variants without worry. It will also get libjvm.a out of the way to cause less trouble for a graal build that uses the static-libs-bundles as input. This goes against what I suggested before, to just use JVM_VARIANT_MAIN, but I think this makes for a cleaner long term solution given the goals of the hermetic java effort.

There are some complications related to jvm.cfg and libjvm.so path runtime handling for JVM variants support, with the JDK traditional/dynamic build. With JDK static support, the usage of libjvm is fully determined at build time, runtime can avoid the complications and associated overhead with accessing jvm.cfg and determining libjvm path.

At the final static image build time users need to know the sub-directory containing libjvm.a, if we go with the suggestion. That adds a small complication. However, from broader point of view there are probably more benefits to do so. I'll change the PR, thanks.

[jianglizhou]

All of that said, I think we can get away with a smaller subset of targets and deliverables. AFAIK, graal needs the combined graal-builder-image as input to their build anyway, so they should not have any dependency on what the target static-libs-image produces. Given that I propose the following behavior:
make static-libs-image produces images/static-libs with all .a (including libjvm.a). make static-libs-graal-image produces images/static-libs-graal with all .a except libjvm.a. make graal-builder-image produces images/graal-builder-image like today, but depends on and uses static-libs-graal-image instead of static-libs-image. make static-libs-bundles depends on and uses static-libs-image like today, so will contain libjvm.a, which is new behavior.

Sure, that should work too as long as there is a way to a) build the static libs only needed for graal some way b) keep graal-builder-image working as it does today. FWIW, we use a) at adoptium so as to be able to have a combination to build mandrel from. Not all users will want to have JDK + static libs so only the ones needing them should need to download them.

Thanks @erikj79 @jerboaa. We can go with what @erikj79 suggested then. I'll revise the PR.

@erikj79 and @jerboaa please please review the updated version, thanks.

[erikj79]

I think you also need to make a change to GraalBuilderImage.gmk and the target in Main.gmk that calls it.

Otherwise this looks pretty good to me.

[erikj79]

Can you add a newline to try to keep line length in check here. No need to reformat the whole block, just don't let random lines shoot way over 80.

[jianglizhou]

Done, thanks.

[jianglizhou]

I think you also need to make a change to GraalBuilderImage.gmk and the target in Main.gmk that calls it.

Good catch! Fixed GraalBuilderImage.gmk, thanks!

For the graal-builder-image target that uses GraalBuilderImage.gmk, it's already changed to use static-libs-graal-image as one of the DEPS. Please let me know if anything additional that I missed.

[jerboaa]

Looks fine to me.

[jianglizhou]

Thanks a lot for the multiple iterations of the discussions and reviews in this PR thread. All the input, especially #13768 (review) from @erikj79 helped shaped this change.

[jianglizhou]

/integrate

[openjdk]

Going to push as commit 1964954.

[openjdk]

@jianglizhou Pushed as commit 1964954.

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

[erikj79]

This change caused all our builds but Linux to fail. Did you verify on other platforms than Linux at all? I see you have GHA turned off.

[jianglizhou]

This change caused all our builds but Linux to fail. Did you verify on other platforms than Linux at all? I see you have GHA turned off.

Sorry about the issue. There were failed workflows after I merged with the latest JDK master this morning. I canceled the workflow and tried a rerun, which didn't work. I retried with a new commit with merging again. And noticed the GHA was off after merging with the latest master, but didn't know what caused it. The last night tests in the workflow (before merging with the master) were successful. So I proceeded with integration.

Please let me know if we should back out or fix forward ...

[erikj79]

This change caused all our builds but Linux to fail. Did you verify on other platforms than Linux at all? I see you have GHA turned off.

Sorry about the issue. There were failed workflows after I merged with the latest JDK master this morning. I canceled the workflow and tried a rerun, which didn't work. I retried with a new commit with merging again. And noticed the GHA was off after merging with the latest master, but didn't know what caused it. The last night tests in the workflow (before merging with the master) were successful. So I proceeded with integration.

Please let me know if we should back out or fix forward ...

Dan is already backing out in #13918, so we have plenty of time to figure out the issues.

[jianglizhou]

This change caused all our builds but Linux to fail. Did you verify on other platforms than Linux at all? I see you have GHA turned off.

Sorry about the issue. There were failed workflows after I merged with the latest JDK master this morning. I canceled the workflow and tried a rerun, which didn't work. I retried with a new commit with merging again. And noticed the GHA was off after merging with the latest master, but didn't know what caused it. The last night tests in the workflow (before merging with the master) were successful. So I proceeded with integration.
Please let me know if we should back out or fix forward ...

Dan is already backing out in #13918, so we have plenty of time to figure out the issues.

Ok, thanks! Sorry about the build failure again.