Replace assemble task dependency with runtimeClasspath #2247

seanabraham · 2020-01-25T02:10:18Z

This introduces a new lifecycle task called jibDependencies (the name can really be anything, that was just what I came up with) that all the jib tasks depend on and it in turn depends on any requisite tasks / configurations. Being a lifecycle task, it doesn't have any task action itself and essentially exists as a convenient hook for other dependencies. In particular, jibDependencies will depend on the war/bootWar tasks when applicable, the jar task when applicable, and always depends on the projects runtimeClasspath configuration.

Gradle allows tasks to depend on FileCollections (rather than just tasks). When doing so Gradle expands the task graph expands to include all the necessary tasks to produce the files listed in the FileCollection. Doing so with the runtimeClasspath configuration means Gradle will run all the tasks to get all the artifacts necessary to run this project, making it more efficient than using the assemble task which will often run slow, and unnecessary tasks. In particular, the main source set's runtimeClasspath's property includes the source set output appended to the project's runtimeClasspath configuration so depending on that obviates the need to run even manually run the classes task. (See https://docs.gradle.org/current/userguide/java_plugin.html#sec:source_set_properties)

If we only want to conditionally depend on the runtime classpath I'm happy to make that change though I assume it's never really going to be a bad thing even in the case of the war based projects.

Also, for what it's worth — jib itself uses the runtimeClasspath to get the list of files to add to the container so it makes sense to use that same collection to set up the task dependencies.

jib/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java

Line 174 in d3615a6

    
           FileCollection allFiles = mainSourceSet.getRuntimeClasspath().filter(File::exists);

There are a few different potential approaches here so I'm happy to make changes to go with a different approach. For example, we don't have to introduce the jibDependencies task but it made the implementation a little cleaner and also made it so that the changes to tests were minimal (some of them make assumptions about the contents of task.getDependsOn() which are no longer true when tasks depend on configurations for example). It might also be useful in the future if there are new dependencies since they can all hook into jibDependencies.

This should resolve #2184

seanabraham · 2020-01-25T02:11:59Z

@googlebot I signed it!

seanabraham · 2020-01-25T02:15:54Z

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java

-      Assert.assertEquals(warTask, taskProviders.get(0).get());
+      TaskProvider<?> jibDependenciesTaskProvider =
+          (TaskProvider<?>) tasks.getByPath(taskName).getDependsOn().iterator().next();
+      Assert.assertEquals(jibDependenciesTaskProvider.get(), jibDependenciesTask);


These test changes (and the ones below) are now checking for whether the jibDependencies task depends on the requisite tasks (like the warTask or bootWarTask below) and whether all the jib tasks depend on jibDependencies which is the only task they need to depend on now.

loosebazooka · 2020-01-25T03:36:33Z

Nice, this is the better solution, so I'll close #2246. I'll take a deeper look in the next few days.

loosebazooka

Awesome, just some really minor things, but thanks for doing this!

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java

loosebazooka · 2020-01-25T05:04:19Z

integ tests look good too.

seanabraham · 2020-01-25T23:46:04Z

@loosebazooka thanks for the review! I addressed the comments, and confirmed that the jibDependencies task doesn't show up in the gradlew tasks output. Let me know if there are any other changes you'd like to see.

loosebazooka

We're actually in the middle of prepping a release. We'll have to wait on that before we merge this in.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java

This introduces a new lifecycle task called "jibDependencies" that all the jib tasks depend on and it in turn depends on any requiste tasks / configurations. In particular, jibDependencies will depend on the war/bootWar tasks when applicable, the jar task when applicable and always the runtime configuration. Depending directly on the runtime configuration allows Gradle to add to the task graph appropriately to provide all the artifacts that are specified in that configuration, making it more efficient than using the assemble task.

loosebazooka · 2020-01-31T17:56:10Z

@seanabraham thanks again. I think adding jibDependencies as an explicit task is not something we want to commit to at the moment. Could you just use the previous mechanism. I understand that the plugin logic and tests become more complicated, but the surface of jib exposed to the user is reduced.

loosebazooka · 2020-01-31T17:57:47Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java

+          TaskProvider<Task> warTask = TaskCommon.getWarTaskProvider(projectAfterEvaluation);
+          TaskProvider<Task> bootWarTask =
+              TaskCommon.getBootWarTaskProvider(projectAfterEvaluation);
+          TaskProvider<Task> jibDependenciesTask = tasks.register(DEPENDENCIES_TASK_NAME);


Sorry, after thinking about this while we were doing the release, I realized I'm not quite convinced this is how we want to do this. Maybe in the future... but the problem is going backwards/deprecating something has a high cost.

