Configuration-cache-safe use of download and verify extensions about gradle-download-task HOT 14 CLOSED

Thanks for the thorough explanation. It totally makes sense to fail the whole download operation if the checksums do not match. I've never thought of that use case, but I see its value!

I'm wondering if it would make sense to merge the verify task into the download task (I mean directly in gradle-download-task, so that you don't have to create a custom task). I've always been reluctant to do that because I didn't want to add "yet another property":tm: to the download task (two properties in this case) but - as said above - I certainly see the value here and I'm willing to reconsider.

My proposal would be this: I'll merge the verify task into the download task, deprecate the current verify task so it can be removed in gradle-download-task 6, and then release a new version for you. I think I can find time for this on the weekend. Would that be OK for you?

from gradle-download-task.

I hooked up the 5.3.1-SNAPSHOT release to my experimental configuration-caching WALA branch, and adapted WALA's VerifiedDownload task to use your suggested approach. It works! A build of nearly everything, with all tasks already up-to-date, now takes under 0.9 seconds, down from 6 seconds without the cache. Nice!

However, I did need to make one other unexpected kind of change to get things working. Previously I had a few tasks whose inputs.files were set directly from some VerifiedDownload task. These other tasks typically unpack or extract something from a downloaded archive, as in:

val downloadArchive by tasks.registering(VerifiedDownload::class) {
  ...
}

val extractFileFromArchive by tasks.registering {
  inputs.files(downloadArchive)
  ...
}

These extraction tasks were incompatible with the configuration cache:

FAILURE: Build failed with an exception.

* What went wrong:
Configuration cache problems found in this build.

1 problem was found storing the configuration cache.
- Task `:com.ibm.wala.core:extractFileFromArchive` of type `org.gradle.api.DefaultTask`: cannot serialize object of type 'com.ibm.wala.gradle.VerifiedDownload', a subtype of 'org.gradle.api.Task', as these are not supported with the configuration cache.
  See https://docs.gradle.org/7.6/userguide/configuration_cache.html#config_cache:requirements:task_access

The detailed report provides no additional clues:

cannot serialize object of type com.ibm.wala.gradle.VerifiedDownload, a subtype of org.gradle.api.Task, as these are not supported with the configuration cache.

I was able to get these post-download extraction tasks to cooperate with the configuration cache by changing how I set the extraction tasks' inputs:

val downloadArchive by tasks.registering(VerifiedDownload::class) {
  ...
}

val extractFileFromArchive by tasks.registering {
  inputs.files(downloadArchive.map { it.outputs.files })
  ...
}

I was surprised to have to make this change, because in general simply using a Task as the argument to inputs.files(...) works fine. I don't know whether that fails here because of a problem with WALA's VerifiedDownload task, or with the gradle-download-task plugin, or with Gradle configuration caching in general. @michel-kraemer, if you have any thoughts on this, I'd love to hear your suggestions.

In any case, if this inputs.files(...) change is what WALA needs to do, then we can do that. I think that gradle-download-task 5.3.1-SNAPSHOT is ready for release. Thank you for all of your help with this challenge, @michel-kraemer!

from gradle-download-task.

I would expect that the Project could be fetched from the Task

Interesting idea. I need to test this. The javadoc of getProject says "Calling this method from a task action is not supported when configuration caching is enabled":
https://github.com/gradle/gradle/blob/master/subprojects/core-api/src/main/java/org/gradle/api/Task.java#L200C4-L208
But in this case, we would just call it from the constructor of the extension and not from the task action... Worth testing...

Much appreciated! I’d be happy to use an experimental branch to test-drive this in WALA before your official release if that would help.

Cool! Thank you. I'll release a SNAPSHOT first then.

from gradle-download-task.

Thanks for testing! Looks very good!

I'm not sure either why this change with inputs.files(...) is necessary. All I can say is that it might indeed be a problem with gradle-download-task. The plugin's code base is 8 years old now and - in order to maintain backwards compatibility - I had to implement some workarounds that might conflict with the configuration cache. If you'd start developing a Gradle plugin in 2023, you'd probably do some things slightly differently 😉 I cannot fix things now without breaking backwards compatibility, so I guess it's high time for a new major version of gradle-download-task, where I can drop support for old Gradle versions and implement the interface like Gradle (or at least the newer versions thereof) likes it.

I'm currently busy with other projects but I'll put this on my TODO list for the upcoming months. Maybe you can then simplify your code again ... hopefully, no promises 😉

In the meantime, I'll release 5.3.1, definitely until the end of the week. I'll keep you up to date.

from gradle-download-task.

Version 5.3.1 has just been released. Have fun! And thanks again for reporting this issue and for testing the fix!

from gradle-download-task.

I don't think this is possible. I'm wondering why you're calling the extensions in your task, though. How about using this.dependsOn? You could also extend de.undercouch.gradle.tasks.download.Download and then call this.finalizedBy to add a VerifyTask. I haven't tried that myself anyhow. Let me know if it works.

from gradle-download-task.

I'm wondering why you're calling the extensions in your task, though.

