Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 30, 2019
+9 −6
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d843cc3
Use Relative Sensitivity for GenerateProtoTask, use name only sensiti…
Jan 11, 2019
608981d
Add Relative Sensitivity to setupExtractProtosTask.
Jan 15, 2019
a8e4250
Use name only sensitivity for setupProtosTask.
Jan 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ package com.google.protobuf.gradle

import com.google.common.base.Preconditions
import com.google.common.collect.ImmutableList

import org.gradle.api.DefaultTask
import org.gradle.api.GradleException
import org.gradle.api.Named
Expand All @@ -43,6 +42,7 @@ import org.gradle.api.internal.file.DefaultSourceDirectorySet
import org.gradle.api.internal.file.FileResolver
import org.gradle.api.internal.file.collections.DefaultDirectoryFileTreeFactory
import org.gradle.api.logging.LogLevel
import org.gradle.api.tasks.PathSensitivity
import org.gradle.api.tasks.SourceSet
import org.gradle.api.tasks.TaskAction
import org.gradle.util.ConfigureUtil
Expand Down Expand Up @@ -336,7 +336,7 @@ public class GenerateProtoTask extends DefaultTask {
includeDirs.add(dir)
// Register all files under the directory as input so that Gradle will check their changes for
// incremental build
inputs.dir(dir)
inputs.dir(dir).withPathSensitivity(PathSensitivity.RELATIVE)
}

/**
Expand All @@ -346,7 +346,7 @@ public class GenerateProtoTask extends DefaultTask {
checkCanConfig()
sourceFiles.from(files)
// Register the files as input so that Gradle will check their changes for incremental build
inputs.files(files)
inputs.files(files).withPathSensitivity(PathSensitivity.RELATIVE)
}

/**
Expand Down
9 changes: 6 additions & 3 deletions src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
package com.google.protobuf.gradle

import com.google.common.collect.ImmutableList

import org.gradle.api.Action
import org.gradle.api.GradleException
import org.gradle.api.Plugin
Expand All @@ -42,6 +41,7 @@ import org.gradle.api.file.SourceDirectorySet
import org.gradle.api.internal.file.FileResolver
import org.gradle.api.logging.LogLevel
import org.gradle.api.plugins.AppliedPlugin
import org.gradle.api.tasks.PathSensitivity
import org.gradle.api.tasks.SourceSet

import javax.inject.Inject
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@zhangkun83 zhangkun83 Jan 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

isTest = Utils.isTest(sourceSetName)
}
}
Expand Down Expand Up @@ -359,6 +360,7 @@ class ProtobufPlugin implements Plugin<Project> {
destDir = getExtractedIncludeProtosDir(sourceSetOrVariantName) as File
inputs.files (compileClasspathConfiguration
?: project.configurations[Utils.getConfigName(sourceSetOrVariantName, 'compile')])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed a long time ago in #366

.withPathSensitivity(PathSensitivity.NAME_ONLY)

// TL; DR: Make protos in 'test' sourceSet able to import protos from the 'main'
// sourceSet. Sub-configurations, e.g., 'testCompile' that extends 'compile', don't
Expand All @@ -379,7 +381,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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

Copy link
Contributor Author

@runningcode runningcode Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

}
isTest = Utils.isTest(sourceSetOrVariantName)
}
Expand Down