Fix trailing path separator in `minecraft_classpath` lazy token

This PR fixes the issue when configuring multiple minecraftLibrary configurations leaves trailing path separator in minecraft_classpath lazy token.

Introduction

This issue arose when I was trying to configure multiple custom minecraftLibrary configurations and running DependencyManagementExtension#configureMinecraftLibraryConfiguration afterwards. Gradle threw the following error:

Invalid module name: '' is not a Java identifier

After some tricky debugging, I was able to trace the root of the issue. It was all about how DependencyManagementExtension#configureMinecraftLibraryConfiguration was setting up the minecraft_classpath lazy token.

Reproduction

In order to understand how and why this happens, I will introduce a step-by-step explanation.

When the minecraft_classpath is not set, we are assigning libraries, joined by a path separator. This is called when a default minecraftLibrary is created and configured by the ForgeGradle itself. https://github.com/MinecraftForge/ForgeGradle/blob/4839283b5536db670936327d2b14bdaf66aa0dbd/src/userdev/java/net/minecraftforge/gradle/userdev/DependencyManagementExtension.java#L109
When the minecraft_classpath is set, we are joining the previous token and the new one via a path separator. This is called when some other configuration is being configured (remember, minecraftLibrary was configured earlier and minecraft_classpath is no longer null) https://github.com/MinecraftForge/ForgeGradle/blob/4839283b5536db670936327d2b14bdaf66aa0dbd/src/userdev/java/net/minecraftforge/gradle/userdev/DependencyManagementExtension.java#L111
Now what if we haven't used our custom configuration yet? What if we haven't defined any dependencies using that configuration? Correct, librariesSupplier.get() will be an empty string, hence oldToken.get() + File.pathSeparator + librariesSupplier.get() will basically become oldToken.get() + File.pathSeparator, leaving us with a trailing path separator. Java "assumes" that the next line contains a library path when there is none. That is why we get Invalid module name: '', as there is no string.

Solution

The solution is quite simple: we trim the trailing path separator if there is any.

- runConfig.lazyToken("minecraft_classpath", () -> oldToken.get() + File.pathSeparator + librariesSupplier.get());
+ runConfig.lazyToken("minecraft_classpath", () -> String.join(
+         File.pathSeparator,
+         oldToken.get(),
+         librariesSupplier.get()
+ ).replaceFirst(File.pathSeparator + "$", ""));

Conclusion

This PR allows developers to create custom minecraftLibrary configuration with first-party support. As for now, developers have to loop though all runs and remove trailing path separators by themselves.

Testing

I've tested this PR locally and can confirm that it successfully removes trailing path separator from the minecraft_classpath token.

I have some questions why this even a problem. If your configuration is empty and it gets read here as empty, then it will never be updated later? So that error with the empty module name is just a side effect of having an unfilled configuration.

Additionally, this check is not implemented well. There is no point to do it this way or use slow regex on a string that tends to be very long when we know where the character is to be. I instead would much prefer expanding the old lambda to query the old supplier, store it in a variable, query the new supplier, store it in a variable, and if it .trim().isEmpty(), then return the old string by itself, otherwise the joined strings.

Please join https://discord.gg/forge and go to the #gradle channel so we can discuss this further. Thanks.

MinecraftForge / ForgeGradle