This repository has been archived by the owner. It is now read-only.

Get rid of eclipse-collections as incompatible with Android #575

Merged
merged 1 commit into from May 8, 2018

Conversation

Projects
None yet
2 participants
@saudet
Copy link
Member

saudet commented May 7, 2018

Fixes deeplearning4j/deeplearning4j#4330

What changes were proposed in this pull request?

The Android Gradle build throws this kind of exception when eclipse-collections is included in a project:

org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':app:transformDexArchiveWithDexMergerForDebug'.
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:100)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:70)
	at org.gradle.api.internal.tasks.execution.OutputDirectoryCreatingTaskExecuter.execute(OutputDirectoryCreatingTaskExecuter.java:51)
	at org.gradle.api.internal.tasks.execution.SkipUpToDateTaskExecuter.execute(SkipUpToDateTaskExecuter.java:62)
	at org.gradle.api.internal.tasks.execution.ResolveTaskOutputCachingStateExecuter.execute(ResolveTaskOutputCachingStateExecuter.java:54)
	at org.gradle.api.internal.tasks.execution.ValidatingTaskExecuter.execute(ValidatingTaskExecuter.java:60)
	at org.gradle.api.internal.tasks.execution.SkipEmptySourceFilesTaskExecuter.execute(SkipEmptySourceFilesTaskExecuter.java:97)
	at org.gradle.api.internal.tasks.execution.CleanupStaleOutputsExecuter.execute(CleanupStaleOutputsExecuter.java:87)
	at org.gradle.api.internal.tasks.execution.ResolveTaskArtifactStateTaskExecuter.execute(ResolveTaskArtifactStateTaskExecuter.java:52)
	at org.gradle.api.internal.tasks.execution.SkipTaskWithNoActionsExecuter.execute(SkipTaskWithNoActionsExecuter.java:52)
	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.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker$1.run(DefaultTaskGraphExecuter.java:248)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:336)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:328)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor.execute(DefaultBuildOperationExecutor.java:199)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:110)
	at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker.execute(DefaultTaskGraphExecuter.java:241)
	at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker.execute(DefaultTaskGraphExecuter.java:230)
	at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker.processTask(DefaultTaskPlanExecutor.java:123)
	at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker.access$200(DefaultTaskPlanExecutor.java:79)
	at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker$1.execute(DefaultTaskPlanExecutor.java:104)
	at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker$1.execute(DefaultTaskPlanExecutor.java:98)
	at org.gradle.execution.taskgraph.DefaultTaskExecutionPlan.execute(DefaultTaskExecutionPlan.java:626)
	at org.gradle.execution.taskgraph.DefaultTaskExecutionPlan.executeWithTask(DefaultTaskExecutionPlan.java:581)
	at org.gradle.execution.taskgraph.DefaultTaskPlanExecutor$TaskExecutorWorker.run(DefaultTaskPlanExecutor.java:98)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:63)
	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:46)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:55)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.RuntimeException: com.android.build.api.transform.TransformException: java.lang.ArrayIndexOutOfBoundsException
	at com.android.builder.profile.Recorder$Block.handleException(Recorder.java:55)
	at com.android.builder.profile.ThreadRecorder.record(ThreadRecorder.java:104)
	at com.android.build.gradle.internal.pipeline.TransformTask.transform(TransformTask.java:212)
	at sun.reflect.GeneratedMethodAccessor492.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:73)
	at org.gradle.api.internal.project.taskfactory.IncrementalTaskAction.doExecute(IncrementalTaskAction.java:46)
	at org.gradle.api.internal.project.taskfactory.StandardTaskAction.execute(StandardTaskAction.java:39)
	at org.gradle.api.internal.project.taskfactory.StandardTaskAction.execute(StandardTaskAction.java:26)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter$1.run(ExecuteActionsTaskExecuter.java:121)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:336)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor$RunnableBuildOperationWorker.execute(DefaultBuildOperationExecutor.java:328)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor.execute(DefaultBuildOperationExecutor.java:199)
	at org.gradle.internal.progress.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:110)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeAction(ExecuteActionsTaskExecuter.java:110)
	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:92)
	... 32 more
