Use Relative Sensitivity for GenerateProtoTask, use name only sensitivity for classpath. #293

runningcode · 2019-01-11T01:10:22Z

Using absolute sensitivity does not allow this task output to be re-used
across different machines.

Classpath jars and files can be located in different parts of the
filesystem relative to the current working directory so the task output
is not cacheable unless we use NAME_ONLY sensitivity.

…vity for classpath. Using absolute sensitivity does not allow this task output to be re-used across different machines. Classpath jars and files can be located in different parts of the filesystem relative to the current working directory so the task output is not cacheable unless we use NAME_ONLY sensitivity.

zhangkun83 · 2019-01-15T00:09:39Z

src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy

@@ -379,7 +380,8 @@ class ProtobufPlugin implements Plugin<Project> {
            // 'resources' of the output of 'main', in which the source protos are placed.  This is
            // nicer than the ad-hoc solution that Android has, because it works for any extended
            // configuration, not just 'testCompile'.
-            inputs.files getSourceSets()[sourceSetOrVariantName].compileClasspath
+            inputs.files (getSourceSets()[sourceSetOrVariantName].compileClasspath)
+                    .withPathSensitivity(PathSensitivity.NAME_ONLY)


I suppose this would also be necessary in setupExtractProtosTask()?

There's actually a lot more places this could possibly be added in setupExtractIncludeProtosTask but I was going for a "one thing at a time" PR. :)

zhangkun83 · 2019-01-16T00:34:35Z

src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy

@@ -329,7 +329,8 @@ class ProtobufPlugin implements Plugin<Project> {
        task = project.tasks.create(extractProtosTaskName, ProtobufExtract) {
          description = "Extracts proto files/dependencies specified by 'protobuf' configuration"
          destDir = getExtractedProtosDir(sourceSetName) as File
-          inputs.files project.configurations[Utils.getConfigName(sourceSetName, 'protobuf')]
+          inputs.files(project.configurations[Utils.getConfigName(sourceSetName, 'protobuf')])
+                  .withPathSensitivity(PathSensitivity.RELATIVE)


This is from configuration and presumably also need to be "NAME_ONLY"?

Honestly, I'm not too familiar with the protobuf configuration. I don't think its a general statement that anything from a "cofiguration" should be name only.
The other NAME_ONLY in the setupExtractIncludeProtosTask was the classpath for the compile dependency which means essentially maven jars. These jars may be located in different local maven caches.
On the other hand some configurations may have Kotlin files which require RELATIVE sensitivity but Java files require NAME_ONLY sensitivity because the java compiler ignores the files path and only looks at the package name.

Anyways, I changed this to be NAME_ONLY sensitivity. Let me know if there are any other changes I can make.

I don't think I fully understand how the files from dependencies are considered by the cache. Take a jar file from compile configuration for example, is the jar as a single file or the individual class files from it considered as the input?

I believe a jar from the compile configuration would be taken as a single file and should be considered NAME_ONLY so I guess in this case NAME_ONLY is correct.
I wasn't sure if the compile configuration also included java/kotlin source files. If it doesn't then this is correct.

Sounds reasonable. Do you know how the Gradle cache check if a directory (just a path, not a file tree) is up-to-date? We currently support such usage for the protobuf configuration (#294)

I believe the PathSensitivity works in a similar way whether a FileTree or a Directory are passed to a tasks inputs. The PathSensitivity only applies to the path of the FileTree or the Directory. It doesn't change how the contents of either are cached.

Here is some more information on task relocatability:
https://guides.gradle.org/using-build-cache/#relocatability

runningcode · 2019-01-30T23:13:16Z

Any thoughts on this?
I'm seeing cache misses in our extractIncludeProto and our generateProto tasks.

Happy to change everything to relative to stay on the safer side for now.

zhangkun83 · 2019-01-30T23:20:14Z

@runningcode sorry for dropping the ball. I think this is a pretty low-risk change. Have you tested with your local build to verify that it works?

runningcode · 2019-01-30T23:34:48Z

I agree that I think it's a pretty low risk change. I have tested with a local build and it compiles, but I have not tested remote caching. We only push to our build cache server on CI so I can't verify the changes.

Also, a separate note - when testing - I realized that the GenerateProtoTask depends on a OS specific protoc so looks like the caching for the GenerateProtoTask likely won't be completely fixed with this change, but the ExtractIncludeProto should be fixed!

zhangkun83

OK, let's give it a try then.

runningcode · 2021-11-09T14:33:37Z

src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy

@@ -359,6 +360,7 @@ class ProtobufPlugin implements Plugin<Project> {
          destDir = getExtractedIncludeProtosDir(sourceSetOrVariantName) as File
          inputs.files (compileClasspathConfiguration
            ?: project.configurations[Utils.getConfigName(sourceSetOrVariantName, 'compile')])


should this still be compile or should it be changed to implementation?

It was changed a long time ago in #366

runningcode mentioned this pull request Jan 11, 2019

Only consider relative paths when adding files to task inputs. #288

Closed

runningcode force-pushed the no/relative-sensitivity branch from d656e12 to d843cc3 Compare January 11, 2019 09:45

zhangkun83 reviewed Jan 15, 2019

View reviewed changes

Add Relative Sensitivity to setupExtractProtosTask.

608981d

zhangkun83 reviewed Jan 16, 2019

View reviewed changes

Use name only sensitivity for setupProtosTask.

a8e4250

zhangkun83 approved these changes Jan 30, 2019

View reviewed changes

zhangkun83 merged commit 3b91ef7 into google:master Jan 30, 2019

zhangkun83 mentioned this pull request Feb 20, 2019

Enabling caching for all proto tasks #228

Open

zhangkun83 mentioned this pull request Mar 7, 2019

Do not use deprecated Android Plugin API when new API is available #305

Merged

runningcode deleted the no/relative-sensitivity branch September 17, 2020 17:57

runningcode commented Nov 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Relative Sensitivity for GenerateProtoTask, use name only sensitivity for classpath. #293

Use Relative Sensitivity for GenerateProtoTask, use name only sensitivity for classpath. #293

runningcode commented Jan 11, 2019

zhangkun83 Jan 15, 2019

runningcode Jan 15, 2019

runningcode Jan 15, 2019 •

edited

Loading

zhangkun83 Jan 16, 2019

runningcode Jan 16, 2019

zhangkun83 Jan 16, 2019 •

edited

Loading

runningcode Jan 16, 2019

zhangkun83 Jan 24, 2019

runningcode Jan 24, 2019

runningcode commented Jan 30, 2019

zhangkun83 commented Jan 30, 2019

runningcode commented Jan 30, 2019 •

edited

Loading

zhangkun83 left a comment

runningcode Nov 9, 2021

ejona86 Nov 9, 2021

Use Relative Sensitivity for GenerateProtoTask, use name only sensitivity for classpath. #293

Use Relative Sensitivity for GenerateProtoTask, use name only sensitivity for classpath. #293

Conversation

runningcode commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runningcode Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 Jan 16, 2019 • 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

runningcode commented Jan 30, 2019

zhangkun83 commented Jan 30, 2019

runningcode commented Jan 30, 2019 • edited Loading

zhangkun83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runningcode Jan 15, 2019 •

edited

Loading

zhangkun83 Jan 16, 2019 •

edited

Loading

runningcode commented Jan 30, 2019 •

edited

Loading