fix(core/patcher): incremental compilation

[Floppy012]

Fixes #199 by modifying the classpath and adding the path to the already compiled classes as first entry. This way the compiler loads all the patched classes instead of the original ones from the dependencies. The classpath modification only takes place when compileJava runs an incremental compile.

I initially tried to use Machine Maker's filtered jar but then noticed that the same problem occurs with classes from external libs such as brigadier.

I have not found anything over the last day that would let me solve this in another (less-hacky) way.

I'm also not that familiar with kotlin & gradle so I'm not really familiar with best-practices. If there is anything wrong feel free to @ me or to fix it yourself 😄

(This could also be placed directly in the paper-server build config. Let me know what you prefer)

[jpenilla]

I think instead of adding our classes to the front we should sort the classpath so that anything in an output directory for the current project is first.

Also, there are other reasons we want to switch to compiling against the filtered jar (#194); but we need to do more testing to ensure the filter is 100% correct first, so that can be left for another time if we can solve this by simply sorting the classpath.

[Floppy012]

Also, there are other reasons we want to switch to compiling against the filtered jar (#194); but we need to do more testing to ensure the filter is 100% correct first, so that can be left for another time if we can solve this by simply sorting the classpath.

We'd have to filter all dependency jars. Brigadier is a good example it's a dependency outside of the Minecraft jar. Currently, even when using the filtered jar the problem persists as the compiler takes the classes from the Brigadier dependency over Paper's overrides.

[jpenilla]

Did some testing, hadn't realized you were simply adding things to the classpath as opposed to adding duplicates to the front. I feel like this is kind of risky and could cause problems when recompiling multiple interdependent classes; the fact that we are having this issue means that javac doesn't inherently prefer sources over an existing classpath entry.

Using the filtered jar will probably end up being the right approach in the end. As for patched dependencies, I think the simplest solution would be to move them to their own source set (with incremental compilation disabled), I'll do some investigation into how feasible that is.

[Floppy012]

Did some testing, hadn't realized you were simply adding things to the classpath as opposed to adding duplicates to the front. I feel like this is kind of risky and could cause problems when recompiling multiple interdependent classes; the fact that we are having this issue means that javac doesn't inherently prefer sources over an existing classpath entry.

I'm not really adding anything new to the classpath. The classpath is just ordered with dependencies first. After that comes the directory with the already compiled classes (that's the one I'm adding upfront with this PR). The problem occurs during Incremental compilation because the compiler is only instructed to compile some classes. When those classes import something that is not part of the files the compiler is instructed to compile, then it looks for them in the classpath.

Gradle builds the classpath with local jar files first (hence minecraft.jar is the first entry in the classpath), then maven dependencies, then comes the directory containing previously compiled classes. I have not found anything that would let us change this over an API from gradle other than adding the directory with the compiled classes first.

The only problem is that someone at gradle didn't think about stuff that paper does (i.e. overwriting classes from dependencies). Paper is probably an extreme edge-case from Gradles pov.

Technically, we could pack all those already compiled classes into the filtered jar but that would have to be done every time before the incremental compilation runs and has the same effect as defining the directory upfront.

[jpenilla]

I checked by capturing the compiler args instead this time and yes you're right, it is adding duplicates to the front. After looking closer at how Gradle manages incremental compilation I think this is probably safe.

[Floppy012]

I've applied your suggested changes but wasn't sure about the incremental restriction since you didn't mention it in the review. My intention was to keep the hacky solution out of full recompiles so that if something interferes with this hack it won't break everything. People could disable incremental compilation and would be good to go (even if slow) until the bug gets fixed.

If you want it removed let me know 😄

[jpenilla]

So, while this does allow incremental compilation to run, it seems like our setup is still confusing Gradle's classpath snapshotter - changing any single class results in it recompiling ~2600: Incremental compilation of 2616 classes completed in 10.631 secs.

Will probably end up merging this, but I want to do testing in combination with the filtered jar and some changes to its implementation first.

Also worth noting there is a Gradle issue to track here: gradle/gradle#21255

[Floppy012]

@jpenilla FYI: An upstream fix has just been merged gradle/gradle#27942