SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.43k stars 193 forks source link

Mixin dependencies cause issues with 1.12 #186

Closed LeafHacker closed 7 years ago

LeafHacker commented 7 years ago

Mixin depends on guava 17.0, gson 2.2.4 and commons-io 2.4.

This is fine with Minecraft 1.11, however 1.12 requires newer versions of these libs: guava 21.0, gson 2.8.0, commons-io 2.5.

This means that if a 1.12 mod depends on mixin, forgegradle will pull in the 1.12 deps and gradle will also load mixin's deps. This potentially causes a number of issues.

I can reliably reproduce the following example:

Mumfrey commented 7 years ago

Is this actually intellij problem though? I'm using 0.6.10 with MC1.12 an ForgeGradle 2.3 but using Eclipse and I'm not experiencing the issue you describe, in fact as far as I can tell the dependencies are evicted correctly by gradle:

------------------------------------------------------------
Root project - LiteLoader
------------------------------------------------------------

apiCompile - Compile classpath for source set 'api'.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

apiRuntime - Runtime classpath for source set 'api'.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

archives - Configuration for archive artifacts.
No dependencies

checkstyle - The Checkstyle libraries to be used for this project.
\--- com.puppycrawl.tools:checkstyle:6.13
     +--- org.apache.commons:commons-lang3:3.4
     +--- antlr:antlr:2.7.7
     +--- org.antlr:antlr4-runtime:4.5.1-1
     +--- commons-beanutils:commons-beanutils:1.9.2
     |    \--- commons-collections:commons-collections:3.2.1
     +--- commons-cli:commons-cli:1.3.1
     \--- com.google.guava:guava:18.0

clientCompile - Compile classpath for source set 'client'.
No dependencies

clientRuntime - Runtime classpath for source set 'client'.
No dependencies

compile - Compile classpath for source set 'main'.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

debugCompile - Compile classpath for source set 'debug'.
No dependencies

debugRuntime - Runtime classpath for source set 'debug'.
No dependencies

default - Configuration for default artifacts.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

deobfCompile
No dependencies

deobfProvided
No dependencies

forgeGradleFfiDeps
No dependencies

forgeGradleGradleStart
No dependencies

forgeGradleMc
\--- net.minecraft:minecraftSrc:1.12

forgeGradleMcDeps
+--- net.minecraft:launchwrapper:1.11
|    +--- org.lwjgl.lwjgl:lwjgl:2.9.1 -> 2.9.4-nightly-20150209
|    |    +--- org.lwjgl.lwjgl:lwjgl-platform:2.9.4-nightly-20150209
|    |    \--- net.java.jinput:jinput:2.0.5
|    |         +--- net.java.jutils:jutils:1.0.0
|    |         \--- net.java.jinput:jinput-platform:2.0.5
|    +--- org.apache.logging.log4j:log4j-core:2.0-beta9 -> 2.8.1
|    |    \--- org.apache.logging.log4j:log4j-api:2.8.1
|    +--- org.ow2.asm:asm-debug-all:5.0.3
|    +--- org.apache.logging.log4j:log4j-api:2.0-beta9 -> 2.8.1
|    \--- net.sf.jopt-simple:jopt-simple:4.5 -> 5.0.3
+--- com.google.code.findbugs:jsr305:3.0.1
+--- com.mojang:patchy:1.1
+--- oshi-project:oshi-core:1.1
|    +--- net.java.dev.jna:jna:3.4.0 -> 4.4.0
|    \--- net.java.dev.jna:platform:3.4.0
+--- net.java.dev.jna:jna:4.4.0
+--- net.java.dev.jna:platform:3.4.0
+--- com.ibm.icu:icu4j-core-mojang:51.2
+--- net.sf.jopt-simple:jopt-simple:5.0.3
+--- io.netty:netty-all:4.1.9.Final
+--- com.google.guava:guava:21.0
+--- org.apache.commons:commons-lang3:3.5
+--- commons-io:commons-io:2.5
+--- commons-codec:commons-codec:1.10
+--- net.java.jutils:jutils:1.0.0
+--- com.google.code.gson:gson:2.8.0
+--- com.mojang:authlib:1.5.25
|    +--- com.google.code.findbugs:jsr305:2.0.1 -> 3.0.1
|    +--- commons-codec:commons-codec:1.10
|    +--- commons-io:commons-io:2.4 -> 2.5
|    +--- com.google.code.gson:gson:2.8.0
|    +--- org.apache.commons:commons-lang3:3.5
|    +--- org.apache.logging.log4j:log4j-api:2.8.1
|    +--- org.apache.logging.log4j:log4j-core:2.8.1 (*)
|    \--- com.google.guava:guava:21.0
+--- com.mojang:realms:1.10.17
|    +--- org.apache.httpcomponents:httpcore:4.3.2
|    +--- org.apache.httpcomponents:httpclient:4.3.3
|    |    +--- org.apache.httpcomponents:httpcore:4.3.2
|    |    +--- commons-logging:commons-logging:1.1.3
|    |    \--- commons-codec:commons-codec:1.6 -> 1.10
|    +--- org.apache.commons:commons-compress:1.8.1
|    \--- commons-logging:commons-logging:1.1.3
+--- org.apache.commons:commons-compress:1.8.1
+--- org.apache.httpcomponents:httpclient:4.3.3 (*)
+--- commons-logging:commons-logging:1.1.3
+--- org.apache.httpcomponents:httpcore:4.3.2
+--- it.unimi.dsi:fastutil:7.1.0
+--- org.apache.logging.log4j:log4j-api:2.8.1
+--- org.apache.logging.log4j:log4j-core:2.8.1 (*)
+--- com.mojang:text2speech:1.10.3
|    +--- net.java.dev.jna:jna:4.4.0
|    +--- com.google.guava:guava:21.0
|    +--- ca.weblite:java-objc-bridge:1.0.0
|    |    \--- net.java.dev.jna:jna:4.1.0 -> 4.4.0
|    +--- org.apache.logging.log4j:log4j-api:2.0-beta9 -> 2.8.1
|    \--- org.apache.logging.log4j:log4j-core:2.0-beta9 -> 2.8.1 (*)
+--- com.paulscode:codecjorbis:20101023
|    \--- com.paulscode:soundsystem:20120107
+--- com.paulscode:codecwav:20101023
|    \--- com.paulscode:soundsystem:20120107
+--- com.paulscode:libraryjavasound:20101123
|    \--- com.paulscode:soundsystem:20120107
+--- com.paulscode:librarylwjglopenal:20100824
|    +--- com.paulscode:soundsystem:20120107
|    \--- org.lwjgl.lwjgl:lwjgl:2.8.3 -> 2.9.4-nightly-20150209 (*)
+--- com.paulscode:soundsystem:20120107
+--- net.java.jinput:jinput:2.0.5 (*)
+--- org.lwjgl.lwjgl:lwjgl:2.9.4-nightly-20150209 (*)
\--- org.lwjgl.lwjgl:lwjgl_util:2.9.4-nightly-20150209
     \--- org.lwjgl.lwjgl:lwjgl:2.9.4-nightly-20150209 (*)

