8350443: GHA: Split static-libs-bundles into a separate job

[shipilev]

Noticed this when reviewing JDK-8349399, which had to kludgy workaround the hunk introduced by static-libs-bundles addition (JDK-8337265). I am somewhat surprised we even have static-libs-bundles as additional target in what I would consider a generic build-linux job! It looks cleaner to yank static-libs-bundles into a separate build job.

This effectively reverts parts of the original change, and does a few modifications:

• I see no reason to store the bundles, and continuing to do so would effectively overwrite linux-x64-bundles when we split the static build into another job, breaking tests. Not sure why we had to publish those bundles, @dougxc? They are not used in current JDK tests, I think?
• The matrix definition in build-linux.xml unconditionally includes debug configuration to override flags and suffix, I had to redo this with inline variables

Named the new job linux-x64-static, since I expect @jianglizhou to slide #23471 just there by adding another make-target into that job definition.

I did a partial GHA run already, and I expect full run to complete without errors.

Testing:

• GHA

---

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-8350443: GHA: Split static-libs-bundles into a separate job (Enhancement - P4)

Reviewers

• Magnus Ihse Bursie (@magicus - Reviewer)
• Yudi Zheng (@mur47x111 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23715

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8350443: GHA: Split static-libs-bundles into a separate job

Reviewed-by: ihse, yzheng

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

• 0f82268: 8345598: Upgrade NSS binaries for interop tests
• ea2c923: 8323807: Async UL: Add a stalling mode to async UL
• e7d4b36: 8350667: Remove startThread_lock() and _startThread_lock on AIX
• 1e18fff: 8328473: StringTable and SymbolTable statistics delay time to safepoint
• a0dd565: 8350643: G1: Make loop iteration variable type correspond to limit in G1SurvRateGroup
• aac9cb4: 8349906: G1: Improve initial survivor rate for newly used young regions
• a431046: 8350518: org.openjdk.bench.java.util.TreeMapUpdate.compute fails with "java.lang.IllegalArgumentException: key out of range"
• a70eba8: 8330174: Protection zone for easier detection of accidental zero-nKlass use
• f529bf7: 8350483: AArch64: turn on signum intrinsics by default on Ampere CPUs
• 037e471: 8350666: cmp-baseline builds fail after JDK-8280682
• ... and 11 more: https://git.openjdk.org/jdk/compare/dea7a9f0d640e5234bafe2157aecd942c71d5de5...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]

@shipilev The following label will be automatically applied to this pull request:

• build

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 pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (35dcd17c)
• 03: Full - Incremental (356f1fa8)
• 02: Full - Incremental (aadf9b6b)
• 01: Full - Incremental (95257a13)
• 00: Full (584c26f3)

[dougxc]

Please do not remove the static-lib-bundles - they are critical for testing OpenJDK PRs against libgraal: https://github.com/dougxc/openjdk-pr-canary/blob/9c38cdb2dcdbe4aef6b81732b3a296856ac1bb4c/.github/scripts/test-openjdk-pullrequests.py#L818
cc @mur47x111

[mur47x111]

We are proactively ensuring Graal remains compatible with the OpenJDK master tip without requiring contributors to build libgraal. For example, we prepared this Graal adaption patch to support your DirectByteBuffers Cleaner work. Removing static-lib-bundles would eliminate our canary tool's automation, significantly increasing manual effort. Please hold off on removing static-lib-bundles until we find a way into GHA.

[shipilev]

OK, this downstream use should really be documented if you want people to know why it should persist. New version does a comment hopefully explaining this. The static bundle is uploaded, but now separately, in linux-x64-static. I am guessing that would work for Graal downstream?

[dougxc]

OK, this downstream use should really be documented if you want people to know why it should persist. New version does a comment hopefully explaining this. The static bundle is uploaded, but now separately, in linux-x64-static. I am guessing that would work for Graal downstream?

Thanks for adding the comment!
We are now adapting the canary to work with both the old and new location of static-libs bundles.

[erikj79]

I'm not sure splitting static out into it's own task is the right long term direction we should be heading in. Magnus is working on reducing the duplicated work needed to build both a regular JDK and static libs at the same time, by reusing all the object files.

I would also argue that there is a big difference between the old static-libs-bundle and the new static JDK distribution conceptually, so bunching them up in the same "static" category doesn't make much sense to me, other than as a (misguided) attempt at optimizing GHA compute resources. The old static-libs-bundle is static versions of JDK libs needed by certain downstream consumers of the regular JDK distribution (Graal). This is essentially an extension of the regular JDK. The new static JDK distro that @jianglizhou and @magicus are working on is a complete separate JDK variant that replaces the regular JDK.

Having one build task produce more than one JDK for testing can cause a challenge in the model for a build and test system, so for that reason it makes sense having a separate build task for the static JDK. That would also isolate build problems with the static JDK and avoid having them block test results for the regular JDK. I never delved deep enough into GHA to know if this would cause an issue here or not.

[shipilev]

The point about difference between static-libs and static JDK is valid. I renamed the job to -static-libs, and would expect #23471 to add another job like build-linux-x64-static that would build static JDK.

Conceptually, I have major reservations about sneaking in static-libs-bundle make target in the generic build-linux job script. It might have been OK when it was originally done, but it is IMO a hacky solution, which prompts even more hacks to workaround the first hack! See #23471. We are also "lucky" that no other jobs call into build-linux script with debug-level: release, so we are not building static-libs-bundle in all cross-compilation, no-pch, Zero and other jobs that only ask for hotspot, for example.

If we want to build static-libs-bundle only for Linux x64 release, the clean way to do this is to explicitly define it as separate job.

At some point in the future -- once build system catches up -- we may consider adding static-libs-bundles into default make target list in build-linux / build-windows / etc. scripts. This would also be clean. But before that happens, the non-standard build targets have IMO no business being spliced into the generic scripts.

[jianglizhou]

Yes, we should distinguish between static-libs and static JDK. Currently, we refer static JDK as a 'fully' statically linked java launcher, plus lib/modules and other JDK resource files needed for runtime.

For longer term, including static-libs-bundles by default may be what we want. For example, in our current hermetic Java prototype, we release a regular JDK + static-libs. The final hermetic Java image creation step builds the single image using:

• launcher (code)
• JDK/hostpot static-libs
• lib/modules
• JDK resource files
• application classes and resources
• application natives and dependencies

During the image creation process, a launcher executable is linked using the launcher .o file, (needed) JDK/hotspot static libs, application natives and dependencies (in .o files or static libraries).

[mur47x111]

Hi @shipilev , we have merged the adaption in our canary tool. Could you please push a commit in this PR and see how it may trigger on our end? A trivial blank trimming or merge master would be sufficient.

[magicus]

I think this is good. The static libs situation is somewhat messy due to historical reasons, and it is not always easy to know what is the best improvement, in the short term or in the long term.

I would like to remove the old static-libs target and replace it with the new static-jdk-image, but we're not there yet. In the meantime, I think it makes sense to separate it out cleanly as you have done here.

[shipilev]

Hi @shipilev , we have merged the adaption in our canary tool. Could you please push a commit in this PR and see how it may trigger on our end? A trivial blank trimming or merge master would be sufficient.

Does a merge commit count? Just merged from master :)

