diffplug / goomph

IDE as build artifact
Apache License 2.0
130 stars 30 forks source link

ZipMisc exception with existing directories #84

Closed hacki11 closed 5 years ago

hacki11 commented 6 years ago

I have a module where ZipMisc has a problem saying a path would be duplicate.

there are some resources within my module like:

model/fileA.xyz model/fileB.xyz

During the BndManifestPlugin ZipMisc.modify(jarTask.getArchivePath(), toModify, Predicates.alwaysFalse()); it wants to add the files in the following order:

model/fileA.xyz model/fileB.xyz model/

The last entry is the problem here.

Stacktrace
Execution failed for task ':module:p2'.
> java.util.zip.ZipException: duplicate entry: model/

* Try:
Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Exception is:
org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':module:p2'.
    at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:110)
    at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:77)
    at org.gradle.api.internal.tasks.execution.OutputDirectoryCreatingTaskExecuter.execute(OutputDirectoryCreatingTaskExecuter.java:51)
    at org.gradle.api.internal.tasks.execution.SkipUpToDateTaskExecuter.execute(SkipUpToDateTaskExecuter.java:59)
    at org.gradle.api.internal.tasks.execution.ResolveTaskOutputCachingStateExecuter.execute(ResolveTaskOutputCachingStateExecuter.java:54)
    at org.gradle.api.internal.tasks.execution.ValidatingTaskExecuter.execute(ValidatingTaskExecuter.java:59)
    at org.gradle.api.internal.tasks.execution.SkipEmptySourceFilesTaskExecuter.execute(SkipEmptySourceFilesTaskExecuter.java:101)
    at org.gradle.api.internal.tasks.execution.FinalizeInputFilePropertiesTaskExecuter.execute(FinalizeInputFilePropertiesTaskExecuter.java:44)
    at org.gradle.api.internal.tasks.execution.CleanupStaleOutputsExecuter.execute(CleanupStaleOutputsExecuter.java:91)
    at org.gradle.api.internal.tasks.execution.ResolveTaskArtifactStateTaskExecuter.execute(ResolveTaskArtifactStateTaskExecuter.java:62)
    at org.gradle.api.internal.tasks.execution.SkipTaskWithNoActionsExecuter.execute(SkipTaskWithNoActionsExecuter.java:59)
    at org.gradle.api.internal.tasks.execution.SkipOnlyIfTaskExecuter.execute(SkipOnlyIfTaskExecuter.java:54)
    at org.gradle.api.internal.tasks.execution.ExecuteAtMostOnceTaskExecuter.execute(ExecuteAtMostOnceTaskExecuter.java:43)
    at org.gradle.api.internal.tasks.execution.CatchExceptionTaskExecuter.execute(CatchExceptionTaskExecuter.java:34)
    at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter$1.run(EventFiringTaskExecuter.java:51)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:317)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:309)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor.execute(DefaultBuildOperationExecutor.java:185)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:97)
    at org.gradle.internal.operations.DelegatingBuildOperationExecutor.run(DelegatingBuildOperationExecutor.java:31)
    at org.gradle.api.internal.tasks.execution.EventFiringTaskExecuter.execute(EventFiringTaskExecuter.java:46)
    at org.gradle.execution.taskgraph.DefaultTaskExecutionGraph$ExecuteTaskAction.execute(DefaultTaskExecutionGraph.java:262)
    at org.gradle.execution.taskgraph.DefaultTaskExecutionGraph$ExecuteTaskAction.execute(DefaultTaskExecutionGraph.java:246)
    at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker$1.execute(DefaultTaskPlanExecutor.java:136)
    at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker$1.execute(DefaultTaskPlanExecutor.java:130)
    at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker.execute(DefaultTaskPlanExecutor.java:201)
    at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker.executeWithTask(DefaultTaskPlanExecutor.java:192)
    at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker.run(DefaultTaskPlanExecutor.java:130)
    at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:63)
    at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:46)
    at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:55)