forgeGradleMcDepsClient
+--- com.paulscode:codecjorbis:20101023
|    \--- com.paulscode:soundsystem:20120107
+--- com.paulscode:codecwav:20101023
|    \--- com.paulscode:soundsystem:20120107
+--- com.paulscode:libraryjavasound:20101123
|    \--- com.paulscode:soundsystem:20120107
+--- com.paulscode:librarylwjglopenal:20100824
|    +--- com.paulscode:soundsystem:20120107
|    \--- org.lwjgl.lwjgl:lwjgl:2.8.3 -> 2.9.4-nightly-20150209
|         +--- org.lwjgl.lwjgl:lwjgl-platform:2.9.4-nightly-20150209
|         \--- net.java.jinput:jinput:2.0.5
|              +--- net.java.jutils:jutils:1.0.0
|              \--- net.java.jinput:jinput-platform:2.0.5
+--- com.paulscode:soundsystem:20120107
+--- net.java.jinput:jinput:2.0.5 (*)
+--- org.lwjgl.lwjgl:lwjgl:2.9.4-nightly-20150209 (*)
\--- org.lwjgl.lwjgl:lwjgl_util:2.9.4-nightly-20150209
     \--- org.lwjgl.lwjgl:lwjgl:2.9.4-nightly-20150209 (*)

forgeGradleMcNatives
+--- org.lwjgl.lwjgl:lwjgl-platform:2.9.4-nightly-20150209
+--- net.java.jinput:jinput-platform:2.0.5
\--- com.mojang:text2speech:1.10.3
     +--- net.java.dev.jna:jna:4.4.0
     +--- com.google.guava:guava:21.0
     +--- ca.weblite:java-objc-bridge:1.0.0
     |    \--- net.java.dev.jna:jna:4.1.0 -> 4.4.0
     +--- org.apache.logging.log4j:log4j-api:2.0-beta9
     \--- org.apache.logging.log4j:log4j-core:2.0-beta9
          \--- org.apache.logging.log4j:log4j-api:2.0-beta9

forgeGradleMcpData
\--- de.oceanlabs.mcp:mcp:1.12

forgeGradleMcpMappings
\--- de.oceanlabs.mcp:mcp_snapshot:20170609-1.11

forgeGradleResolvedDeobfCompile
No dependencies

forgeGradleResovledDeobfProvided
No dependencies

provided
No dependencies

runtime - Runtime classpath for source set 'main'.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

shadow
No dependencies

testCompile - Compile classpath for source set 'test'.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

testRuntime - Runtime classpath for source set 'test'.
\--- org.spongepowered:mixin:0.6.10-SNAPSHOT
     +--- commons-io:commons-io:2.4
     \--- com.google.code.gson:gson:2.2.4

The whole point of declaring what version of a dependency a project uses, is that other projects can evict that version for later versions when necessary, this is a normal aspect of ivy/maven/gradle artefact resolution. It shouldn't be the case that I have to break backward compatibility with 1.11 in order to support something which requires newer deps when those deps will naturally evict their older counterparts.

I can't reproduce this issue in Eclipse, or using gradle runClient so I have to assume it's an issue with IntelliJ.

Mumfrey commented 7 years ago

Paging @Minecrell as he uses IntelliJ and might have a solution to this.

stephan-gh commented 7 years ago

As far as I know this issue affects both Eclipse and IntelliJ IDEA and is due to the way ForgeGradle sets up its dependencies: It uses separate dependency configurations for Minecraft dependencies, forgeGradleMcDeps and a few others as you can see above. These configurations are then added manually to the compile classpath, and the module classpath of Eclipse and IntelliJ IDEA.

Unfortunately, adding the configurations separately to the classpath bypasses Gradle's logic to evict older versions of dependencies. It applies the logic when resolving a configuration, not when computing the final classpath. You can see this above, Mixin brings in commons-io:commons-io:2.4 in the compile configuration, and ForgeGradle has commons-io:commons-io:2.5 in its forgeGradleMcDeps configuration. These should be also both on your Eclipse classpath.

Gradle's solution for this problem is configuration inheritance: you can make a configuration extend another. For example, the following build script will automatically have only the latest version of Guava and Gson on the classpath:

apply plugin: 'java'

repositories {
    mavenCentral()
}

configurations {
    deps
    compile {
        extendsFrom deps
    }
}

dependencies {
    compile 'com.google.guava:guava:17.0'
    deps 'com.google.guava:guava:22.0'
    deps 'com.google.code.gson:gson:2.8.1'
}

Currently, ForgeGradle is not using this approach. Consequently the dependencies are added twice to the classpath. It hasn't caused major problems (up to now) so I never investigated it further.

Without changes in ForgeGradle, the only solution is to either explicitly set the newer dependency versions to the compile configuration or to exclude specific transitive dependencies from the Mixin dependency. This is likely why it's working in Liteloader: The Guava dependency is excluded in its build script.

Mumfrey commented 7 years ago

This is likely why it's working in Liteloader: The Guava dependency is excluded in its build script.

Right that would make sense. So basically ForgeGradle is defeating normal eviction behaviour, and I hadn't noticed in my setup because that's presumably happened in the past and I manually excluded it).

So I assume that basically means we have 4 choices:

Of those choices, I'm not sure which one I dislike the least. I disagree on principal with changing declared dependencies in an upstream project to satisfy a downstream one, especially in a way which breaks backward compatibility. I mean, it kind of defeats the whole purpose of dependency eviction in the first place!

Hm, I just thought of another (still not ideal) approach:

Personally I'm leaning towards the 3rd and 4th options as the most sensible approach, because if it's FG that's breaking this then it would make sense to fix it in FG.

stephan-gh commented 7 years ago

I've patched this in VanillaGradle a while ago (because the PROVIDED mapping didn't work anymore with ForgeGradle's approach), it shouldn't be too hard to port it to ForgeGradle. I'll take a look in the next days and see if I can make a PR to fix it.

Mumfrey commented 7 years ago

Thanks dude, in the mean time I assume that @LeafHacker can fix his issue by manually evicting guava from mixin the same way I do in liteloader.

LeafHacker commented 7 years ago

Don't think I mentioned it, but in this particular project I was using the net.minecraftforge.gradle.tweaker-client plugin. Although of course it would be better if this was fixed for all of forgegradle.

I'll look into using a manual eviction workaround for now, thanks.

EDIT: This work around works fine:

compile('org.spongepowered:mixin:0.6.10-SNAPSHOT') {
        exclude module: 'launchwrapper'
        exclude module: 'guava'
        exclude module: 'gson'
        exclude module: 'commons-io'
}
Mumfrey commented 7 years ago

Don't think I mentioned it, but in this particular project I was using the net.minecraftforge.gradle.tweaker-client plugin. Although of course it would be better if this was fixed for all of forgegradle.

So am I, liteloader is a standalone project so it uses that plugin too. I'm going to consider this issue resolved, pending a more permanent solution in FG.