Caused by: com.android.build.api.transform.TransformException: java.lang.ArrayIndexOutOfBoundsException
	at com.android.build.gradle.internal.transforms.DexMergerTransform.transform(DexMergerTransform.java:225)
	at com.android.build.gradle.internal.pipeline.TransformTask$2.call(TransformTask.java:221)
	at com.android.build.gradle.internal.pipeline.TransformTask$2.call(TransformTask.java:217)
	at com.android.builder.profile.ThreadRecorder.record(ThreadRecorder.java:102)
	... 47 more
Caused by: java.lang.ArrayIndexOutOfBoundsException

Let's replace the only thing we use from it (DoubleArrayList) with the version from fastutil that gets pulled in by com.clearspring.analytics:stream anyway.

How was this patch tested?

Unit tests pass.

@saudet saudet requested a review from AlexDBlack May 7, 2018

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented May 7, 2018

Unless I've misunderstood the cause (that being different versions of the same dependency), wouldn't this be solved easier by aligning the DL4J and DataVec versions of Eclipse Collections? That's a 1-line change in DL4J...

https://github.com/deeplearning4j/deeplearning4j/blob/master/pom.xml#L96
https://github.com/deeplearning4j/DataVec/blob/master/pom.xml#L106

Otherwise we're effectively decided with this PR not to use Eclipse Collections in DataVec in the future, as any reintroduction (unless versions are aligned) would break Android again... Eclipse Collections has some nice classes/utils, so I'd like to avoid that unless there's no other option here.

@saudet

This comment has been minimized.

Copy link
Member

saudet commented May 8, 2018

No the cause is eclipse-collections. Adding it as a dependency, even without DataVec or Deeplearning4j, just does not work on Android, period.

@saudet

This comment has been minimized.

Copy link
Member

saudet commented May 8, 2018

We could ask them to fix it and reinclude it when it becomes compatible with Android. They do appear to manage the project on GitHub these days: https://github.com/eclipse/eclipse-collections

@saudet

This comment has been minimized.

Copy link
Member

saudet commented May 8, 2018

Or we could ask Google to fix Android, but these guys are known to be even less responsive to the community...

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented May 8, 2018

So effectively you're effectively telling me we have to entirely and permanently remove Eclipse Collections from all of our libraries if we want to work on Android?! o_O

We use it (only a few places, mainly the primitive collections) in DataVec, Arbiter and DL4J.
But that seems like a fairly extreme measure... and I do have to wonder why we haven't seen this until now, it's not like this dependency was just added recently. And no reported issues on github mentioning android... https://github.com/eclipse/eclipse-collections/issues?utf8=%E2%9C%93&q=is%3Aissue+android

Anyway, do we have any insight beyond that exception stack trace as to why this occurs?

@saudet

This comment has been minimized.

Copy link
Member

saudet commented May 8, 2018

It's been added to DataVec recently yes: a1f288e

The other modules where it's used have simply never been used by anyone on Android, that's all.

A search for Android on the Eclipse Collections repository returns nothing, so it just looks like no one has ever used it on Android: https://github.com/eclipse/eclipse-collections/search?utf8=%E2%9C%93&q=android&type=

The alternative is to exclude eclipse-collections manually for Android builds. That works, I don't think we'll be having users complaining about not being able to use PathMultiLabelGenerator anytime soon, and we can document that. Would you prefer that?

@saudet

This comment has been minimized.

Copy link
Member

saudet commented May 8, 2018

I haven't found anything about this kind of error, no. If you're asking me to debug Android's DEX compiler, I can start doing that, but I don't think it's the best use of our time.

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented May 8, 2018

An exclusion probably isn't the best idea... someone, somewhere, will use that on android and then we'll have to fix it then anyway. :)

Maybe let's merge this for now, and open an issue to consider the other uses of Eclipse collections later.
I'm just frustrated that we effectively can't use this library (that I've found to be pretty nice in practice :)) ever again because it "does not work" without really getting an explanation why (beyond a stack trace).

@saudet saudet merged commit fd4e55c into master May 8, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@saudet saudet deleted the sa_maven branch May 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.