-
Notifications
You must be signed in to change notification settings - Fork 324
Remove remaining use of JAVA_X_HOME
#10340
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
base: master
Are you sure you want to change the base?
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 7 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.092 s) : 0, 1091744
Total [baseline] (10.903 s) : 0, 10902682
Agent [candidate] (1.086 s) : 0, 1085670
Total [candidate] (10.796 s) : 0, 10796038
section appsec
Agent [baseline] (1.266 s) : 0, 1265904
Total [baseline] (11.061 s) : 0, 11060868
Agent [candidate] (1.266 s) : 0, 1265614
Total [candidate] (11.046 s) : 0, 11046377
section iast
Agent [baseline] (1.244 s) : 0, 1244260
Total [baseline] (11.256 s) : 0, 11255638
Agent [candidate] (1.236 s) : 0, 1235517
Total [candidate] (11.211 s) : 0, 11211275
section profiling
Agent [baseline] (1.205 s) : 0, 1205324
Total [baseline] (10.874 s) : 0, 10874272
Agent [candidate] (1.203 s) : 0, 1202775
Total [candidate] (10.836 s) : 0, 10836406
gantt
title petclinic - break down per module: candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.187 ms) : 0, 1187
crashtracking [candidate] (1.2 ms) : 0, 1200
BytebuddyAgent [baseline] (656.109 ms) : 0, 656109
BytebuddyAgent [candidate] (651.706 ms) : 0, 651706
GlobalTracer [baseline] (284.02 ms) : 0, 284020
GlobalTracer [candidate] (283.0 ms) : 0, 283000
AppSec [baseline] (32.84 ms) : 0, 32840
AppSec [candidate] (32.498 ms) : 0, 32498
Debugger [baseline] (68.336 ms) : 0, 68336
Debugger [candidate] (68.359 ms) : 0, 68359
Remote Config [baseline] (645.52 µs) : 0, 646
Remote Config [candidate] (645.157 µs) : 0, 645
Telemetry [baseline] (9.066 ms) : 0, 9066
Telemetry [candidate] (8.923 ms) : 0, 8923
Flare Poller [baseline] (3.832 ms) : 0, 3832
Flare Poller [candidate] (3.832 ms) : 0, 3832
section appsec
crashtracking [baseline] (1.188 ms) : 0, 1188
crashtracking [candidate] (1.189 ms) : 0, 1189
BytebuddyAgent [baseline] (691.373 ms) : 0, 691373
BytebuddyAgent [candidate] (691.687 ms) : 0, 691687
GlobalTracer [baseline] (258.876 ms) : 0, 258876
GlobalTracer [candidate] (258.88 ms) : 0, 258880
AppSec [baseline] (173.734 ms) : 0, 173734
AppSec [candidate] (172.558 ms) : 0, 172558
Debugger [baseline] (66.724 ms) : 0, 66724
Debugger [candidate] (67.613 ms) : 0, 67613
Remote Config [baseline] (757.567 µs) : 0, 758
Remote Config [candidate] (777.081 µs) : 0, 777
Telemetry [baseline] (9.405 ms) : 0, 9405
Telemetry [candidate] (9.362 ms) : 0, 9362
Flare Poller [baseline] (3.856 ms) : 0, 3856
Flare Poller [candidate] (3.737 ms) : 0, 3737
IAST [baseline] (24.514 ms) : 0, 24514
IAST [candidate] (24.434 ms) : 0, 24434
section iast
crashtracking [baseline] (1.192 ms) : 0, 1192
crashtracking [candidate] (1.209 ms) : 0, 1209
BytebuddyAgent [baseline] (804.49 ms) : 0, 804490
BytebuddyAgent [candidate] (798.455 ms) : 0, 798455
GlobalTracer [baseline] (261.08 ms) : 0, 261080
GlobalTracer [candidate] (258.769 ms) : 0, 258769
AppSec [baseline] (35.84 ms) : 0, 35840
AppSec [candidate] (35.393 ms) : 0, 35393
Debugger [baseline] (65.53 ms) : 0, 65530
Debugger [candidate] (65.975 ms) : 0, 65975
Remote Config [baseline] (597.494 µs) : 0, 597
Remote Config [candidate] (637.616 µs) : 0, 638
Telemetry [baseline] (8.634 ms) : 0, 8634
Telemetry [candidate] (8.621 ms) : 0, 8621
Flare Poller [baseline] (3.564 ms) : 0, 3564
Flare Poller [candidate] (3.568 ms) : 0, 3568
IAST [baseline] (27.682 ms) : 0, 27682
IAST [candidate] (27.229 ms) : 0, 27229
section profiling
crashtracking [baseline] (1.207 ms) : 0, 1207
crashtracking [candidate] (1.218 ms) : 0, 1218
BytebuddyAgent [baseline] (702.354 ms) : 0, 702354
BytebuddyAgent [candidate] (700.19 ms) : 0, 700190
GlobalTracer [baseline] (220.748 ms) : 0, 220748
GlobalTracer [candidate] (220.928 ms) : 0, 220928
AppSec [baseline] (32.157 ms) : 0, 32157
AppSec [candidate] (31.943 ms) : 0, 31943
Debugger [baseline] (68.433 ms) : 0, 68433
Debugger [candidate] (68.074 ms) : 0, 68074
Remote Config [baseline] (655.168 µs) : 0, 655
Remote Config [candidate] (641.329 µs) : 0, 641
Telemetry [baseline] (8.938 ms) : 0, 8938
Telemetry [candidate] (8.819 ms) : 0, 8819
Flare Poller [baseline] (3.76 ms) : 0, 3760
Flare Poller [candidate] (3.709 ms) : 0, 3709
ProfilingAgent [baseline] (97.122 ms) : 0, 97122
ProfilingAgent [candidate] (97.448 ms) : 0, 97448
Profiling [baseline] (97.707 ms) : 0, 97707
Profiling [candidate] (98.024 ms) : 0, 98024
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.084 s) : 0, 1084023
Total [baseline] (8.75 s) : 0, 8750178
Agent [candidate] (1.094 s) : 0, 1094155
Total [candidate] (8.825 s) : 0, 8824620
section iast
Agent [baseline] (1.231 s) : 0, 1231062
Total [baseline] (9.335 s) : 0, 9335306
Agent [candidate] (1.224 s) : 0, 1223548
Total [candidate] (9.34 s) : 0, 9339911
gantt
title insecure-bank - break down per module: candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.203 ms) : 0, 1203
crashtracking [candidate] (1.215 ms) : 0, 1215
BytebuddyAgent [baseline] (651.205 ms) : 0, 651205
BytebuddyAgent [candidate] (660.043 ms) : 0, 660043
GlobalTracer [baseline] (283.08 ms) : 0, 283080
GlobalTracer [candidate] (284.256 ms) : 0, 284256
AppSec [baseline] (32.751 ms) : 0, 32751
AppSec [candidate] (32.468 ms) : 0, 32468
Debugger [baseline] (66.765 ms) : 0, 66765
Debugger [candidate] (66.966 ms) : 0, 66966
Remote Config [baseline] (650.914 µs) : 0, 651
Remote Config [candidate] (628.652 µs) : 0, 629
Telemetry [baseline] (9.014 ms) : 0, 9014
Telemetry [candidate] (8.961 ms) : 0, 8961
Flare Poller [baseline] (3.783 ms) : 0, 3783
Flare Poller [candidate] (3.781 ms) : 0, 3781
section iast
crashtracking [baseline] (1.19 ms) : 0, 1190
crashtracking [candidate] (1.18 ms) : 0, 1180
BytebuddyAgent [baseline] (796.363 ms) : 0, 796363
BytebuddyAgent [candidate] (791.131 ms) : 0, 791131
GlobalTracer [baseline] (258.079 ms) : 0, 258079
GlobalTracer [candidate] (256.758 ms) : 0, 256758
AppSec [baseline] (35.193 ms) : 0, 35193
AppSec [candidate] (33.495 ms) : 0, 33495
Debugger [baseline] (64.534 ms) : 0, 64534
Debugger [candidate] (65.798 ms) : 0, 65798
Remote Config [baseline] (596.276 µs) : 0, 596
Remote Config [candidate] (570.848 µs) : 0, 571
Telemetry [baseline] (8.626 ms) : 0, 8626
Telemetry [candidate] (8.486 ms) : 0, 8486
Flare Poller [baseline] (3.654 ms) : 0, 3654
Flare Poller [candidate] (3.59 ms) : 0, 3590
IAST [baseline] (27.159 ms) : 0, 27159
IAST [candidate] (27.055 ms) : 0, 27055
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 4 performance regressions! Performance is the same for 14 metrics, 16 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (1.173 ms) : 1162, 1184
. : milestone, 1173,
iast (3.203 ms) : 3156, 3249
. : milestone, 3203,
iast_FULL (5.904 ms) : 5844, 5963
. : milestone, 5904,
iast_GLOBAL (3.693 ms) : 3634, 3752
. : milestone, 3693,
profiling (1.919 ms) : 1903, 1935
. : milestone, 1919,
tracing (1.848 ms) : 1832, 1864
. : milestone, 1848,
section candidate
no_agent (1.178 ms) : 1166, 1190
. : milestone, 1178,
iast (3.121 ms) : 3083, 3159
. : milestone, 3121,
iast_FULL (5.806 ms) : 5749, 5864
. : milestone, 5806,
iast_GLOBAL (3.436 ms) : 3385, 3486
. : milestone, 3436,
profiling (1.861 ms) : 1846, 1875
. : milestone, 1861,
tracing (1.8 ms) : 1784, 1815
. : milestone, 1800,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (17.955 ms) : 17770, 18140
. : milestone, 17955,
appsec (18.934 ms) : 18739, 19128
. : milestone, 18934,
code_origins (17.755 ms) : 17580, 17930
. : milestone, 17755,
iast (17.637 ms) : 17467, 17808
. : milestone, 17637,
profiling (18.679 ms) : 18492, 18865
. : milestone, 18679,
tracing (17.818 ms) : 17640, 17996
. : milestone, 17818,
section candidate
no_agent (19.014 ms) : 18823, 19205
. : milestone, 19014,
appsec (20.222 ms) : 20013, 20431
. : milestone, 20222,
code_origins (17.541 ms) : 17366, 17716
. : milestone, 17541,
iast (17.602 ms) : 17430, 17774
. : milestone, 17602,
profiling (19.363 ms) : 19168, 19558
. : milestone, 19363,
tracing (18.096 ms) : 17915, 18276
. : milestone, 18096,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (1.47 ms) : 1458, 1482
. : milestone, 1470,
appsec (3.639 ms) : 3424, 3854
. : milestone, 3639,
iast (2.211 ms) : 2146, 2276
. : milestone, 2211,
iast_GLOBAL (2.254 ms) : 2188, 2320
. : milestone, 2254,
profiling (2.064 ms) : 2011, 2117
. : milestone, 2064,
tracing (2.044 ms) : 1992, 2096
. : milestone, 2044,
section candidate
no_agent (1.474 ms) : 1463, 1486
. : milestone, 1474,
appsec (3.735 ms) : 3514, 3956
. : milestone, 3735,
iast (2.213 ms) : 2148, 2279
. : milestone, 2213,
iast_GLOBAL (2.253 ms) : 2188, 2319
. : milestone, 2253,
profiling (2.061 ms) : 2008, 2113
. : milestone, 2061,
tracing (2.051 ms) : 1999, 2103
. : milestone, 2051,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.59.0-SNAPSHOT~f13392e02e, baseline=1.59.0-SNAPSHOT~671cc8cdbf
dateFormat X
axisFormat %s
section baseline
no_agent (14.956 s) : 14956000, 14956000
. : milestone, 14956000,
appsec (14.881 s) : 14881000, 14881000
. : milestone, 14881000,
iast (18.166 s) : 18166000, 18166000
. : milestone, 18166000,
iast_GLOBAL (17.831 s) : 17831000, 17831000
. : milestone, 17831000,
profiling (14.976 s) : 14976000, 14976000
. : milestone, 14976000,
tracing (14.819 s) : 14819000, 14819000
. : milestone, 14819000,
section candidate
no_agent (15.476 s) : 15476000, 15476000
. : milestone, 15476000,
appsec (14.529 s) : 14529000, 14529000
. : milestone, 14529000,
iast (18.245 s) : 18245000, 18245000
. : milestone, 18245000,
iast_GLOBAL (17.967 s) : 17967000, 17967000
. : milestone, 17967000,
profiling (15.611 s) : 15611000, 15611000
. : milestone, 15611000,
tracing (14.691 s) : 14691000, 14691000
. : milestone, 14691000,
|
a1e732b to
e6e93dd
Compare
…A_*_HOME env vars
ad686ad to
bea5f86
Compare
BUILDING.md
Outdated
| #### macOS | ||
|
|
||
|
|
||
| Use your JDK manager ([mise](https://mise.jdx.dev/), [sdkman](https://sdkman.io/), etc.) or set-up the required JDKs with `brew` | ||
|
|
||
| * Install the required JDKs using `brew`: | ||
| ```shell | ||
| brew install --cask zulu@8 zulu@11 zulu@17 zulu@21 zulu graalvm/tap/graalvm-ce-java17 |
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.
| #### macOS | |
| Use your JDK manager ([mise](https://mise.jdx.dev/), [sdkman](https://sdkman.io/), etc.) or set-up the required JDKs with `brew` | |
| * Install the required JDKs using `brew`: | |
| ```shell | |
| brew install --cask zulu@8 zulu@11 zulu@17 zulu@21 zulu graalvm/tap/graalvm-ce-java17 | |
| #### macOS | |
| Use your JDK manager ([mise](https://mise.jdx.dev/), [sdkman](https://sdkman.io/), etc.) or set up the required JDK with `brew`. | |
| * Install the required JDK using `brew`: | |
| ```shell | |
| brew install --cask zulu@21 |
Here and below, should we remove instructions on downloading other JDKs / GraalVMs since they're no longer required? Alternatively, we could keep all of the original instructions and add a note that only JDK 21 installation is required.
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.
Actually graalvm is still needed for some smoke tests
BUILDING.md
Outdated
| ``` | ||
| Example: `graalvm-ce-java17-22.3.1` | ||
| * Use this version in the following command to fix the GraalVM installation by [removing the quarantine flag](https://www.graalvm.org/latest/docs/getting-started/macos/): | ||
| ``` |
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.
The formatting got a bit messed up here. Maybe keep the original suggestion and include the * Add... line after
docs/how_instrumentations_work.md
Outdated
| ``` | ||
|
|
||
| Running tests that require JDK-21 will require the `JAVA_21_HOME` env var set and can be done like this: | ||
| Running tests that require JDK-21 to be installed, use the `-PtestJvm=21` flag, for example: |
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.
| Running tests that require JDK-21 to be installed, use the `-PtestJvm=21` flag, for example: | |
| Run tests that require JDK-21 to be installed by using the `-PtestJvm=21` flag. For example: |
sarahchen6
left a comment
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.
Wow this is cool! Left a few small wording nits, but otherwise looks good!
I was wondering -- how were you able to test this? My guess is by removing local Java installations and confirming that non-JDK 21 tests can still run.
| } | ||
|
|
||
| // "stable" is calculated as the largest X found in JAVA_X_HOME | ||
| // "stable" is calculated as the largest Java version found via toolchains or JAVA_X_HOME |
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.
misc: I still do not like stable name... can we rename it to latest or something similar?
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.
I'd rather not change it in this PR.
AlexeyKuznetsov-DD
left a comment
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.
LGTM. Just have a question - would it be possible to have JVMs from various vendors to test for example Oracle 8 and Zulu on my laptop?
PerfectSlayer
left a comment
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.
Erf, I just notice my review was left in draft. Submitting now
docs/how_instrumentations_work.md
Outdated
| ``` | ||
|
|
||
| Running tests that require JDK-21 will require the `JAVA_21_HOME` env var set and can be done like this: | ||
| Running tests that require JDK-21 to be installed, use the `-PtestJvm=21` flag, for example: |
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.
🎯 suggestion: How will it work to run with a specific JVM?
For JDK version based criteria, we can set the version number. But what happens for the case where you want to run against a JDK installed in a specific path?
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.
The toolchain will be automatically downloaded with a number or (zulu11) e.g. :
// -------------> 1% CONFIGURING [10s]
// > :dd-java-agent > :dd-java-agent:agent-iast > Downloading toolchain from URI https://api.foojay.io/disco/v3.0/ids/dc9716d3e8f8baffc294fb182ceca2c2/redirect > https://api.foojay.io/disco/v3.0/ids/dc9716d3e8f8baffc294fb182ceca2c2/redirect > 80 MiB/104.5 MiB downloaded
That said provisioning won't work oracle as it cannot be provisonned automatically, so it has to be installed manually in a location that Gradle know about.
About JVM paths, this was already handled with the dd-trace-java.test-jvm-contraints plugin, so passing -PtestJvm=/path/to/fancy/jvm/ will use that jvm for tests.
dd-trace-java/buildSrc/src/main/kotlin/datadog/gradle/plugin/testJvmConstraints/TestJvmSpec.kt
Lines 159 to 161 in f13392e
| private val Project.javaToolchains: JavaToolchainService | |
| get() = | |
| extensions.getByName("javaToolchains") as JavaToolchainService |
CONTRIBUTING.md
Outdated
| For IntelliJ IDEA, we suggest the following settings and plugin. | ||
|
|
||
| The default JVM to build and run tests from the command line should be Java 8. | ||
| The default JVM to build and run tests from the command line should be Java 21. |
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.
❔ question: Is this still needed? Won't it be picked automatically?
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.
Your comment made me realize the gradle-daemon-jvm.properties was missing something for this to work. Next commit fixes that.
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.
I'm removing this line as it shouldn't be a concern anymore.
BUILDING.md
Outdated
| #### macOS | ||
|
|
||
|
|
||
| Use your JDK manager ([mise](https://mise.jdx.dev/), [sdkman](https://sdkman.io/), etc.) or set-up the required JDKs with `brew` |
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.
💭 thought: This is confusing as the next section gives explanations for brew. We choose to explain the brew way as it is the simplest as already available tools for most MacOS users not accustomed to Java development. For the others, they already know how to install a JDK and use their preferred tool.
BUILDING.md
Outdated
| * The `JAVA_8_HOME`, `JAVA_11_HOME`, `JAVA_17_HOME`, `JAVA_21_HOME`, `JAVA_25_HOME`, and `JAVA_GRAALVM17_HOME` must point to their respective JDK location. | ||
| * The JDK 8 `bin` directory must be the only JDK on the PATH (e.g. `$JAVA_8_HOME/bin`). | ||
| * The `JAVA_HOME` environment variable may be unset. If set, it must point to the JDK 8 location (same as `JAVA_8_HOME`). | ||
| * JDK 21 must be installed and `JAVA_HOME` must point to it. |
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.
❔ question: Is the JAVA_HOME really needed? Can't gradle find it?
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.
You're right it can be removed.
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.
Actually you made me realize that, the gradle-daemon-jvm.properties were missing an important part in order to make gradle auto-provision the Gradle JDK, the download urls need to be present.
./gradlew updateDaemonJvm --jvm-version=21
After that i works
> Starting Daemon > Downloading toolchain from URI https://api.foojay.io/disco/v3.0/ids/46949723aaa20c7b64d7ecfed7207034/redirect > 56 MiB/190.5 MiB downloaded
BUILDING.md
Outdated
| * The JDK 8 `bin` directory must be the only JDK on the PATH (e.g. `$JAVA_8_HOME/bin`). | ||
| * The `JAVA_HOME` environment variable may be unset. If set, it must point to the JDK 8 location (same as `JAVA_8_HOME`). | ||
| * JDK 21 must be installed and `JAVA_HOME` must point to it. | ||
| * Other JDK versions (8, 11, 17, 25) will be automatically downloaded by Gradle's toolchain resolver. |
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.
🎯 suggestion: This is not a requirement. We can remove it for clarity. We don't give precision on Maven or Gradle versions being downloaded either.
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.
OK
| check-jvm "JAVA_21_HOME" "21" | ||
| check-jvm "JAVA_25_HOME" "25" | ||
| check-jvm "JAVA_GRAALVM17_HOME" "17" | ||
| echo "ℹ️ Other JDK versions will be automatically downloaded by Gradle toolchain resolver." |
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.
Looks like some part of my review were lost at some point but I had a comment about the onboarded script that was lost.
Here is a follow up PR to address what I originally posted: #10384
We can proceed with yours first and I will update / resolve conflict on mine after.
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.
Actually, is this even necessary as JAVA_HOME is not strictly required anymore ?
| return launcher | ||
| } as Closure<Provider<JavaLauncher>> | ||
|
|
||
| ext.getLazyJavaHomeFor = (int javaVersionInteger, boolean hasNativeImage = false) -> { |
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.
Eventually this should be moved to the test-jvm-contraints plugin.
URLs are acquired from the foojay resolver, when running this task ./gradlew updateDaemonJvm --jvm-version=21 See https://docs.gradle.org/8.14.3/userguide/gradle_daemon.html#configuring_the_daemon_jvm
|
|
||
| Gradle should automatically detect the JDK in usual places. As a fallback it can automatically provision them. | ||
|
|
||
| ### Install git |
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.
Actually shouldn't this be moved before gradle
What Does This Do
This PR allows to completely get rid of setting up JAVA_X_HOME on local dev environment.
The
stablespecial word has special handling though, that is using Gradle internal toolchain registry, and fallback to env vars. However, this mainly affect CI.Motivation
Easier project setup
Additional Notes
After this PR, setting up JAVA_X_HOME won't be needed anymore. One can use
-PtestJvm=..., e.g.-PtestJvm=11,-PtestJvm=graalvm17, ...