GoogleCloudPlatform / appengine-plugins

A client Java library to manage App Engine Java applications for any project that performs App Engine Java application management. For example, the Maven, Gradle and Eclipse App Engine plugins, custom user tools, etc.
Apache License 2.0
40 stars 26 forks source link

Support lazy property evaluation (appengine.stage.setArtifact() doesn't carry task dependencies) #999

Open martinbonnin opened 2 years ago

martinbonnin commented 2 years ago

The follow Gradle configuration:

val myTask = tasks.register("myTask", Jar::class.java) {
  archiveBaseName.set("myJar")
}
appengine {
  stage {
    setArtifact(myTask.flatMap { it.archiveFile })
  }
}

Fails with the below:

$ ./gradlew appengineDeploy
> Task :backend:service-import:appengineStage FAILED

FAILURE: Build failed with an exception.

* What went wrong:
A problem was found with the configuration of task ':appengineStage' (type 'StageAppYamlTask').
  - In plugin 'com.google.cloud.tools.gradle.appengine.appyaml.AppEngineAppYamlPlugin' type 'com.google.cloud.tools.gradle.appengine.appyaml.StageAppYamlTask' property 'stagingExtension.artifact' specifies file '/Users/mbonnin/git/[...]/build/libs/myJar.jar' which doesn't exist.

    Reason: An input file was expected to be present but it doesn't exist.

I would have expected the provider to call myTask as a dependency automatically as outlined in https://docs.gradle.org/current/userguide/lazy_configuration.html#lazy_properties.

Without this, one has to rely on manual dependency wiring

elefeint commented 2 years ago

Lazy property evaluation is not supported in this plugin. Leaving it open as a feature request.

yigit commented 1 year ago

Note that this is happening for other properties too. e.g. this also doesn't work:

setAppEngineDirectory(buildAppEngineDirectoryTask.map {
  it.outputDirectory
})

The problem seems to be in these lines where the inputs are being normalized into a file immediately.

If you change StageAppYamlExtension's properties to RegularFileProperty, that should fix the issue but not sure if you would need to break API to do that.

elefeint commented 1 year ago

We would accept a user contribution for the feature.

Backwards compatibility would be ideal, although not always possible (e.g. the user programmatically accessing the value will have to call .get() on the object, which is not needed when the value is a string).

TWiStErRob commented 1 year ago

Wanted to open a similar issue. Laziness is missing overall. The problem goes as far as NPE in some cases because a task is not realized yet, so a workaround like this is needed:

// Eagerly configure Jar task, because AppEngineAppYamlPlugin uses Groovy properties APIs to access it.
tasks.named("jar").get()

Based on best practices from Google's very own @liutikas, here are some easily fixable red flags from AppEngineAppYamlPlugin:

They all have solutions, some are breaking as you described above. Would anyone be willing to collab on having a stab at this with me? And would anyone from @GoogleCloudPlatform promise to review the changes we make to land it, because it looks like @chanseokoh tried something similar 1.5 years ago, but it stalled.

martinbonnin commented 1 year ago

Still interested by this. Haven't looked at my GCP projects for a while but I can try to do a small PR at some point to get the ball rolling. Happy to review/test anything too.