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

[OPENJDK-2916] Reintroduce Pathfinder module #482

Closed
wants to merge 4 commits into from

Conversation

Josh-Matsuoka
Copy link
Contributor

This PR combines https://github.com/jboss-container-images/openjdk/pull/401 and https://github.com/jboss-container-images/openjdk/pull/455 and reintroduces the pathfinder module, targeting the ubi9 branch since we were unable to isolate the commits and cherry pick it over, a new PR was the cleanest way forward.

@Josh-Matsuoka Josh-Matsuoka requested review from jmtd and jhuttana April 12, 2024 17:25
Copy link
Contributor

@jhuttana jhuttana left a comment

Choose a reason for hiding this comment

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

LGTM
I have verified the build for JDK 21 and I can see the jlink related script placed under expected path

$ docker run --rm -ti ubi9/openjdk-21:latest bash -c "ls -l /opt/jboss/container/java/jlink"
total 16
-rwxrwxr--. 1 default root  313 Apr 15 05:44 generatejdkdeps.sh
-rwxrwxr--. 1 default root 1082 Apr 15 05:44 mkdeps.sh
-rwxrwxr--. 1 default root  322 Apr 15 05:44 mkjreimage.sh
-rwxrwxr--. 1 default root  612 Apr 15 05:44 mkstrippeddeps.sh

modules/jdk/11/module.yaml Outdated Show resolved Hide resolved
modules/s2i/bash/module.yaml Outdated Show resolved Hide resolved
Copy link
Member

@jmtd jmtd left a comment

Choose a reason for hiding this comment

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

A few changes to make please, the main ones being remove the jlink module and references to it (we want to save that for the final PR that merges the jlink-dev branch in) and update the pathfinder logic to reflect the current logic in the ubi9 branch, which has moved on a bit from what it was when you originally created pathfinder.

@Josh-Matsuoka Josh-Matsuoka requested a review from jmtd April 15, 2024 19:09
modules/s2i/bash/module.yaml Outdated Show resolved Hide resolved
modules/s2i/core/module.yaml Outdated Show resolved Hide resolved
Copy link
Member

@jmtd jmtd left a comment

Choose a reason for hiding this comment

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

Thanks for iterating fast on this Josh. A couple more issues to resolve.

@Josh-Matsuoka Josh-Matsuoka requested a review from jmtd April 16, 2024 14:40
Copy link
Member

@jmtd jmtd left a comment

Choose a reason for hiding this comment

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

PLease remove these remaining jlink module files:
modules/jlink/artifacts/opt/jboss/container/java/jlink/generatejdkdeps.sh
modules/jlink/artifacts/opt/jboss/container/java/jlink/mkdeps.sh
modules/jlink/artifacts/opt/jboss/container/java/jlink/mkjreimage.sh
modules/jlink/artifacts/opt/jboss/container/java/jlink/mkstrippeddeps.sh
modules/jlink/configure.sh
modules/jlink/module.yaml

@jmtd
Copy link
Member

jmtd commented Apr 25, 2024

We should revisit the approach to this. Merging this in isolation from jlink-dev carries no advantages to the images but introduces the risk of a bug (however small).

@jmtd jmtd closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants