Dependencies that produce the same jar name overwrite each other

[amandeepg]

If we have a IntelliJ plugin gradle module that has 2 dependencies that we dont' control (so we don't control their .jar name), then the JARs in the plugin overwrite each other (in build/idea-sandbox/plugins/DemonstrateSameNameJarProblem/lib/core.jar) so when we run the plugin, only one "core" jar is available, and that unexpectedly causes a java.lang.NoClassDefFoundError exception.

Demonstrated in this small example: https://github.com/amandeepg/demonstrate-intellij-same-jar-problem. ./gradlew runIde then opening any Java file runs the line marker provider which causes the NoClassDefFoundError.

I've temporarily worked around this issue by:

prepareSandbox {
    doLast {
        def sandboxPluginDir = "${intellij.sandboxDirectory}/plugins/${getPluginName()}/lib"
        delete fileTree(sandboxPluginDir)
        copy {
            from configurations.default.allArtifacts.files
            from configurations.default
            exclude 'tools.jar'
            into sandboxPluginDir
            duplicatesStrategy DuplicatesStrategy.FAIL
            def count = 0
            rename { count++ + it }
        }
    }
}

[zolotov]

what's the expected behavior?

[amandeepg]

In this case, I made the line marker provider a no-op aside from some System.out printing, so the expected behavior in this case is to simply not crash when the line marker is used, and the line marker runs on every java file, so the expected behavior is no crash when any java file is inspected.

[zolotov]

Sorry, I don't understand. What's the expected behavior about overwriting jars? What exactly you want me to do here on the gradle-intellij-plugin side?

[amandeepg]

When a plugin using gradle-intellij-plugin depends on 2 different libraries with the same JAR name, only 1 jar is included in the plugin. There should be 2, one for each dependency.

[zolotov]

There should be 2, one for each dependency.

I get this but what you want me to do about it on gradle-intellij-plugin side?

Renaming files will likely break task incrementality. As far as I understand https://github.com/amandeepg/demonstrate-intellij-same-jar-problem is artificial recreated. Could you describe the real use case how it may happen, please? By "it" I mean clashed names and inability to change them.

[amandeepg]

I'm not sure on the best solution for it. The workaround we've performed is to use renaming but I'm sure it can be solved in other ways as well. Perhaps nested folders? Not sure. Does renaming break incrementality if you rename it to the same name every time, say if you were to hash the file and use the hash as the name?

The real use case is that I have an IntelliJ plugin that depends on a few different features, where each feature is split up into multiple gradle modules, often named the same, like "feature1:core", "feature2:core".

[zolotov]

Perhaps nested folders?

They won't be loaded by IDEA

Does renaming break incrementality if you rename it to the same name every time

You cannot guarantee the ordering of files you're copying, thus you cannot be sure that you rename the very same file all the time.

often named the same, like "feature1:core", "feature2:core".

Probably dependencies on subprojects should be always renamed to something like feauture1-core.jar

[cesar1000]

I think that the question of whether project artifacts should use globally unique names is not quite relevant to this issue: the reality is that Gradle uses the project name as the default base name for an artifact, which is not guaranteed to be unique. In a setup described above where the plugin depends on :library1:core and :library2:core the IntelliJ plugin creates a corrupt sandbox that causes the deployed plugin to crash with a nondescript error that's really hard to diagnose. This may not be a frequent issue, but it's not that unusual in a complex modular plugin.

I think that the IntelliJ plugin should, at the very least, detect the issue and fail gracefully. A better option would be to rename the jar files to use a unique name at the time when they are copied to the sandbox – after all, jar file names are expected to be used within a repository structure where only the full directory path is meant to be unique. The option of numbering files doesn't strike me as unreasonable: as far as I know, the Sync task is neither cacheable nor incremental, and the up-to-date check doesn't depend on whether the output is deterministic. I understand that you may want to avoid renames in the general case when there are not clashes, in which case you can just attach a counter suffix to a file only when a clash is detected.

[zolotov]

I think that the IntelliJ plugin should, at the very least, detect the issue and fail gracefully.

I agree.

Sync task is neither cacheable nor incremental

It's both cacheable (if there are no custom actions) and incremental at some level. Inconsistent rename will lead to false not-up-to-date results.

[cesar1000]

It's both cacheable (if there are no custom actions) and incremental at some level. Inconsistent rename will lead to false not-up-to-date results.

I don't think that's accurate. Gradle's documentation makes it explicit that the Copy task is not cacheable since its faster to execute than to cache. Also, I don't see any evidence in the implementation of the task of any use of incremental inputs.

The task does declare inputs and outputs, as required for supporting up-to-date checks. However, why would a non-deterministic output cause up-to-date checks to fail? If the inputs and outputs haven't changed since the last run, the task will be skipped. If any of them has changed, the task will be re-run.

As I mentioned, you may want to make your output deterministic just because as a matter of hygiene, in which case you can process the files in a fixed order and add the suffix only when a clash is detected.

[zolotov]

Gradle's documentation makes it explicit that the Copy task is not cacheable since its faster to execute than to cache.

I looked at https://github.com/gradle/gradle/blob/master/subprojects/core/src/main/java/org/gradle/api/tasks/AbstractCopyTask.java#L91 and it still makes me think it's cacheable despite what documentation says.

The non-deterministic result is bad for incrementality no matter whether the non-determined task is cacheable/incremental. A task doesn't live in isolation. There might be different reasons (not only jars changed) why prepareSandbox task was restarted. There also might be other tasks that depend on its results (e.g. plugin dependency like intellij { plugins = [project(':A')]) and that might be updated not because of a real change of that jars but because of nondeterministic nature of that prepareSandbox task.

[cesar1000]

Well, the documentation says it's not cacheable, the @Cacheable annotation is missing, and Gradle Enterprise describes the task as not cacheable, so I don't know what else to say :)

Like I said, I understand that it's not ideal that results are not deterministic – which is why I suggested that you order the files and only add a suffix if you find a clash. That's a simple solution that addresses all concerns.

Ultimately, this is a report of a corrupt deployment that's caused by a broken assumption in prepareSandbox: that a jar file name is unique. We have proposed a solution that works around the issue, and it is your choice whether you want to fix it or not.

[zolotov]

Well, the documentation says it's not cacheable, the @Cacheable annotation is missing, and Gradle Enterprise describes the task as not cacheable, so I don't know what else to say :)

I believe that it's unrelated to the issue, but it's still an interesting question :) Cacheable annotation is not required:

• Caching for individual task instances can be enabled and disabled via {@link TaskOutputs#cacheIf(String, Spec)} or disabled via {@link TaskOutputs#doNotCacheIf(String, Spec)}, even if the task type is not annotated with {@link CacheableTask}

from
https://github.com/gradle/gradle/blob/71b11a2a4b6a274e2382167e8314fe81a2574f51/subprojects/core-api/src/main/java/org/gradle/api/tasks/CacheableTask.java#L32

We have proposed a solution that works around the issue, and it is your choice whether you want to fix it or not.

Well, yeah, I've got the proposal from the message by @amandeepg, but I'm sorry, but I don't understand what you're trying to convince me in right now.

[cesar1000]

I don't understand what you're trying to convince me in right now.

Let me break it down for you:

• The task is not cacheable, and I invite you to go confirm it yourself instead of schooling me on how cacheability works on Gradle.
• You rejected @amandeepg's proposed solution on the grounds that it's indeterministic. I explained that it doesn't matter in practice, but gave you a simple solution in case you're still concerned.

[zolotov]

Thank you for the clarification.

I invite you to go confirm it yourself instead of schooling me on how cacheability works on Gradle.

I didn't mean to school anyone. In fact, the opposite, I'd like to learn the truth even if it's not directly connected to the issue. I see the mismatch between what you took from the website and what I see in the Gradle's code, and if you're an expert in Gradle I'm ready to believe in your words about cachaebility, and I would love to review your PR that fixes the issue, preferably with a test that covers my concerns about up-to-date checks.

You rejected @amandeepg's proposed solution

The only strong opinion about the suggestions that I see here was about nested folders-approach. I cannot find any rejections in the thread.

[cesar1000]

I don't claim to be an expert in Gradle! The documentation literally explains why the Copy task is not cacheable, and I find the explanation is entirely reasonable :).

As for testing, what kind of test would you like to see? As far as I can tell, a test checking that the deployment of a single plugin collects all its dependencies and renames them deterministically in the case of a clash should do the trick.

[zolotov]

The documentation literally explains why the Copy task is not cacheable, and I find the explanation is entirely reasonable :).

I trust code more than the documentation website and the code looks like it is cacheable unless there is a custom action. I trust runtime even more, but I'm too lazy to check how it works in the runtime :(

As for testing, what kind of test would you like to see? As far as I can tell, a test checking that the deployment of a single plugin collects all its dependencies and renames them deterministically in the case of a clash should do the trick.

With the deterministic rename, a separate test for an up-to-date check is not needed.

For anyone who would like to contribute, see org.jetbrains.intellij.tasks.PrepareSandboxTask#configurePlugin method. This is the one that collects all runtime dependencies and copies them to the sandbox.

Probably for the failing strategy, this change will be enough:

Index: src/main/groovy/org/jetbrains/intellij/tasks/PrepareSandboxTask.groovy
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/groovy/org/jetbrains/intellij/tasks/PrepareSandboxTask.groovy	(revision 87ed2aaa5eb2395f8e264d63ec2274b72821b448)
+++ src/main/groovy/org/jetbrains/intellij/tasks/PrepareSandboxTask.groovy	(date 1592474858346)
@@ -1,6 +1,7 @@
 package org.jetbrains.intellij.tasks
 
 import org.gradle.api.file.CopySpec
+import org.gradle.api.file.DuplicatesStrategy
 import org.gradle.api.file.FileCollection
 import org.gradle.api.plugins.JavaPlugin
 import org.gradle.api.tasks.*
@@ -20,6 +21,7 @@
     List<Object> pluginDependencies = []
 
     PrepareSandboxTask() {
+        duplicatesStrategy = DuplicatesStrategy.FAIL
         configurePlugin()
     }

[cesar1000]

Ok, let's get this fixed then :). I did some more research and I checked directly with Gradle, and this is what I found: the order of the input to {{PrepareSandboxTask}} is guaranteed already to be deterministic. The task takes a runtime classpath as input, and Gradle guarantees that classpaths use a stable order, since the order of jar files in a classpath affects runtime behavior. The order is fully determined by the order in which dependencies are added to a configuration at configuration time. Using this assumption, adding an incremental counter to the file name in the case of a clash should be deterministic. We should be able to validate this assumption in a unit test by showing that switching the order in which the dependencies are added to the configuration also switch the order of the jars in the output. What do you think? Does that sound like an acceptable solution?

[zolotov]

The task takes a runtime classpath as input, and Gradle guarantees that classpaths use a stable order, since the order of jar files in a classpath affects runtime behavior.

good point

What do you think? Does that sound like an acceptable solution?

It does sound like a solution. Thanks for the investigation!