Fixes issues with preparePaparazzi{Variant}Resources task that is caching incorrectly for different variants. #720

geoff-powell · 2023-02-21T16:04:50Z

Fixes the issue this pr attempted to reproduce/solve.

Investigation doc on this issue for Cash.

TL;DR

Looking into the generated cache key it seems that it creates a build cache for the same generated key. So to recap preparePaparazziDebugResources runs around 6m into the build process and generates the correct debug resource file for paparazzi. At 9m into the build preparePaparazziReleaseResources runs to generate the correct release resource file for paparazzi but stores the output for the same cache key 8c39bb.
So this validates why the output for the debug task contains references to release paths.

I added a test gradle plugin project here to simulate the exact issue in the first commit - Sample Build Failure

The last commit adds variantName as a build input and causes the failing test to pass.

cc @mohsin-motorway this may fix the issue you were having as well.

Fixes issue #560 as well.

TWiStErRob · 2023-02-21T16:30:49Z

gradlew.bat

-if "%OS%"=="Windows_NT" endlocal
-
-:omega
+@rem


my bad #721

rebasing off of master as this seems to be fixed

geoff-powell · 2023-02-21T17:38:45Z

...razzi/paparazzi-gradle-plugin/src/test/java/app/cash/paparazzi/gradle/PaparazziPluginTest.kt

+    with(firstRun.task(":preparePaparazziDebugResources")) {
+      assertThat(this).isNotNull()
+      assertThat(this!!.outcome).isNotEqualTo(FROM_CACHE)
+    }
+
+    with(firstRun.task(":preparePaparazziReleaseResources")) {
+      assertThat(this).isNotNull()
+      assertThat(this!!.outcome).isNotEqualTo(FROM_CACHE)
+    }


Added these checks here as they should never use cache on the first run. This will fail due to the task inputs being treated by gradle as the same and therefore the preparePaparazziReleaseResources is cached and the preparePaparazziDebugResources uses that cache instead of running.

geoff-powell · 2023-02-21T17:40:08Z

...razzi/paparazzi-gradle-plugin/src/test/java/app/cash/paparazzi/gradle/PaparazziPluginTest.kt

+    var resourcesFile = File(fixtureRoot, "build/intermediates/paparazzi/debug/resources.txt")
+    assertThat(resourcesFile.exists()).isTrue()
+    var resourceFileContents = resourcesFile.readLines()
+    assertThat(resourceFileContents.any { it.contains("release") }).isFalse()
+
+    resourcesFile = File(fixtureRoot, "build/intermediates/paparazzi/release/resources.txt")
+    assertThat(resourcesFile.exists()).isTrue()
+    resourceFileContents = resourcesFile.readLines()
+    assertThat(resourceFileContents.any { it.contains("debug") }).isFalse()


These will verify that the resource file shouldn't have any paths with debug for release or vice-versa. The first commit doesn't verify this but it does fail if you comment out the task outcome assertions above.

Nice test strategy!

geoff-powell · 2023-02-21T17:41:10Z

...razzi/paparazzi-gradle-plugin/src/test/java/app/cash/paparazzi/gradle/PaparazziPluginTest.kt

+    resourcesFile = File(fixtureRoot, "build/intermediates/paparazzi/debug/resources.txt")
+    assertThat(resourcesFile.exists()).isTrue()
+    resourceFileContents = resourcesFile.readLines()
+    assertThat(resourceFileContents.any { it.contains("release") }).isFalse()


this will also fail for the same reason above. Uses release resource output for preparePaparazziDebugResources as it is cached with the same key as the previous preparePaparazziDebugResources

geoff-powell · 2023-02-22T17:05:13Z

After talking with @jrodbx yesterday, the fix I had here works but may not be the final answer. The main issue is that the paths for inputs mergeResourcesOutput and mergeAssetsOutput are incorrectly being treated as the same, even though the paths contain debug or release folders. I began investigating @PathSensitity annotation used to see why these are treated as the same.

PathSensitivity.RELATIVE (Issue)

When using this path sensitivity, the “root” directory is /paparazzi/sample/build/intermediates/merged_res/{variant}/. This means that the task sees the input files of the directory as ADDED: drawable/camera.png

PathSensitivity.NAME_ONLY

This reports the filenames only in the directory ADDED: camera.png which again is not what we want.

PathSensitivity.ABSOLUTE

This reports the absolute file path which is great for separation but doesn’t scale to different machines and environments. ADDED: /{sourceCodeRoot}/Development/paparazzi/sample/build/intermediates/merged_res/debug/drawable/camera.png

PathSensitivity.NONE

This should not be used here as the files in the directory are ignored.

Very important note in gradle documentation that task caching is that Gradle will try and reuse outputs for the different task given the inputs. This is basically the root cause of the issue as we are using relative paths for the directory inputs, which are identical for different task variants.

So when looking at the @InputDirectory annotation there is a note which signifies using @Input instead if you only care about the directory and not the files within it (Similar issue asked on gradle discussion).

Note: To make the task dependent on the directory's location but not its contents, expose the path of the directory as an Input property instead.

This is exactly what we are looking to do and I have updated the pr to reflect a simpler and more direct solution.

geoff-powell · 2023-02-23T12:19:40Z

...azzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PrepareResourcesTask.kt

-  private fun Directory.relativize(child: Directory): String {
-    return asFile.toPath().relativize(child.asFile.toPath()).toFile().invariantSeparatorsPath
-  }


Moved this function to the plugin to relativize the path input for mergeAssetsOutput and mergeResourcesOutput

can we move this function as is to the plugin as a private extension method?

geoff-powell · 2023-02-23T12:20:27Z

paparazzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt

+        fun DirectoryProperty.asRelativePathString(childDirectory: Provider<Directory>): Provider<String> =
+          flatMap { buildDir ->
+            childDirectory.map { childDir ->
+              buildDir.asFile.toPath().relativize(childDir.asFile.toPath()).invariantSeparatorsPathString
+            }
+          }
+


Using mapping functions here to keep this property lazy for configuration perf

swankjesse · 2023-02-23T18:47:26Z

...azzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PrepareResourcesTask.kt

-  @get:PathSensitive(PathSensitivity.RELATIVE)
-  abstract val mergeResourcesOutput: DirectoryProperty
+  @get:Input
+  abstract val mergeResourcesOutput: Property<String>


It has to be a String? Gradle is so annoying

Yep, basically DirectoryProperty with @PathSensitive(PathSensitivity.RELATIVE) is used for a directory where you want a sub folder or file changes to run the task. Since its relative and the base directory is .../debug and .../release gradle sees the underlying files as the same.

The suggestion for using Property here is if you only want the path to be used as an input and the contents don't matter. This was our exact usecase as we just use the path in the generated resources.txt file.

The worst is when Gradle simply ignores your parameter without telling you it's not the right type and that what you expect to happen is never gonna be!

swankjesse · 2023-02-23T18:47:57Z

...razzi/paparazzi-gradle-plugin/src/test/java/app/cash/paparazzi/gradle/PaparazziPluginTest.kt

+      .withArguments("testRelease", "testDebug", "--build-cache", "--stacktrace")
+      .runFixture(fixtureRoot) { build() }
+
+    println(firstRun.output)


nit: remove?

swankjesse · 2023-02-23T18:48:07Z

...razzi/paparazzi-gradle-plugin/src/test/java/app/cash/paparazzi/gradle/PaparazziPluginTest.kt

+    var resourcesFile = File(fixtureRoot, "build/intermediates/paparazzi/debug/resources.txt")
+    assertThat(resourcesFile.exists()).isTrue()
+    var resourceFileContents = resourcesFile.readLines()
+    assertThat(resourceFileContents.any { it.contains("release") }).isFalse()


swankjesse · 2023-02-23T18:48:46Z

paparazzi/paparazzi-gradle-plugin/src/test/projects/prepare-resources-task-caching/build.gradle

+    url "file://${projectDir.absolutePath}/../../../../../build/localMaven"
+  }
+  mavenCentral()
+  //mavenLocal()


nit: remove?

swankjesse

Very much looking forward to having these fixes. THANK YOU!

geoff-powell · 2023-02-23T19:11:32Z

Very much looking forward to having these fixes. THANK YOU!

Ya no problem! Fixed up your suggested changes. Will merge once CI passes 👍🏽

…hing incorrectly for different variants. cashapp#720

jrodbx

Nice work, def learned a lot more about Input properties as a result of this PR!

...azzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PrepareResourcesTask.kt

jrodbx · 2023-03-25T07:11:36Z

...azzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PrepareResourcesTask.kt

-  private fun Directory.relativize(child: Directory): String {
-    return asFile.toPath().relativize(child.asFile.toPath()).toFile().invariantSeparatorsPath
-  }


can we move this function as is to the plugin as a private extension method?

...azzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PrepareResourcesTask.kt

paparazzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt

...azzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PrepareResourcesTask.kt

paparazzi/paparazzi-gradle-plugin/src/main/java/app/cash/paparazzi/gradle/PaparazziPlugin.kt

…f preparePaparazzi{Variant}Resources in the same command.

…the appropriate variant

…String properties

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch 2 times, most recently from 368c898 to 1f40444 Compare February 21, 2023 16:25

TWiStErRob reviewed Feb 21, 2023

View reviewed changes

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch 4 times, most recently from d8c4e8f to 00a046e Compare February 21, 2023 16:55

geoff-powell marked this pull request as ready for review February 21, 2023 17:05

geoff-powell requested review from jrodbx and gamepro65 February 21, 2023 17:05

geoff-powell commented Feb 21, 2023

View reviewed changes

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch from 922d3e9 to 5d4aefa Compare February 21, 2023 17:55

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch from 5215c6b to c6af0aa Compare February 23, 2023 11:46

geoff-powell commented Feb 23, 2023

View reviewed changes

swankjesse reviewed Feb 23, 2023

View reviewed changes

swankjesse approved these changes Feb 23, 2023

View reviewed changes

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch from c6af0aa to 1dca484 Compare February 23, 2023 19:08

gabrielittner added a commit to freeletics/paparazzi that referenced this pull request Mar 18, 2023

Fixes issues with preparePaparazzi{Variant}Resources task that is cac…

5f95111

…hing incorrectly for different variants. cashapp#720

jrodbx added this to the 1.3 milestone Mar 24, 2023

jrodbx approved these changes Mar 25, 2023

View reviewed changes

geoff-powell added 2 commits March 25, 2023 08:18

Add test to verify caching is broken when running multiple variants o…

8f77504

…f preparePaparazzi{Variant}Resources in the same command.

Add variant to task inputs to ensure that the task caching works for …

636e9bf

…the appropriate variant

Remove variant name input and add swap DirectoryProperty inputs with …

ffae0e2

…String properties

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch 3 times, most recently from 11b5df6 to 2e04e2f Compare March 25, 2023 12:21

Cleanup

cfc4b6d

geoff-powell force-pushed the gpowell/cash-font-loading-issue-repro branch from 2e04e2f to cfc4b6d Compare March 25, 2023 12:25

geoff-powell merged commit 75b9ead into master Mar 25, 2023

geoff-powell deleted the gpowell/cash-font-loading-issue-repro branch March 25, 2023 14:58

jrodbx mentioned this pull request Jan 20, 2024

Fix variant caching issues in new resource/asset loading mechanisms #1247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issues with preparePaparazzi{Variant}Resources task that is caching incorrectly for different variants. #720

Fixes issues with preparePaparazzi{Variant}Resources task that is caching incorrectly for different variants. #720

geoff-powell commented Feb 21, 2023 •

edited

Loading

TWiStErRob Feb 21, 2023

geoff-powell Feb 21, 2023

geoff-powell Feb 21, 2023

geoff-powell Feb 21, 2023

swankjesse Feb 23, 2023

geoff-powell Feb 21, 2023

geoff-powell commented Feb 22, 2023 •

edited

Loading

geoff-powell Feb 23, 2023

jrodbx Mar 25, 2023

geoff-powell Feb 23, 2023 •

edited

Loading

swankjesse Feb 23, 2023

geoff-powell Feb 23, 2023 •

edited

Loading

oldergod Apr 5, 2023

swankjesse Feb 23, 2023

swankjesse Feb 23, 2023

swankjesse Feb 23, 2023

swankjesse left a comment

geoff-powell commented Feb 23, 2023

jrodbx left a comment

jrodbx Mar 25, 2023

Fixes issues with preparePaparazzi{Variant}Resources task that is caching incorrectly for different variants. #720

Fixes issues with preparePaparazzi{Variant}Resources task that is caching incorrectly for different variants. #720

Conversation

geoff-powell commented Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoff-powell commented Feb 22, 2023 • edited Loading

PathSensitivity.RELATIVE (Issue)

PathSensitivity.NAME_ONLY

PathSensitivity.ABSOLUTE

PathSensitivity.NONE

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoff-powell Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoff-powell Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

geoff-powell commented Feb 23, 2023

jrodbx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoff-powell commented Feb 21, 2023 •

edited

Loading

geoff-powell commented Feb 22, 2023 •

edited

Loading

geoff-powell Feb 23, 2023 •

edited

Loading

geoff-powell Feb 23, 2023 •

edited

Loading