Improve native-image executable discovery with explicit/convention launcher distinction - #845
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
|
Thank you for signing the OCA. |
f6c2d68 to
048b68d
Compare
|
Hey @jormundur00, @hadryyassine, @vjovanov. Can someone check this? |
jormundur00
left a comment
There was a problem hiding this comment.
Thanks for the PR, and sorry for the late review.
I think we should distinguish explicit native-image launcher selection from a launcher selected by convention.
With the current change, enabling toolchain detection makes the native binary inherit the Java plugin toolchain first. If that Java toolchain is a normal JDK without bin/native-image, the locator will fail there and will not try GRAALVM_HOME/JAVA_HOME, because env fallback is only entered when detection is disabled or no launcher is present.
I do not think we should silently fall back when the user explicitly configured graalvmNative.binaries.<name>.javaLauncher; that should fail with a clear message if it does not contain native-image.
But when the launcher is selected by convention, either from JavaVersion.current() or from the Java plugin toolchain, fallback should remain available if that launcher does not supply native-image. Otherwise a normal Java compilation toolchain can accidentally block a valid GraalVM in GRAALVM_HOME/JAVA_HOME.
Could we structure the fix so issue #542 is handled by adding the current Gradle JVM home as a fallback, and so convention-selected toolchains only win when they actually contain native-image? That would preserve explicit user intent while keeping the existing environment fallback behavior.
Separately, I would adjust the new functional test. It hard-codes JavaLanguageVersion.of(25), so it only works on machines where Gradle can discover/provision a JDK 25. The existing Gradle functional-test setup pins the test launcher/toolchain to Java 17 and existing tests generally use JavaVersion.current() or environment-provided GraalVM paths. Could we avoid the fixed 25, either by comparing the native plugin launcher to the Java plugin toolchain provider directly, adding an explicit availability assumption, or covering the fallback behavior directly instead?
|
Hi @jormundur00, thanks for the careful review! I've addressed your concerns in the latest commit:
Let me know if you'd like any further changes! |
jormundur00
left a comment
There was a problem hiding this comment.
Hey @vpelikh, thanks for the timely response and updates.
I've done a second round of reviews and pointed out more inconsistencies/things that need to be changed. Please take another look when you have the time.
vpelikh
left a comment
There was a problem hiding this comment.
@jormundur00, thanks again for review. Now PR has become much more polished.
|
Oh, I see failed tests. Will address them soon. |
jormundur00
left a comment
There was a problem hiding this comment.
Thanks for the updates. I think this is moving in the right direction for the toolchainDetection = true path, but as implemented I don't think it fully address the original issue as reported. The original reproducer relies on the JDK running Gradle being used by default when JAVA_HOME and GRAALVM_HOME are unset; this PR still requires either toolchainDetection = true or an explicit javaLauncher.
I'm on the fence whether we should open a separate issue (regarding toolchain problems) and link it to this PR (and then I'd try to address the original reporters issue), or just leave it as it is.
On second look, I think you should open a separate issue for this toolchain detection mismatch, and link this PR (and |
52a5092 to
2c06f19
Compare
…nDiscoveryTest Signed-off-by: Vasily Pelikh <2010720+vpelikh@users.noreply.github.com>
|
@jormundur00, you mentioned opening a new issue to discuss the toolchain detection logic. Just to be clear – should that new issue be:
I want to make sure I open the right type of issue. Thanks! |
I was thinking more of the latter (point 2.), but let's wait till Monday for opening this issue, as I'd like to discuss this PR with my team on a triaging meet on Monday. |
|
@jormundur00, while we wait, can you approve the workflow? I want to see if the tests in CI have been fixed. |
|
Hey @jormundur00. In this PR I have added the The test passes successfully on my macOS, but it fails on the Linux CI (as shown in the logs). I would like to reproduce the failure locally in a Linux environment to debug it, but I'm not sure how to properly set up and run the functional tests in such a setup. Could you please share some guidance?
docker run --rm --platform linux/amd64 -v "$PWD":/workspace -w /workspace gradle:9.0.0-jdk21 \
bash -c "apt update && apt install -y openjdk-17-jdk && \
export JAVA_HOME=/opt/java/openjdk && \
export JAVA_HOME_17_X64=/usr/lib/jvm/java-17-openjdk-amd64 && \
export GRAALVM_HOME=/opt/java/openjdk && \
./gradlew :native-gradle-plugin:functionalTest -DgradleVersion=9.0.0 --tests ToolchainDiscoveryTest"Any hints on what I might be missing?
I want to ensure my changes are correct and that the test will pass once the CI runs. Thank you very much for your help! |
There was a problem hiding this comment.
Sorry for the delayed response. We did a separate round of reviews after the latest updates and found the additional issues below.
Please rebase onto the latest master before the next update, so the PR is evaluated against the current CI environment and JDK-25 setup.
@vpelikh, replying to the Linux/Docker question: we do not maintain a supported Docker image or script that faithfully mirrors a GitHub-hosted runner, so I would not spend more time trying to reproduce the runner exactly. The closest useful local setup is Linux with Gradle running on the workflow's matrix JDK and a separate GraalVM JDK 25 configured for Native Image:
./gradlew :native-gradle-plugin:functionalTest \
-DgradleVersion=9.0.0 \
--tests org.graalvm.buildtools.gradle.ToolchainDiscoveryTest \
--no-daemon --infoJAVA_HOME_17_X64 and common.repo.url should not be necessary for this test. If you do not have access to Linux locally, that is fine: once you push the rebase and test adjustment, I can run the CI for you; the functional-test workflow runs on Linux by default.
|
We recently added the Please update
The implementation should preserve whether a launcher is explicit or convention-selected, so environment fallback applies only to the convention case. Please add coverage for: explicit missing launcher fails; convention launcher missing Native Image falls back to Please cite §gradle/FS-native-invocation.1 from the affected source, connect the functional tests to that behavior, and run If the specification work is troublesome, let me know—I can prepare it in a separate follow-up PR. |
|
@jormundur00 @vjovanov. The failing checks fail on the same pre-test setup task: This looks like a transient network issue on the runners (the download from GitHub fails), unrelated to this PR. A re-run of the failed jobs should resolve it. |
|
Hey @jormundur00, could you please do a final review as @vjovanov (and me :)) suggested? Also, could you please restart the CI tests? Some of them failed last time due to a flaky network error. Thanks! |
|
Hey @vpelikh, I'm currently (and the previous week) on vacation and don't have the necessary tooling to provide a proper review. I'll take a look next Monday when I'll be back to work. |
|
@jormundur00, thanks for letting me know, and sorry for the ping. Enjoy your vacation — no rush at all. I'll be happy to wait for your review next week. Have a great time! |
jormundur00
left a comment
There was a problem hiding this comment.
Apologies for the wait, and thank you for addressing all the previous comments. The earlier provenance, fallback-input, diagnostics, public-convention, and mixed-JDK issues are now fixed. The focused tests and grund check pass.
I found one final blocker: launcher provenance affects task execution but is not tracked as a task input. If a convention-selected launcher lacks native-image, the task can successfully fall back to GRAALVM_HOME. When that same launcher is subsequently assigned explicitly, nativeCompile remains UP-TO-DATE instead of rerunning and failing according to §FS-native-invocation.1.1.
Please model the explicit/convention provenance as a task input for both nativeCompile and metadataCopy, and add a two-build regression test covering that transition.
Once this is addressed and the tests pass, I believe the PR will be ready to merge.
b4556d3 to
c359646
Compare
Thanks for review once again! Addressed in c359646. |
jormundur00
left a comment
There was a problem hiding this comment.
Thanks for the latest update. The production change now tracks launcher provenance as a task input for both nativeCompile and metadataCopy, and the original stale UP-TO-DATE issue is fixed in the focused local runs.
I found two remaining blockers:
JavaLauncherPropertyprevents the plugin from loading on Gradle 8.4 because its bytecode references the newerSupportsConventionAPI.- The new provenance regression fixture does not reliably select the fake JDK and is not Windows-compatible, causing
ToolchainDiscoveryTestfailures on Gradle 9.x and across platforms.
The Windows LayeredApplicationFunctionalTest failures are unrelated to this PR and are being addressed in a separate PR.
Once the two issues above are addressed and the relevant matrix jobs pass, I believe this PR will be ready to merge.
|
Hey @jormundur00 — I'd like your input on a trade-off before I finalize the ContextYour P1 guidance was: "avoid implementing the evolving
This removes the 8.4-incompatible The trade-offThis convention-flag approach handles the provenance cases except for one spec'd behavior — covered by our def launcher = main.javaLauncher.get() // reads the convention value
main.javaLauncher.set(launcher) // re-assigns the same value explicitly
then:
main.javaLauncherExplicit.get() == true // expected by the spec (§1.1/§1.2)Reading the convention ( So the realistic options are:
I'd rather not unilaterally decide, since it affects the executable-discovery spec (§FS-native-invocation.1.1/1.2). Which do you prefer — relax the "read-then-reassign" edge case, or keep a small interception layer to preserve it exactly? |
|
@vpelikh I prefer preserving the full read-then-set semantics. An explicit A small runtime proxy around the real |
|
Blocker P1 (Gradle 8.4 /
Honestly, I'm not fully proud of this runtime-proxy approach and would be glad to change it if you have a cleaner suggestion — let me know and I'll fix it. Blocker P2 (provenance regression fixture) — fixed: The fixture now:
The One note on P2: I changed the build-1 assertion to verify |
|
Addressed test failure in 55e1c3a. @jormundur00 run pipelines please. |
jormundur00
left a comment
There was a problem hiding this comment.
Thanks for the update. The original Gradle 8.4 SupportsConvention linkage is fixed: the plugin now applies successfully on Gradle 8.4, and the generated JavaLauncherProperty bytecode no longer references that API. The provenance regression also passes locally on Gradle 9.6.1 and with configuration cache on Gradle 9.0.
I found three remaining blockers:
- The runtime proxy does not fully preserve Gradle
Propertymutation and exception semantics, including incorrectly clearing explicit provenance onunsetConvention(). - The regression fixture forces Gradle 8.4 to run on Java 25 and fails before plugin application.
- The new
gu.cmdfixtures still contain POSIX shell scripts and are not executable on Windows.
grund check, plugin inspections, and the existing provenance unit tests pass.
Since this PR has been rebased onto a snapshot NBT version that does not test Gradle 8.4 in CI, please test the final changes manually with Gradle 8.4 before merging.
Once these three issues are addressed and the relevant matrix jobs pass, I believe the PR will be ready to merge.
|
Blocker 1 (runtime proxy must preserve Gradle
I also added a provenance unit test: "unsetting the convention does not clear an explicit launcher assignment" — it sets a launcher, calls Blocker 2 (regression fixture forced Gradle 8.4 onto Java 25) — fixed: The fixture no longer sets Blocker 3 (POSIX The two test fixtures that write a fake Verification:
As you requested, since Gradle 8.4 is not exercised by the current snapshot CI matrix, I ran the Gradle 8.4 functional test manually (above) before finalizing. |
jormundur00
left a comment
There was a problem hiding this comment.
Thanks for the update. The Gradle 8.4 daemon compatibility fix works locally: the exact provenance regression passes on Gradle 8.4 when GRAALVM_HOME is a canonical path. The configuration-cache regression on Gradle 9.0, the two gu tests on Linux with Gradle 8.4, the existing unit suite, and grund check also pass.
I found four remaining blockers:
- The runtime proxy still changes normal Property behavior for value(null), exception propagation, and equality.
- The generated Windows native-image.cmd only examines the first argument and will not handle the normal build invocation where -o appears later.
- Plugin inspections fail because the new JavaDoc contains non-ASCII em dashes.
- The provenance fixture still fails when GRAALVM_HOME is a symlink.
GitHub currently shows only the OCA check, so the Windows behavior has not been exercised by CI.
Once these issues are addressed and the relevant checks pass, I believe the PR will be ready to merge.
aa7cafb to
78ff90a
Compare
|
Blocker 1 (runtime proxy still changed normal
The previously-reviewed semantics are preserved: I added a dedicated unit test, Blocker 2 (installed Windows The generated Blocker 3 (non-ASCII em dashes broke the ASCII checkstyle rule) — fixed: The two JavaDoc lines in Blocker 4 (symlinked
Outstanding for CI: the Windows branch of blocker 2's |
jormundur00
left a comment
There was a problem hiding this comment.
I re-reviewed the latest changes locally. All of my previous blocking findings have been addressed, and I found no new blocking issues.
The focused proxy tests, plugin inspections, the symlinked GRAALVM_HOME regression, and the full ToolchainDiscoveryTest suite on Gradle 8.4 all pass locally. The current GitHub CI matrix is also green.
This is ready to merge from my side.
…uncher distinction
78ff90a to
0076b89
Compare
Thank you for the final review. I realize this went through quite a few follow-up rounds, and I appreciate your patience throughout all of them – sorry it took so many iterations to get everything right. I've also squashed the commits and rebased onto the latest master, so the branch should be clean and up to date. Let me know if there's anything else you need before merging. Glad we finally got there! |
|
To help get this PR merged faster, I attempted to fix the Windows-specific test problems directly and pushed commit The fix makes the fake batch executable valid for Locally, the full |
|
@jormundur00 thank you for fixing Windows tests! NativeConfigRepoFunctionalTest failure looks like a transient network issue: A re-run of the failed job should resolve it. |
Summary
This PR makes native-image executable discovery safer and more predictable by distinguishing an explicitly configured launcher from one selected by convention, so the plugin always builds against a launcher that actually provides
native-image, and fails with a clear message when it cannot.Before this PR
toolchainDetection = true) used the toolchain-resolved JDK as the launcher without checking whether that JDK containednative-image, leading to confusing failures.javaLauncherlackednative-image, the build failed silently or with an opaque downstream error.GRAALVM_HOME/JAVA_HOMEfallback chain.After this PR
Explicit
javaLauncher— treated as authoritative.graalvmNative.binaries.<name>.javaLauncherand that launcher does not containbin/native-image, the build fails immediately with a message naming the launcher and its installation path. No fallback, nogu install.Convention launcher (toolchain detection) — used only when it can supply Native Image.
toolchainDetection = true, toolchain detection is used as thejavaLauncherconvention, sograalvmNative.binaries.main.javaLauncherstays queryable.bin/native-image. If it does, that launcher is used.GRAALVM_HOME→JAVA_HOME→ the Gradle JVMjava-home), optionally installingnative-imageviaguthere. A convention-selected launcher never blocks a working GraalVM in the environment. §FS-native-invocation.1.5gu install native-imageruns only on this environment-fallback path, never against a user-configured or toolchain installation (a convention launcher is never mutated in place).native-image, the build fails with a diagnostic that lists the probed paths and the selected location source.Net effect: the plugin finds a usable
native-imagewhenever one exists on the machine, fails fast with actionable messages when it does not, and always respects an explicit choice over automation.Spec documentation
FS-native-invocation.1(Executable discovery) documented innative-gradle-plugin/docs/functional/native-image-invocation.md:Test coverage
NativeImageExecutableLocatorTest(unit tests):native-image→GradleExceptionwith diagnostic messagenative-image→ falls back toGRAALVM_HOMEToolchainDiscoveryTest(functional tests,src/functionalTest):explicit javaLauncher overrides toolchain(§1.1) — explicit launcher used, not GRAALVM_HOMEtoolchain takes precedence over GRAALVM_HOME env var when running nativeCompile(§1.2)disabling toolchainDetection uses GRAALVM_HOME fallback(§1.3)native-image found in alternative GraalVM home when GRAALVM_HOME has no native-image and no gu(§1.3 cross-home fallback) — whenGRAALVM_HOMElacksnative-imageand has nogu, the locator probesJAVA_HOMEand resolvesnative-imagefrom theregu installs native-image when not found(§1.4)gu installation failure falls back to error message(§1.5)convention launcher provides native-image when no explicit launcher set(§1.2) — toolchain convention used when user sets no explicit launchercompatibility mode detects and uses convention fallback launcher(§1.2/§1.6) — toolchain-detection active + convention fallback message on both runschanging JAVA_HOME re-runs nativeCompile when native-image comes from JAVA_HOME fallback(§1.3) — env fallback candidates are task inputs: switching JAVA_HOME forces re-run, not UP-TO-DATEunchanged environment keeps nativeCompile UP-TO-DATE(§1.3) — control: proves the JAVA_HOME swap is what forces the re-runFixes #542