neoforged / MDK

The Mod Developer Kit - this is where you start if you want to develop a new mod
https://github.com/NeoForgeMDKs
185 stars 59 forks source link

Make MDK show info on how to switch to full gradle sources #65

Closed TelepathicGrunt closed 3 months ago

TelepathicGrunt commented 4 months ago

This will significantly improve error reporting in the buildscripts and lead to a better development experience.

Before: image

After: image

TelepathicGrunt commented 4 months ago

Size wise, it's not crazy bigger and the benefits are worth it imo. image

Unzipped image

TelepathicGrunt commented 4 months ago

also, should we add

tasks.named('wrapper', Wrapper).configure {
    //Define wrapper values here so as to not have to always do so when updating gradlew.properties
    gradleVersion = '8.7'
    distributionType = Wrapper.DistributionType.ALL
}

to the build.gradle? We can the tell people to change gradle version here and run the wrapper task to update without needing them to dig into wrapper folder

Matyrobbrt commented 4 months ago

That's generally discouraged, to update Gradle one shall run ./gradlew wrapper --gradle-version=<version> twice.

Martmists-GH commented 4 months ago

I'm not sure if there's a big need for more than the API sources, especially if it triples the file size. Groovy syntax highlighting should already be complete highlighting without it.

lukebemish commented 4 months ago

Why using the full sources artifact would change any of that... that's weird, and honestly makes me concerned about what IntelliJ is doing because the available info should be the same. Furthermore, it shouldn't use the sources at all for highlighting in theory -- all the info needed is available in the binary. That said: I'm not certain about the improvement this provides. When testing locally, I seem to get substantially more highlighting that seen above: image This is on latest IntelliJ; changing to the distribution with sources does not change this. I am utterly unable to reproduce what TelepathicGrunt shows. I would like to confirm what's actually going on there before increasing the size of the downloaded gradle distribution when there's really no reason to expect it to change the IDE support of the buildscript and when this effect does not seem reproducible. Are you sure that the effect you observed isn't just from IntelliJ finally figuring out what's going on after repeatedly re-importing the project?

TelepathicGrunt commented 4 months ago

@lukebemish perhaps it was that needing reimport project. But do you have an alternative solution then? It seems intellij for me has issues often with gradle. @thiakil was the one who recommended I try this so it seems the mek team had issues with gradle often and switching to -all worked for them. And strange worked for me when I did the switch

lukebemish commented 4 months ago

I'm uncertain. Deleting the .idea cache and reimporting tends to be reliable enough for me. I am simply hesitant to go with a solution that feels like waving our hands over a magic cauldron and hoping, especially when doing so has a specific adverse effect on the amount of stuff gradle downloads (and thus on import times)

thiakil commented 4 months ago

all the info needed is available in the binary.

Technically, yes, but that's not how the IntelliJ PSI works, it doesn't do much with binary files (especially when it comes to generics which gradle is heavy on). Using the All distro would also bring in documentation in the java/groovy-doc

What happens when you ctrl/cmd/middle-click one of the items, like configurations?

Do you have shared indexes enabled and downloaded?

lukebemish commented 4 months ago

ctrl-clicking, say, configurations for me takes me to Project.class (not the sources), as expected as I do not have the sources present. Wiping IntelliJ's caches (which should toss out any sort of shared index?) doesn't seem to change anything. Furthermore, IntelliJ's IDE support, as a general rule, does use the binaries extensively, not just the source -- IntelliJ's more general groovy support, which I have to assume its gradle support is based on, does not get any more IDE support if you enable source downloading than if you leave that off.

thiakil commented 4 months ago

does not get any more IDE support if you enable source downloading than if you leave that off.

In gradle land , this has not been my experience

lukebemish commented 4 months ago

Well given that it's just how stuff works in every other language support in IntelliJ where you don't need sources at all for stuff like syntax highlighting, do we have a good reason to believe that the presence of the sources is actually helping IntelliJ with anything here (and there could be, gradle gets special treatment all the time) or is this just a magic cauldron to wave our hands over? Basically: if this is changing something I'd want more of a reason why because (a) it's obviously not having an effect in all cases, and (b) there's no reason, given how IntelliJ acts with other language support, to expect it to change anything.

thiakil commented 4 months ago

you don't need sources at all for stuff like syntax highlighting

Well we're not really talking syntax highlighting, it's the resolution that's helped, i.e. the grey text with underlines

IMO adding the docs on hover or ctrl-clicking is a benefit even if it doesn't help resolution (it keeps insisting on finding unzipped sources on my system, so can't verify either way at this point).

Does it cause harm to use the all distro? If not, why be such a stick in the mud....

lukebemish commented 4 months ago

It causes a significant increase in size on disk, a significant increase in initial project load time, and an extra flag to keep in mind when updating the wrapper. And by "syntax highlighting" I was including stuff like the grey highlighted bits -- the inference of what methods are available and whatnot is not source-dependent in any other language in IntelliJ, so I'd want to confirm that that's actually what's going on here, because it would be strange for that to be the case, instead of just calling it magic.

TelepathicGrunt commented 4 months ago

Got a fresh laptop. Opened a project with -bin and checked build.gradle with latest intellij version

image image image

With -all

image image image

While the original issue of no syntax highlighting was not caused by -bin, the extra documentation from -all is very tempting and may be useful for newcomers to be able to understand each gradle part with official documentation. Gonna adjust the PR to make it more clear to users that the ALL source is being pulled and how to switch to BIN if desired

Shadows-of-Fire commented 4 months ago

The file size increase is fairly nontrivial (the gradle docs included in the -all distro are 300MB). Granted, 300MB is not exactly a huge amount of disk space in this age.

a significant increase in initial project load time

Not sure what you would be deeming "significant" here. The download for the -all distro took about a minute, and that's with the very slow download speed provided by Gradle.

lukebemish commented 4 months ago

I would consider an extra minute wait a significant increase in project load time when the full load time on initial load is measured in minutes.

TelepathicGrunt commented 4 months ago

I figured keeping it bin but have the comment and wrapper task say that all is an option for more documentation would be fine. This lets people that mess with Gradle be able to switch to all by seeing that is an option they can do while those that don't touch gradle can leave it as bin.