Can we just use the mechanism used previously: https://github.com/GoogleContainerTools/jib/pull/2247/files#diff-aef8bf567c91df6622aa0ea29ed0c682L175

Sure, I can make that change

seanabraham · 2020-02-03T19:26:47Z

@loosebazooka I refactored to remove the jibDependencies task and instead have each of the jib tasks depend directly on relevant items (war, bootWar, etc. tasks and the runtimeClasspath configuration from the main source set). PTAL

loosebazooka · 2020-02-03T19:35:52Z

Awesome, thanks @seanabraham

This comment has been minimized.

Sign in to view

googlebot added the cla: no label Jan 25, 2020

This comment has been minimized.

Sign in to view

googlebot added cla: yes and removed cla: no labels Jan 25, 2020

seanabraham commented Jan 25, 2020

View reviewed changes

seanabraham requested review from chanseokoh, loosebazooka and TadCordle January 25, 2020 02:24

seanabraham mentioned this pull request Jan 25, 2020

Too many tasks executed on jib build #2184

Closed

loosebazooka mentioned this pull request Jan 25, 2020

only depend on assemble on java-library projects #2246

Closed

loosebazooka reviewed Jan 25, 2020

View reviewed changes

loosebazooka added the kokoro:run label Jan 25, 2020

kokoro-team removed the kokoro:run label Jan 25, 2020

loosebazooka reviewed Jan 26, 2020

View reviewed changes

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java Outdated Show resolved Hide resolved

seanabraham added 5 commits January 26, 2020 16:01

Remove now-useless test

927b8bf

Formatting changes from the googleJavaFormat task

9073d9f

Use method reference style for filter call

a905f2f

Refactor task name into constant

2b7a205

seanabraham force-pushed the sean--fix-gradle-task-dependencies branch from 26649a1 to 2b7a205 Compare January 27, 2020 00:09

chanseokoh added the kokoro:run label Jan 28, 2020

kokoro-team removed the kokoro:run label Jan 28, 2020

seanabraham requested review from chanseokoh and removed request for TadCordle and chanseokoh January 31, 2020 04:58

loosebazooka reviewed Jan 31, 2020

View reviewed changes

Remove jibDependencies task

92b376d

Fix formatting

c2e09c3

loosebazooka added the kokoro:run label Feb 3, 2020

kokoro-team removed the kokoro:run label Feb 3, 2020

loosebazooka approved these changes Feb 5, 2020

View reviewed changes

loosebazooka merged commit 98db391 into GoogleContainerTools:master Feb 5, 2020

loosebazooka mentioned this pull request Feb 11, 2020

Multi-module build with nested references fails #2285

Closed

loosebazooka mentioned this pull request Mar 10, 2020

jib doesn't update images when included Gradle projects change #1745

Closed

chanseokoh mentioned this pull request Feb 23, 2021

Remove dead code #3064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace assemble task dependency with runtimeClasspath #2247

Replace assemble task dependency with runtimeClasspath #2247

seanabraham commented Jan 25, 2020 •

edited

Loading

This comment has been minimized.

seanabraham commented Jan 25, 2020

This comment has been minimized.

seanabraham Jan 25, 2020

loosebazooka commented Jan 25, 2020 •

edited

Loading

loosebazooka left a comment

loosebazooka commented Jan 25, 2020

seanabraham commented Jan 25, 2020

loosebazooka left a comment

loosebazooka commented Jan 31, 2020

loosebazooka Jan 31, 2020 •

edited

Loading

seanabraham Jan 31, 2020

seanabraham commented Feb 3, 2020 •

edited

Loading

loosebazooka commented Feb 3, 2020

Replace assemble task dependency with runtimeClasspath #2247

Replace assemble task dependency with runtimeClasspath #2247

Conversation

seanabraham commented Jan 25, 2020 • edited Loading

This comment has been minimized.

seanabraham commented Jan 25, 2020

This comment has been minimized.

seanabraham Jan 25, 2020

Choose a reason for hiding this comment

loosebazooka commented Jan 25, 2020 • edited Loading

loosebazooka left a comment

Choose a reason for hiding this comment

loosebazooka commented Jan 25, 2020

seanabraham commented Jan 25, 2020

loosebazooka left a comment

Choose a reason for hiding this comment

loosebazooka commented Jan 31, 2020

loosebazooka Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

seanabraham Jan 31, 2020

Choose a reason for hiding this comment

seanabraham commented Feb 3, 2020 • edited Loading

loosebazooka commented Feb 3, 2020

seanabraham commented Jan 25, 2020 •

edited

Loading

loosebazooka commented Jan 25, 2020 •

edited

Loading

loosebazooka Jan 31, 2020 •

edited

Loading

seanabraham commented Feb 3, 2020 •

edited

Loading