Caused by: com.diffplug.common.base.Errors$WrappedAsRuntimeException: java.util.zip.ZipException: duplicate entry: model/
    at com.diffplug.common.base.Errors.asRuntime(Errors.java:408)
    at com.diffplug.common.base.Errors.rethrowErrorAndWrapOthersAsRuntime(Errors.java:132)
    at com.diffplug.common.base.Errors$Rethrowing.lambda$new$10(Errors.java:327)
    at com.diffplug.common.base.Errors.lambda$wrap$5(Errors.java:220)
    at com.diffplug.common.base.Errors.run(Errors.java:210)
    at com.diffplug.gradle.osgi.BndManifestPlugin.lambda$null$3(BndManifestPlugin.java:113)
    at org.gradle.api.internal.AbstractTask$TaskActionWrapper.execute(AbstractTask.java:794)
    at org.gradle.api.internal.AbstractTask$TaskActionWrapper.execute(AbstractTask.java:761)
    at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter$1.run(ExecuteActionsTaskExecuter.java:131)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:317)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:309)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor.execute(DefaultBuildOperationExecutor.java:185)
    at org.gradle.internal.operations.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:97)
    at org.gradle.internal.operations.DelegatingBuildOperationExecutor.run(DelegatingBuildOperationExecutor.java:31)
    at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeAction(ExecuteActionsTaskExecuter.java:120)
    at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:99)
    ... 30 more
Caused by: java.util.zip.ZipException: duplicate entry: model/
    at com.diffplug.gradle.ZipMisc.modify(ZipMisc.java:129)
    at com.diffplug.gradle.ZipMisc.modify(ZipMisc.java:145)
    at com.diffplug.gradle.osgi.BndManifestPlugin.lambda$null$2(BndManifestPlugin.java:117)
    at com.diffplug.common.base.Errors.lambda$wrap$5(Errors.java:218)
    ... 42 more

My fix would be to check if the entry already exists in ZipMisc:

} else if (!toOmit.test(entry.getName())) {
    if(!names.contains(entry.getName())) {
        names.add(entry.getName());
        // if it isn't being modified, just copy the file stream straight-up
        ZipEntry newEntry = new ZipEntry(entry);
        newEntry.setCompressedSize(-1);
        zipOutput.putNextEntry(newEntry);
        copy(zipInput, zipOutput);
        }
    }
nedtwigg commented 6 years ago

Hmm... It's always been a little unclear to me whether model/ belonged in the zip file, or if it would just be implicitly created because it's a prerequisite for model/fileA.xyz. The ZipMisc model ignores parent directories entirely - it creates them if needed when extracting, and doesn't worry about them when encoding.

The nice thing about ZipMisc, as it is, is that it tries to modify the zip file as little as possible - it just copies entries over wholesale. To make the "names" work, we'll have to first read the whole file upfront to populate names.

It looks to me like model/ is being implicitly added to the zip file when we add model/fileA.xyz, which is what causes explicitly adding model/ to fail. Seems that ZipMisc's previous input has always been either this:

model/fileA.xyz
model/fileB.xyz

or

model/
model/fileA.xyz
model/fileB.xyz

and now, for whatever reason, for the first time it's seeing

model/fileA.xyz
model/fileB.xyz
model/

I could be wrong in my diagnosis, but if I'm correct, it seems to me that the easiest fix might be this:

} else if (!toOmit.test(entry.getName())) {
  // if it isn't being modified, just copy the file stream straight-up
  try {
    ZipEntry newEntry = new ZipEntry(entry);
    newEntry.setCompressedSize(-1);
    zipOutput.putNextEntry(newEntry);
    copy(zipInput, zipOutput);
  } catch (java.util.zip.ZipException e) {
    if (e.getMessage().startsWith("duplicate entry")) {
      // no worries, sometimes input zips have strange orders, see https://github.com/diffplug/goomph/issues/84
    } else {
      throw e;
    }
}
hacki11 commented 6 years ago

further investigation had shown that the order is not the problem. there are indeed duplicate entries within the zip file (ZipOutputStream provided by gradle does support this). One can set the behavior at jar { duplicatesStrategy = DuplicatesStrategy.INCLUDE/WARN/EXCLUDE }.

My duplicates arise due the following setting:

if (project.convention.findPlugin(JavaPluginConvention)) {
        // Change the output directory for the main and test source sets back to the old path
        sourceSets.main.output.classesDir = new File(buildDir, "classes/main")
        sourceSets.main.output.resourcesDir = new File(buildDir, "classes/main")
        sourceSets.test.output.classesDir = new File(buildDir, "classes/test")
        sourceSets.test.output.resourcesDir = new File(buildDir, "classes/test")
    }

I did this because IntelliJ does not support the different folders for languages (java, groovy, etc.) like gradle needs. Because i want to use the platform testrunner instead of gradle i need this setting.

Though not directly an issue with ZipMisc here, but for me it would be nice to have. It is your decision what to do - with your go i will provide a pr of course.

nedtwigg commented 6 years ago

Sounds like your zip file is gonna have duplicate entries for every file? What's the platform testrunner? The eclipse built-in one? Making this work seems less productive than making it so that you don't need your zips to have duplicate entries.

nedtwigg commented 5 years ago

Closing due to inactivity. Feel free to reopen.