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

Open
wants to merge 4 commits into
base: 1.21
Choose a base branch
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.

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.

4 participants