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

Fix spring-boot#481: add JRE_HOME #395

Merged
merged 2 commits into from
May 23, 2024
Merged

Fix spring-boot#481: add JRE_HOME #395

merged 2 commits into from
May 23, 2024

Conversation

anthonydahanne
Copy link
Member

  • JRE_HOME is often used as a convention to convey the path of a JRE; in this case it's convenient to distinguish the JRE from a JDK (when both are required), since the PATH can insert the JRE_HOME/bin or JDK_HOME/bin in a non predictable order.
  • Thanks to JRE_HOME, subsequent build steps will be able to use JRE_HOME/bin/java to make sure they pick a JRE and not a JDK
  • in case there's just a JDK desired by the user, JRE_HOME will also be set, as the JDK_HOME

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@anthonydahanne anthonydahanne requested a review from a team as a code owner May 22, 2024 20:44
* JRE_HOME is often used as a convention to convey the path of a JRE; in this case it's convenient to distinguish the JRE from a JDK (when both are required), since the PATH can insert the JRE_HOME/bin or JDK_HOME/bin in a non predictable order.
* Thanks to JRE_HOME, subsequent build steps will be able to use JRE_HOME/bin/java to make sure they pick a JRE and not a JDK
* in case there's just a JDK desired by the user, JRE_HOME will also be set, as the JDK_HOME
@anthonydahanne anthonydahanne added type:bug A general bug semver:patch A change requiring a patch version bump labels May 22, 2024
anthonydahanne added a commit to paketo-buildpacks/spring-boot that referenced this pull request May 23, 2024
* JRE_HOME is now set in libjvm, see paketo-buildpacks/libjvm#395
* thanks to that, we know for sure that the JRE_HOME/bin/java will be used for the training run
Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@dmikusa
Copy link
Contributor

dmikusa commented May 23, 2024

So if I have this correct.

At build time:

  • JRE_HOME -> the JRE
  • JDK_HOME -> the JDK
  • JAVA_HOME -> the JDK (cause JRE is set to default, and JDK is set to override)

I'm curious what the behavior of PATH is here, given that both the JRE & JDK will be on PATH. I guess it depends on how the lifecycle processes the layers and puts them on the PATH? What have you observed @anthonydahanne ?

@dmikusa
Copy link
Contributor

dmikusa commented May 23, 2024

Should we also update the JLink installer?

That presently sets JAVA_HOME at build time but not JRE_HOME or JDK_HOME. I'm not 100% sure what it should do though. Probably JRE_HOME cause we strip out the JDK part, at least by default, and you're guaranteed to have at least a JRE.

@anthonydahanne
Copy link
Member Author

anthonydahanne commented May 23, 2024

At build time:

JRE_HOME -> the JRE
JDK_HOME -> the JDK
JAVA_HOME -> the JDK (cause JRE is set to default, and JDK is set to override)

Almost: if the buildplan requires both JRE and JDK: JRE_HOME -> the JDK

I'm curious what the behavior of PATH is here, given that both the JRE & JDK will be on PATH. I guess it depends on how the lifecycle processes the layers and puts them on the PATH? What have you observed @anthonydahanne ?

Well, PATH is kind of... not predictable. What I have observed is JDK path sometimes before, sometimes after JRE path in the PATH env.
So, that was not deterministic to call java from a contribute method, so that's why it's fixed using JRE_HOME/bin/java

Should we also update the JLink installer?

That, I don't know. But... I supposed it's not required if nobody had any issues with it so far.
We had the issue with CDS training run: CDS MUST use the same JVM during training run and runtime; and because of PATH, it was not reliable

@dmikusa
Copy link
Contributor

dmikusa commented May 23, 2024

Well, PATH is kind of... not predictable. What I have observed is JDK path sometimes before, sometimes after JRE path in the PATH env.

Interesting 🤔 I'm going to mention this upstream and see what they say.

Almost: if the buildplan requires both JRE and JDK: JRE_HOME -> the JDK

Oh, that's not what I would have expected. Why would JRE_HOME point to the JDK? Isn't that what you're trying to avoid? Cause the JDK would run the CDS training, but then the JRE would run the app at runtime and they'd be different?

anthonydahanne added a commit to paketo-buildpacks/spring-boot that referenced this pull request May 23, 2024
* JRE_HOME is now set in libjvm, see paketo-buildpacks/libjvm#395
* thanks to that, we know for sure that the JRE_HOME/bin/java will be used for the training run
@anthonydahanne
Copy link
Member Author

oh I'm sorry, I pressed enter too fast
I meant to say:

Almost: if the buildplan requires ONLY a JDK (BP_JVM_TYPE=jdk): JRE_HOME -> the JDK

@anthonydahanne anthonydahanne merged commit ed45e1b into main May 23, 2024
4 checks passed
@anthonydahanne anthonydahanne deleted the fix-sb-481 branch May 23, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants