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

Update to gradle 8.8 and use new daemon toolchain feature #67

Closed
wants to merge 4 commits into from

Conversation

lukebemish
Copy link

With neogradle 7.0.138, java 17 or higher is now required to run neogradle; thanks to a new feature in gradle 8.8, we can require that gradle run with a specific java version. This PR upgrades neogradle to 7.0.138, gradle to 8.8, and sets the required gradle toolchain to J21. I picked J21 instead of J17 -- which is the minimum version needed -- because this version is an exact match and I thought it more likely that this would be installed in a locatable location than J17.

Some feedback is needed on this. Is this too likely to cause confusion about being unable to locate a JVM, compared to the confusion it alleviates about NG running with the wrong JVM? Should it use J17 for gradle -- the minimum needed by NG -- or J21 -- the current version used by MC which may be more likely to be present on the system (though I'm truly not entirely sure of that)?

@Shadows-of-Fire
Copy link

I think this is fine, regardless of the minimum requirement actually set by NG, users will need J21 around to actually do anything, and we recommend that J21 be installed (rather than J17).

@lukebemish
Copy link
Author

Users do not actually need J21 around to do anything because gradle will install it for you if it's missing. But it won't do that if this setting is set and it can't launch at all. Hence why I wasn't sure of the best approach.

@Technici4n
Copy link
Member

What does the error message look like when the toolchain can't be found?

@neoforged-automation neoforged-automation bot added the needs rebase This Pull Request needs to be rebased before being merged label Nov 11, 2024
@neoforged-automation
Copy link

@lukebemish, this pull request has conflicts, please resolve them for this PR to move forward.

@Technici4n
Copy link
Member

I found the daemon toolchain annoying last time I tried to use it, but forgot the details.

@shartte
Copy link

shartte commented Nov 11, 2024

It does not actually download anything. They're adding this in future versions as far as I know.

@lukebemish
Copy link
Author

lukebemish commented Nov 11, 2024

At least for now, I'd say we shouldn't use daemon toolchains in the MDK -- it'd stop someone with only, for instance, a system level J23 installation from running the MDK, since it wouldn't be able to download J21 till after the first gradle invocation, and the requirement is strict. If there were a way to make it, instead, "prefer this version if it's present" or something that'd be awesome, but it doesn't work that way.

@sciwhiz12
Copy link
Member

The base branch of this PR has since been updated to Gradle 8.11, so this PR should be changed to only enable the Daemon JVM discovery feature.

@lukebemish
Copy link
Author

As mentioned above, that is likely a bad idea given the current behavior of that feature (aka, it would cause someone with only a newer java to be unable to set up a dev env, despite it currently importing fine and working fine by downloading the newer java later). As such, for now I am going to close this PR.

@lukebemish lukebemish closed this Nov 18, 2024
@lukebemish lukebemish deleted the gradle-java-toolchain branch November 18, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase This Pull Request needs to be rebased before being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants