-
Notifications
You must be signed in to change notification settings - Fork 7
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
Set 22.0.1-tem as SDKMAN env Java #110
Conversation
🔧 Report generated by pr-comment-scanbuild Scan-Build Report
Bug Summary
Reports
|
@@ -0,0 +1,3 @@ | |||
# Enable auto-env through the sdkman_auto_env config | |||
# Add key=value pairs of SDKs to use below | |||
java=22.0.1-tem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have a matching README requirement section if we use sdkman ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you put in it? Prompt the user to install sdkman?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are currently pre-requisites to building the project.
Using sdkman simplifies this. We can mention it as an alternative or we can just put this as the standard path to building the project.
This is not a blocking comment to merge.
Nice, using the rc file :) I wonder if we need JDK 22 for anything - we still need to keep the bytecode of the Java part compatible with Java 8. |
@jbachorik some symbols and jvmtiCapabilities are not present. Do you build successfully when using a JDK such as |
@MattAlp Yes, the baseline is JDK 11 for us - because of the JVMTI sampler. We can be progressive and use the tip (JDK 22) - we are setting the compatibility in ddprof-lib to Java 8 so we are fine at least for JDK 22 |
Is this still relevant? If not, please close it - it's been dangling for long enough time. |
It doesn't appear that we really needed this and it was suggested to check off our onboarding get-a-commit-in tradition. If we run into something that requires Java |
What does this PR do?:
This configures
sdkman
to use Temurin 22 as the Java version when working in this repo.Motivation:
We need the correct JDK registered for using JVMTI symbols when building, and
dd-trace-java
requiresJAVA_HOME
to point to a JDK 8 build. This is a convenient way of mitigating forgetting to runsdk use
or some other env var switching prior to building.Additional Notes:
I'm considering adding a shell hook here and in dd-trace-java + relevant private repos that will automatically set the appropriate environment variables for these multi-JDK-dep projects. I've used https://github.com/Shopify/shadowenv in the past and like it quite a bit, any of https://direnv.net/#related-projects would work though.
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!