Skip to content

#31659 Add support for relative java installation paths for Daemon toolchain#32869

Merged
ljacomet merged 1 commit into
gradle:releasefrom
vmadalin:daemon-toolchain-supports-relative-path-custom-locations
Mar 28, 2025
Merged

#31659 Add support for relative java installation paths for Daemon toolchain#32869
ljacomet merged 1 commit into
gradle:releasefrom
vmadalin:daemon-toolchain-supports-relative-path-custom-locations

Conversation

@vmadalin

@vmadalin vmadalin commented Mar 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Context

This was reported on #31659 and in addition represents a misalignment between Daemon toolchain and tasks/project toolchain where the last one already supports relative paths, while for Daemon toolchain fails with the following exception message:

java.lang.UnsupportedOperationException: Cannot convert relative path ../jdk/jdk17/linux to an absolute file.

For Daemon toolchain the DaemonClientToolchainServices end up instantiating LocationListInstallationSupplier using the provided FileResolver being IdentityFileResolver class this is because on Launcher context most of the services aren't available. However, the IdentityFileResolver doesn't support relative paths and instead BaseDirFileResolver should be used instead which requires the baseDir to resolve the relative path. To obtain project dirctory from the launcher context the BuildLayoutParameters.currentDir is used being this aligned with how project gradle-daemon-jvm.properties are read as a criteria for the Daemon JVM.

NOTE: All of this aligns with the task/project toolchain behavior for custom toolchain when org.gradle.java.installations.paths is defined under GRADLE_HOME instead of the project.

  • Absolute path: ✅
  • Relative path to the project: ✅
  • Relative path to GRADLE_HOME: ❌

Tests

Contributor Checklist

  • Review Contribution Guidelines.
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
  • Provide unit tests (under <subproject>/src/test) to verify logic.
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes.
  • Ensure that tests pass sanity check: ./gradlew sanityCheck.
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

@vmadalin vmadalin requested a review from a team as a code owner March 27, 2025 16:57
@vmadalin vmadalin requested review from a team and donat March 27, 2025 16:57
@bot-gradle bot-gradle added from:contributor PR by an external contributor to-triage labels Mar 27, 2025
ljacomet
ljacomet requested changes Mar 28, 2025
View reviewed changes

@ljacomet ljacomet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @vmadalin ,

This look good to me.

However, to land in 8.14, it needs to target the release branch.

Could you rebase your commit on top of that branch?

@vmadalin vmadalin changed the base branch from master to release March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team as a code owner March 28, 2025 15:45
@vmadalin vmadalin requested review from a team March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team as a code owner March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team as a code owner March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team March 28, 2025 15:45
@vmadalin vmadalin requested review from a team, alllex and bamboo as code owners March 28, 2025 15:45
@vmadalin vmadalin requested review from a team March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team as a code owner March 28, 2025 15:45
@vmadalin vmadalin requested a review from a team March 28, 2025 15:45
@vmadalin vmadalin requested review from a team as code owners March 28, 2025 15:45
@vmadalin vmadalin requested a review from ghale March 28, 2025 15:45
@vmadalin
Daemon toolchain supports relative paths for java installations
c701334 
Signed-off-by: Madalin Valceleanu <vmadalin@google.com>
(cherry picked from commit cf5f11231c4ed229bf7859d04267697838861619)
@vmadalin vmadalin force-pushed the daemon-toolchain-supports-relative-path-custom-locations branch from cf5f112 to c701334 Compare March 28, 2025 16:02
@wolfs wolfs removed the request for review from a team March 28, 2025 16:04
@vmadalin vmadalin requested a review from ljacomet March 28, 2025 16:04
@jvandort

jvandort commented Mar 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

It feels odd to me to support relative paths here. Obviously, the relative path needs to be calculated relative to some path, but which one? You could argue a number of base paths are equally relevant including:

  • The cwd
  • The path of the project

The CWD probably does not make much sense, but in what case does the path of the project make sense?

Do we really want to support/promote the pattern where JDKs are installed relative to the project? This seems to tie in builds tightly to a certain directory structure of a given machine, which seems unwise

@ljacomet ljacomet removed request for a team, alllex and bamboo March 28, 2025 16:50
@ljacomet

ljacomet commented Mar 28, 2025

Copy link
Copy Markdown
Member

we have use cases of projects setup exactly like that.
We (accidentally) supported relative paths in toolchain, where we should not have.

Now that we have the right scope for those services, this gives a clear contract - that should be documented:

  • The path is relative to the folder of the settings file.

@ljacomet ljacomet added this pull request to the merge queue Mar 28, 2025
@bot-gradle bot-gradle added this to the 8.14 RC1 milestone Mar 28, 2025
Merged via the queue into gradle:release with commit d3b099e Mar 28, 2025
@ljacomet ljacomet added in:daemon in:toolchains Java Toolchains and removed to-triage labels Mar 31, 2025
@ljacomet ljacomet self-assigned this Mar 31, 2025
@ljacomet

ljacomet commented Mar 31, 2025

Copy link
Copy Markdown
Member

Looks like I approved and merged this a bit too eagerly.

This is not working as intended. When running Gradle inside the lib folder of a generated (by Gradle init) library project, paths are evaluated relative to the execution location and not the folder containing settings.

So I will be reverting those changes.

ljacomet added a commit that referenced this pull request Mar 31, 2025
@ljacomet
Revert "#31659 Add support for relative java installation paths for D…
ff8dd22 
…aemon toolchain (#32869)"

The changes do not work as expected.

Issue #31659
@ljacomet ljacomet mentioned this pull request Mar 31, 2025
Merged
@gradle gradle deleted a comment from bot-gradle Mar 31, 2025
@vmadalin vmadalin mentioned this pull request Mar 31, 2025
Merged
9 tasks
@vmadalin

vmadalin commented Mar 31, 2025

Copy link
Copy Markdown
Contributor Author

This is not working as intended. When running Gradle inside the lib folder of a generated (by Gradle init) library project, paths are evaluated relative to the execution location and not the folder containing settings.

Created #32894 to re-enable it after addressing the missed scenario that somehow I still missed it despite validating with composite builds and using ../../gradlew from another directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljacomet ljacomet ljacomet approved these changes

@donat donat Awaiting requested review from donat donat is a code owner automatically assigned from gradle/bt-tide

@ghale ghale Awaiting requested review from ghale ghale was automatically assigned from gradle/bt-jvm

Assignees

@ljacomet ljacomet

Labels

from:contributor PR by an external contributor in:daemon in:toolchains Java Toolchains

Projects

None yet

Milestone

8.14 RC1

Development

Successfully merging this pull request may close these issues.

4 participants

@vmadalin @jvandort @ljacomet @bot-gradle