Fix non-existent snapshot directory when running verifyPaparazziDebug #1695
Conversation
| @@ -237,7 +237,7 @@ public class PaparazziPlugin @Inject constructor( | |||
| test.inputs.dir( | |||
| isVerifyRun.flatMap { | |||
| project.objects.directoryProperty().apply { | |||
| set(if (it) snapshotOutputDir else null) | |||
| set(if (it && snapshotOutputDir.asFile.exists()) snapshotOutputDir else null) | |||
There was a problem hiding this comment.
If you use a file collection instead you won't have this issue. This works but creating that directory will cause a configuration cache miss when the directory is later created I believe.
There was a problem hiding this comment.
yep, I noticed that in the gradle docs but didn't go that way. Let me check that.
Updated 👍🏽
There was a problem hiding this comment.
Switching to this should work as a drop in replacement:
inputs.files(isVerifyRun.map { if (it) listOf(snapshotOutputDir) else emptyList() })
.withPropertyName("paparazzi.snapshots")
.withPathSensitivity(PathSensitivity.RELATIVE)
.optional()There was a problem hiding this comment.
It works because calling input.files with an empty directory simply adds no input files
There was a problem hiding this comment.
so this works but we don't want to cache based on the snapshot files themselves and we really want to rely on the snapshot path alone.
There was a problem hiding this comment.
To make caching work (and predictive test selection) the files themselves need to be included in the cache key so using a file property is not a good idea.
There was a problem hiding this comment.
yep makes sense, I opted to instead keep the directory property I originally had.
There was a problem hiding this comment.
The main issue with this approach is that the outcome can depend on when the task is configured. I also want to make clear that a directory property will still include the contents of the directory in the cache key for the task so you're still going to be checksumming all the snapshots (which is a good thing)
| project.objects.fileProperty().apply { | ||
| set(if (it) snapshotOutputDir.asFile else null) |
There was a problem hiding this comment.
I think you either want to ditch the file property or change keep it as a directory property. I'm not sure what happens if you pass a file property that points to a directory.
geoff-powell
force-pushed
the
gpowell/no-snapshot-dir
branch
2 times, most recently
from
1ba787e to
995defe
Compare
geoff-powell
force-pushed
the
gpowell/no-snapshot-dir
branch
from
995defe to
7608ffb
Compare
| test.logger.log( | ||
| WARN, | ||
| """ | ||
| Snapshot directory not found: ${snapshotOutputDir.asFile.path} | ||
| Please run the `recordPaparazzi$variantSlug` task to generate snapshots. | ||
| """.trimIndent() | ||
| ) |
There was a problem hiding this comment.
cc @jrodbx I added a warning log for cases where the verify task runs without a snapshot directory created.
There was a problem hiding this comment.
Can you add a way to disable this warning? It'll be really annoying for us to always see this warning
No description provided.