Android Gradle: add javac intermediates to classpath #4723
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4723 +/- ##
=========================================
Coverage 84.71% 84.71%
- Complexity 3423 3425 +2
=========================================
Files 490 490
Lines 11255 11256 +1
Branches 2069 2069
=========================================
+ Hits 9535 9536 +1
Misses 675 675
Partials 1045 1045
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| internal fun Project.registerIntermediatesJavacJarTask(variant: BaseVariant): TaskProvider<Jar> { |
There was a problem hiding this comment.
Q: Why do we need a new task for getting the intermediates classes? Wouldn't it be sufficient to just include them in the classpath?
There was a problem hiding this comment.
To be honest, I thought it was necessary because the PR #3867 also implemented it. But using the path directly is also possible and it saves a lot overhead.
| task.from( | ||
| layout | ||
| .buildDirectory | ||
| .dir("intermediates/javac/${variant.name}/classes") |
There was a problem hiding this comment.
This feels a bit fragile 🤔 IIRC the new Variant API from AGP allowed to access the compilation classpath for each variant.
There was a problem hiding this comment.
Well, you're right. I found a way to find out the directory directly over the javaCompileProvider. I think that's a much cleaner solution.
| setFrom(variant.getCompileClasspath(null).filter { it.exists() }) | ||
| from(bootClasspath) | ||
| from(javaCompileDestination(variant)) | ||
| } |
There was a problem hiding this comment.
You should be able to do this:
classpath.setFrom(
variant.getCompileClasspath(null).filter { it.exists() },
bootClasspath,
javaCompileDestination(variant)
}| from(bootClasspath) | ||
| from(javaCompileDestination(variant)) | ||
| } | ||
| dependsOn(variant.javaCompileProvider) |
There was a problem hiding this comment.
Manually adding task dependencies should not be required if the new inputs to this task are properly declared. Including javaCompileDestination(variant) in the classpath should be enough - if not, that's something that should be investigated instead of adding the manual task dependency.
There was a problem hiding this comment.
This might be necessary though because DetektTask does not necessarily declare on .java or .class as the sources input.
There was a problem hiding this comment.
It doesn't have to, if the properties are wired up correctly. Task dependencies are added implicitly when the input to classpath includes types that carry task dependencies. DirectoryProperty (returned by javaCompileDestination(variant)) is one of those types, and if that's the property that contains the correct path to the .java or .class files then Gradle has all the information it needs to do the right thing.
There was a problem hiding this comment.
Good to know, thanks!
| val javaCompile = variant.javaCompileProvider.orNull | ||
| if (javaCompile == null) { | ||
| logger.warn( | ||
| "Unable to find Java compiler on variant '{}'. Detekt analysis can show false negatives.", |
There was a problem hiding this comment.
Is there a way to rewrite the warning to make it actionable for users? Even if that's just updating the AGP version.
Actually while I think of it, will this cause an exception if an older version of AGP is used, if javaCompileProvider was only recently added?
There was a problem hiding this comment.
We have the compatibility table listed here, so that backward incompatibility with older AGP versions shouldn't be a major concern.
There was a problem hiding this comment.
That doesn't address my feedback. If the correct action is to update AGP then the warning message should recommend the update and/or direct users to the compatibility table so they know what to do and why, so that the user understands that using older AGP versions will lead to false positives.
Or if the action to correct this is something else, then that should be shown instead.
There was a problem hiding this comment.
Note that the .getJavaCompileProvider() was added back in 2018-08 through this PR.
and .destinationDirectoy was built since Gradle 6.1
https://cs.github.com/gradle/gradle/blob/b593969ee3fe63c50466019722cb847d6cd89f4c/subprojects/language-jvm/src/main/java/org/gradle/api/tasks/compile/AbstractCompile.java?q=AbstractCompile
I can't tell easily which AGP version is incompatible with this call, but I assume the current PR is good enough that if customer's build is broken from detekt, it is likely broken from many other plugins and the AGP itself.
There was a problem hiding this comment.
.getJavaCompileProvider was introduced with AGP 3.3 (https://developer.android.com/studio/releases/gradle-plugin#3-3-0), which means Gradle 4.10.
In the end, I don't know what I should recommend to the user. The .destinationDirectory should work since Gradle 6.1 (thanks chao2zhang for the research) and detekt has already the minimum of Gradle 6.1+ in the Readme.
This PR is mainly a rebuild of #3867 from @damianw and would fix a lot of issues together with Android (#4676 and #4689 as example). The description is almost the same:
I simply updated the tests and changed that the task detektIntermediatesJavacJarVARIANT depends on the java compile task and not the full assemble task. This should be enough to create the intermediate *.class files and is way faster as the whole packaging process.
Also I wasn't able to reproduce #3952 with this configuration. But maybe someone with deeper gradle knowledge (@chao2zhang ?) could take a look to be sure.