WALA builds have had problems in the past with truncated or corrupted downloads. Therefore, if the checksum does not match our expectation, then we want to consider the entire download operation as having failed. Sequentially using both extensions in the action method of a single task gives us that behavior. We don't get that from either alternative that you suggest.

How about using this.dependsOn?

If some download depends on a corresponding verification, and the verification fails, then Gradle will still consider the download to have succeeded. For example, Gradle will retain that download in the build area, and will also cache the result of that download. Future builds will not reattempt the download. Thus, if a download is silently truncated or corrupted, then our user will be stuck with a copy of that bad download in his or her build directory. Even removing build is not sufficient: that bad download will keep coming back from the build cache until the user manually clears the build cache. That's not a good user experience.

This "bad download in the cache" problem hit us repeatedly in the past, and is the whole reason we added checksum verification. This problem is difficult to recognize, and equally difficult to correct, unless you're quite knowledgeable about Gradle caching. That's not a level of skill we want to require from our users.

You could also extend de.undercouch.gradle.tasks.download.Download and then call this.finalizedBy to add a VerifyTask.

finalizedBy tasks are run regardless of whether the original task succeeded or failed. But there's no sense in verifying a failed download. This extra work is harmless, but useless.

Furthermore, the Gradle documentation is silent on whether a task is considered to have failed if its finalizer task fails. As noted above, it's important to us that a download be considered to have failed if its corresponding verification fails.

from gradle-download-task.

Thanks for the thorough explanation. It totally makes sense to fail the whole download operation if the checksums do not match. I've never thought of that use case, but I see its value!

Thank you for proving this plugin in the first place, and for being open to new ideas about how it can be used.

My proposal would be this: I'll merge the verify task into the download task, deprecate the current verify task so it can be removed in gradle-download-task 6, and then release a new version for you.

Allowing the download talk to also verify makes good sense to me. I'd be happy to use that in place of WALA's custom VerifiedDownload task, especially if your version is compatible with the Gradle configuration cache.

I am neutral about deprecating the verify task. That won't affect me either way, but it might affect someone else. Are you confident that nobody is using the verify task for its own merits, on data that did not come from a download?

I think I can find time for this on the weekend. Would that be OK for you?

That would be great! No rush on this. I'm not paying your salary, after all. And even my own work as a WALA maintainer is donated personal time, mostly on weekends. So a hard deadline of "eventually, when I get around to it" is perfectly fine with me.

Thanks again!

from gradle-download-task.

I've been playing around with different things and I think I came up with a solution for your original problem. I still need to make minor adjustments and release a new version, but when I'm done, you will be able to do something along the lines of this:

abstract class DownloadAndVerifyTask extends DefaultTask {
    @Input
    abstract Property<String> getSrc()

    @OutputFile
    abstract RegularFileProperty getDest()

    @Internal
    final def downloadExt

    @Internal
    final def verifyExt

    DownloadAndVerifyTask() {
        downloadExt = project.getObjects().newInstance(DownloadExtension, project, this)
        verifyExt = project.getObjects().newInstance(VerifyExtension, project, this)
    }

    @TaskAction
    def downloadAndVerify() {
        downloadExt.run {
            src this.src
            dest this.dest
            overwrite true
            onlyIfModified true
        }
        verifyExt.run {
            src this.dest.get().asFile
            algorithm 'MD5'
            checksum '84238dfc8092e5d9c0dac8ef93371a07' // can be made configurable too
        }
    }
}

While the project cannot be serialized in the configuration cache, the two extensions can. You just have to create new instances of them. At the moment, the constructors for that are missing - that's why I need to release an update. It's running locally on my machine already, but I still need to test everything and prepare the release. I'll get back to you within the next days.

from gradle-download-task.

While the project cannot be serialized in the configuration cache, the two extensions can. You just have to create new instances of them.

Ah, interesting strategy. I can see this working well for me. Nice!

At the moment, the constructors for that are missing - that's why I need to release an update.

Makes sense. Regarding the constructors, are the project and this arguments both needed? I would expect that the Project could be fetched from the Task with only the latter given to the extensions’ constructors.

It's running locally on my machine already, but I still need to test everything and prepare the release. I'll get back to you within the next days.

That’s great. Much appreciated! I’d be happy to use an experimental branch to test-drive this in WALA before your official release if that would help. Entirely up to you.

from gradle-download-task.

I've just published 5.3.1-SNAPSHOT. You'll need something like this in your settings.gradle file to use it:

pluginManagement {
    repositories {
        maven {
            url "https://oss.sonatype.org/content/groups/public"
        }
        gradlePluginPortal()
    }
}

from gradle-download-task.

Also, I've removed project from the constructor. You only need to provide this.

from gradle-download-task.

Hooray! It was great working with you on this enhancement, Michael. Many thanks from the WALA team to you.

from gradle-download-task.

@liblit I've tried to follow up discussion and I still need a picture of how to use the task to be a configure cachable.

We use it to load the manifest during the build, and if the manifest changes, we would load things defined in the manifest.

However, I think I have to go over the Gradle tutorial about configuration cache.

from gradle-download-task.