[mur47x111]

Graal's canary tool now works with this change!

[shipilev]

Great! Let's integrate and unblock @jianglizhou's work.

/integrate

[openjdk]

Going to push as commit bd112c4.
Since your change was applied there have been 22 commits pushed to the master branch:

• 2731712: 8287749: Re-enable javadoc -serialwarn option
• 0f82268: 8345598: Upgrade NSS binaries for interop tests
• ea2c923: 8323807: Async UL: Add a stalling mode to async UL
• e7d4b36: 8350667: Remove startThread_lock() and _startThread_lock on AIX
• 1e18fff: 8328473: StringTable and SymbolTable statistics delay time to safepoint
• a0dd565: 8350643: G1: Make loop iteration variable type correspond to limit in G1SurvRateGroup
• aac9cb4: 8349906: G1: Improve initial survivor rate for newly used young regions
• a431046: 8350518: org.openjdk.bench.java.util.TreeMapUpdate.compute fails with "java.lang.IllegalArgumentException: key out of range"
• a70eba8: 8330174: Protection zone for easier detection of accidental zero-nKlass use
• f529bf7: 8350483: AArch64: turn on signum intrinsics by default on Ampere CPUs
• ... and 12 more: https://git.openjdk.org/jdk/compare/dea7a9f0d640e5234bafe2157aecd942c71d5de5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit bd112c4.

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

[jianglizhou]

Thank you, @shipilev!