Avoid creating and configuring tasks when unnecessary #394

chanseokoh · 2021-02-05T22:51:21Z

Fixes GoogleCloudPlatform/appengine-plugins#1005.

Based on https://docs.gradle.org/current/userguide/task_configuration_avoidance.html

chanseokoh · 2021-02-05T22:56:12Z

However, this still doesn't work because project.afterEvaluate() is forbidden in the new configuration avoidance API. From https://docs.gradle.org/current/userguide/task_configuration_avoidance.html, it says

Some APIs may be disallowed if you try to access them from the new API’s configuration blocks. For example, Project.afterEvaluate() cannot be called when configuring a task registered with the new API.

So when actually applied to a project, it fails:

* What went wrong:
A problem occurred configuring project ':backend'.
> Failed to notify project evaluation listener.
   > Could not create task ':backend:downloadCloudSdk'.
      > Project#afterEvaluate(Action) on project ':backend' cannot be executed in the current context.
   > Could not create task ':backend:appengineDeployAll'.
      > Project#afterEvaluate(Action) on project ':backend' cannot be executed in the current context.

codecov · 2021-02-10T16:14:25Z

Codecov Report

Merging #394 (bae6dc2) into master (093c818) will decrease coverage by 2.61%.
The diff coverage is 88.34%.

@@             Coverage Diff              @@
##             master     GoogleCloudPlatform/app-gradle-plugin#394      +/-   ##
============================================
- Coverage     75.76%   73.14%   -2.62%     
+ Complexity      277      275       -2     
============================================
  Files            42       43       +1     
  Lines          1073     1013      -60     
  Branches         41       42       +1     
============================================
- Hits            813      741      -72     
- Misses          242      255      +13     
+ Partials         18       17       -1

Impacted Files	Coverage Δ	Complexity Δ
...tools/gradle/appengine/core/CloudSdkLoginTask.java	`60.00% <ø> (ø)`	`2.00 <0.00> (ø)`
...ools/gradle/appengine/standard/ExplodeWarTask.java	`100.00% <ø> (ø)`	`5.00 <0.00> (ø)`
.../cloud/tools/gradle/appengine/core/DeployTask.java	`53.84% <33.33%> (-9.80%)`	`4.00 <1.00> (ø)`
...e/appengine/sourcecontext/SourceContextPlugin.java	`84.09% <76.19%> (-5.10%)`	`9.00 <5.00> (ø)`
...pengine/core/AppEngineCorePluginConfiguration.java	`79.71% <77.63%> (-6.01%)`	`37.00 <29.00> (-7.00)`
.../cloud/tools/gradle/appengine/AppEnginePlugin.java	`84.21% <100.00%> (ø)`	`8.00 <1.00> (ø)`
...adle/appengine/appyaml/AppEngineAppYamlPlugin.java	`90.76% <100.00%> (+0.29%)`	`14.00 <12.00> (+5.00)`
...ud/tools/gradle/appengine/core/BaseDeployTask.java	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
...oud/tools/gradle/appengine/core/DeployAllTask.java	`91.66% <100.00%> (ø)`	`8.00 <1.00> (ø)`
...ud/tools/gradle/appengine/core/DeployCronTask.java	`55.55% <100.00%> (ø)`	`3.00 <1.00> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 093c818...bae6dc2. Read the comment docs.

…dPlatform/app-gradle-plugin into task-config-avoidance

chanseokoh · 2021-02-10T22:09:21Z

Fixed the problem, and the integration tests are passing. However, I don't have strong confidence that it doesn't break anything. Previously, even after the integration tests passing, I encountered an error on a sample project. For now, I'll leave this in the draft state.

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java

chanseokoh · 2021-02-12T22:00:45Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java

@@ -89,10 +91,10 @@ private void configureExtensions() {
          // we can only set the default location of "archive" after project evaluation (callback)
          if (stageExtension.getArtifact() == null) {
            if (project.getPlugins().hasPlugin(WarPlugin.class)) {
-              War war = (War) project.getProperties().get("war");
+              War war = tasks.withType(War.class).getByName("war");


For some reason that I don't know, the previous code was returning null.

Also I think the new code may still unnecessarily instantiate the war task.

Right you are. It does instantiate create task.
The way to avoid task creation is stageExtension.setArtifact(tasks.named<War>(WarPlugin.WAR_TASK_NAME).map { it.getArchiveFile() })

Removed getByName() in another place in bae6dc2. I assume getProject().zipTree(object) will work fine when given a TaskProvider. (Previously, it was getting File directly from getArchivePath().) Does this look good?

vlsi · 2021-02-14T23:52:58Z

...main/java/com/google/cloud/tools/gradle/appengine/core/AppEngineCorePluginConfiguration.java

+    if (injectDeployExtension) {
+      project.afterEvaluate(
+          project -> taskProvider.configure(task -> task.setDeployExtension(deployExtension)));
+    }


What is the purpose of afterEvaluate here?
It looks like task.setDeployExtension(deployExtension) can be called during task registration.

I have the same question. (I have never worked on this repo, and I don't have a lot of Gradle knowledge.)

What I can see is that deployExtension is instantiated in the following way in Plugin.apply().

First, a parent extension is created with .create()

appengineExtension = project.getExtensions().create("appengine", AppEngineAppYamlExtension.class);

and then the sub-extension (this below is appengineExtension created above)

deploy = ((ExtensionAware) this).getExtensions().create(DEPLOY_EXT, DeployExtension.class, project);

This deploy is what is called deployExtension in this class (if I am following the code flow correctly).

So the question is, do we still need to use afterEvaluate() when calling setDeployExtension(deployExtension)? I guess not, because it's just passing and setting a Java reference. But since I have never worked on this repo, I'm concerned there may be something that I'm not seeing. WDYT?

I guess not, because it's just passing and setting a Java reference

Indeed. If the reference is not changed, then it can be passed without afterEvaluate.

I removed more afterEvaluate blocks in 1ca1a13 and 29190de.

vlsi · 2021-02-14T23:58:10Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java

              stageExtension.setArtifact(war.getArchivePath());
            } else if (project.getPlugins().hasPlugin(JavaPlugin.class)) {
-              Jar jar = (Jar) project.getProperties().get("jar");
+              Jar jar = tasks.withType(Jar.class).getByName("jar");


Same here: it is better to use tasks.named(...)

vlsi · 2021-02-14T23:59:57Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java

+                    task.setStageDirectory(stageExtension.getStagingDirectory());
+                    task.setDeployExtension(deploy);


If you use Property<...> for stageExtension.getStagingDirectory, then you could move this task configuration outside of afterEvaluate.

vlsi · 2021-02-15T00:00:24Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java

+    project.afterEvaluate(
+        project -> stage.configure(task -> task.setStagingConfig(stageExtension)));


Is afterEvaluate needed here?

InLaw · 2021-07-20T19:26:28Z

What can be done to continue here, who needs to do the review?

TWiStErRob · 2022-11-23T14:41:32Z

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java

-              Jar jar = (Jar) project.getProperties().get("jar");
-              stageExtension.setArtifact(jar.getArchivePath());
+              stageExtension.setArtifact(
+                  tasks.withType(Jar.class).named("jar").map(jar -> jar.getArchivePath()));


no need to create a filtered task collection:

Suggested change

tasks.withType(Jar.class).named("jar").map(jar -> jar.getArchivePath()));

tasks.named("jar", Jar.class).map(jar -> jar.getArchivePath()));

chanseokoh added 2 commits February 5, 2021 17:48

task configuration avoidance

165ea84

remove bin/

f59206c

ciferkey mentioned this pull request Feb 9, 2021

app-gradle-plugin breaks tasks in multi project build? GoogleCloudPlatform/appengine-plugins#1005

Closed

Merge branch 'master' into task-config-avoidance

5b2cd8d

chanseokoh added 3 commits February 10, 2021 14:23

Merge remote-tracking branch 'origin/master' into task-config-avoidance

4647ebf

Fix broken plugin setup

6236f8e

Merge branch 'task-config-avoidance' of https://github.com/GoogleClou…

17f21be

…dPlatform/app-gradle-plugin into task-config-avoidance

chanseokoh commented Feb 10, 2021

View reviewed changes

src/main/java/com/google/cloud/tools/gradle/appengine/appyaml/AppEngineAppYamlPlugin.java Outdated Show resolved Hide resolved

chanseokoh added 2 commits February 12, 2021 16:54

Fix

99e9c0f

Add BaseDeployTask

0fcb729

chanseokoh commented Feb 12, 2021

View reviewed changes

chanseokoh added 5 commits February 12, 2021 17:17

Fix

9141e22

Fix around injectGcloud / refactor code

743492a

Fix injectGcloud

1d12f1c

Style

c6c0ab0

style

877d1d0

chanseokoh marked this pull request as ready for review February 12, 2021 22:59

chanseokoh force-pushed the task-config-avoidance branch from 9018e56 to 877d1d0 Compare February 13, 2021 00:59

vlsi reviewed Feb 14, 2021

View reviewed changes

vlsi reviewed Feb 15, 2021

View reviewed changes

chanseokoh added 3 commits February 18, 2021 11:11

no need for afterEvaluate

1ca1a13

remove more afterEvaluate blocks

29190de

Avoid War and Jar task creation

bae6dc2

TWiStErRob reviewed Nov 23, 2022

View reviewed changes

TWiStErRob mentioned this pull request Nov 23, 2022

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

Open

JoeWang1127 closed this Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid creating and configuring tasks when unnecessary #394

Avoid creating and configuring tasks when unnecessary #394

chanseokoh commented Feb 5, 2021

chanseokoh commented Feb 5, 2021

codecov bot commented Feb 10, 2021 •

edited

Loading

chanseokoh commented Feb 10, 2021 •

edited

Loading

chanseokoh Feb 12, 2021 •

edited

Loading

vlsi Feb 14, 2021

chanseokoh Feb 18, 2021

vlsi Feb 14, 2021

chanseokoh Feb 17, 2021

vlsi Feb 18, 2021

chanseokoh Feb 18, 2021

vlsi Feb 14, 2021

vlsi Feb 14, 2021

vlsi Feb 15, 2021

InLaw commented Jul 20, 2021

TWiStErRob Nov 23, 2022

		task.setStageDirectory(stageExtension.getStagingDirectory());
		task.setDeployExtension(deploy);

		project.afterEvaluate(
		project -> stage.configure(task -> task.setStagingConfig(stageExtension)));

	tasks.withType(Jar.class).named("jar").map(jar -> jar.getArchivePath()));
	tasks.named("jar", Jar.class).map(jar -> jar.getArchivePath()));

Avoid creating and configuring tasks when unnecessary #394

Avoid creating and configuring tasks when unnecessary #394

Conversation

chanseokoh commented Feb 5, 2021

chanseokoh commented Feb 5, 2021

codecov bot commented Feb 10, 2021 • edited Loading

Codecov Report

chanseokoh commented Feb 10, 2021 • edited Loading

chanseokoh Feb 12, 2021 • 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

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

InLaw commented Jul 20, 2021

Choose a reason for hiding this comment

codecov bot commented Feb 10, 2021 •

edited

Loading

chanseokoh commented Feb 10, 2021 •

edited

Loading

chanseokoh Feb 12, 2021 •

edited

Loading