rjaros / kvision

Object oriented web framework for Kotlin/JS
https://kvision.io
MIT License
1.2k stars 67 forks source link

KVConvertPoTask can't be cached, and so always runs (even if there are no changes) #436

Closed aSemy closed 1 year ago

aSemy commented 1 year ago

The task convertPoToJson always runs because the task does not declare any output files. This slightly slows down the build, and can trigger more work than is necessary. The task also runs even if there aren't any .po files.

> Task :modules:web-map:convertPoToJson
Caching disabled for task ':modules:web-map:convertPoToJson' because:
  Caching has not been enabled for the task
Task ':modules:web-map:convertPoToJson' is not up-to-date because:
  Task has not declared any outputs despite executing actions.
:modules:web-map:convertPoToJson (Thread[Execution worker Thread 3,5,main]) completed. Took 0.002 secs.

https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:up_to_date_checks

Normally this is easy to fix, however KVConvertPoTask is a little more difficult because it generates a dynamic number of files in the same directory as the input files. It might be possible to work around this using some clever Gradle magic... but it would be easier to split up the .po and .json files into separate directories.

I thought I'd make an issue first to discuss it, because it's a behaviour change.

As a temporary fix I've manually disabled the task (in my project I don't have .po files... yet.

tasks.withType<io.kvision.gradle.tasks.KVConvertPoTask>().configureEach {
  enabled = false
}
rjaros commented 1 year ago

As far as I understand (it was reimplemented a number of times) this task takes *.po files from build/processedResources/js/main/i18n, creates *.json file for every *.po file and deletes *.po file afterwards. I'm not sure why it works like that but it seems over-complicated :-) It could just process src/main/resources/i18n/*.po files and create build/processedResources/js/main/i18n/*.json files. And *.po files should be excluded from processResources task (just like *.pot files are).

aSemy commented 1 year ago

Yes it seems like it has some history :)

Does the task need to run every time? Maybe it shouldn't be part of the build cycle. It can be a utility task that can run as requested? That would help improve performance.

Keeping the generated .json files would make it easier. Instead of generating the .json files into the same directory, instead I suggest defining a new directory (./build/generated/i18n for example) that could be registered as a resource directory. The key difference being that the Gradle task would have different input and output directories - this makes defining the task much more easy.

rjaros commented 1 year ago

I have a feeling convertPoToJson task should not even be a task at all. It should be some kind of filter or extension to the processResources task (making the transformation when copying). Something like this example: https://www.oreilly.com/library/view/gradle-beyond-the/9781449373801/ch01.html#EX-FILTER-WITH-CLASS What do you think?

aSemy commented 1 year ago

Yes that makes sense, so we'd treat the generated .json files as files that should be included in the built artifact, but they shouldn't be committed to Git, because they can be generated from .po files (if there are any) as required. And if a user wants, they can manually replace the .po files with the .json files.

In this case I think it makes sense to keep the task, but output the generated .json files into some ./build/ directory (registered as the task output using @OutputDirectory), and then wire up the task output as a resource directory input.

Here's an example of a task in Kotest that produces a generated file. It's a .kt file but the principle is the same.

https://github.com/kotest/kotest/blob/9c8ccc591c6022b179fc0ec7306215c7f21dd74a/kotest-framework/kotest-framework-multiplatform-plugin-gradle/build.gradle.kts#L114-L135

rjaros commented 1 year ago

I've pushed some very simple modifications. I'm not sure how to test if the task is now correctly cacheable.

aSemy commented 1 year ago

Nice! I'll see if I can use Gradle TestKit to create a test

rjaros commented 1 year ago

Released with 5.15.0