Support configuration caching #257
Conversation
Just checking the first char isn't enough!
ZacSweers
changed the title
|
The test failure appears to be due to the tweaked hashcode calculation to exclude buildUuid like the doc mentioned. Reverted that for now in dcf8007 but probably worth a second look later |
src/main/kotlin/com.bugsnag.android.gradle/AndroidManifestInfo.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com.bugsnag.android.gradle/AndroidManifestParser.kt
Outdated
Show resolved
Hide resolved
| * will be returned. If no existing requests match, a new request will | ||
| * be enqueued and executed. | ||
| * | ||
| * Equality is measured by the [AndroidManifestInfo] (excluding buildUuid) |
There was a problem hiding this comment.
This doesn't actually exclude the buildUuid property. I tried to exclude it in f1c443b but that resulted in some failing tests that expected more results than would happen when it's excluded. Reverted and left for another time
There was a problem hiding this comment.
The docs are wrong on this - the buildUuid property should be part of the hashcode. There's been a bit of back and forth on the design behind this internally, but we've now settled on the following behaviour:
- By default the plugin generates a build UUID, and it is recommended that the plugin should be disabled for any unnecessary variants to improve build performance
- Advanced users can specify their own deterministic build UUID by adding a
meta-datatag in the manifest
It's a bit of a tricky balance between the risk of overwriting existing mapping files with the same version information, versus improving performance by using cached task outputs. This approach is the best solution we've come up with so far.
In terms of actions, the comment just needs editing here to remove the erroneous information.
| * will be returned. If no existing requests match, a new request will | ||
| * be enqueued and executed. | ||
| * | ||
| * Equality is measured by the [AndroidManifestInfo] (excluding buildUuid) |
There was a problem hiding this comment.
| * Equality is measured by the [AndroidManifestInfo] (excluding buildUuid) | |
| * Equality is measured by the [AndroidManifestInfo] |
| import java.util.concurrent.ConcurrentHashMap | ||
| import java.util.concurrent.FutureTask | ||
|
|
||
| sealed class UploadRequestClient : AutoCloseable { |
There was a problem hiding this comment.
nice use of a sealed class 👍
| internal object GradleVersions { | ||
| val VERSION_5_3: VersionNumber = VersionNumber.parse("5.3") | ||
| val VERSION_6: VersionNumber = VersionNumber.parse("6.0") | ||
| val VERSION_6_1: VersionNumber = VersionNumber.parse("6.1") |
There was a problem hiding this comment.
nit: SYS_PROPERTIES_VERSION in BugsnagReleasesTask could use this constant instead
| return when { | ||
| gradleVersion >= GradleVersions.VERSION_6 -> { | ||
| when { | ||
| gradleVersion >= GradleVersions.VERSION_6_6 -> { |
There was a problem hiding this comment.
style nit: the indentation is off by two spaces for this when block
Resolves #253
There is still more work to do though, namelyUploadRequestClientneeds to be rethought as its state-sharing nature is incompatible with configuration caching. This PR gets most of the way there though!Fully implements support for configuration caching, and modernizes much of the internals along the way in a backward-compatible way!
Some highlights:
BuildServiceto share state for certain things, likeUploadRequestClientand okhttpclient instances