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

Remove generate proto task link to compile on #586

Closed
wants to merge 2 commits into from
Closed

Remove generate proto task link to compile on #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a74e97d
Remove generate proto task link to compile on
rougsig Jul 17, 2022
e24e2f2
Add kotlin compile depends on generate proto task
rougsig Jul 17, 2022
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
27 changes: 2 additions & 25 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 @@ -34,7 +34,6 @@ import static java.nio.charset.StandardCharsets.US_ASCII
import com.google.common.base.Preconditions
import com.google.common.collect.ImmutableList
import groovy.transform.CompileStatic
import groovy.transform.PackageScope
import groovy.transform.TypeChecked
import groovy.transform.TypeCheckingMode
import org.gradle.api.Action
Expand All @@ -45,7 +44,6 @@ import org.gradle.api.NamedDomainObjectContainer
import org.gradle.api.file.ConfigurableFileCollection
import org.gradle.api.file.FileCollection
import org.gradle.api.file.ProjectLayout
import org.gradle.api.file.SourceDirectorySet
import org.gradle.api.internal.file.FileResolver
import org.gradle.api.logging.LogLevel
import org.gradle.api.model.ObjectFactory
Expand All @@ -60,13 +58,13 @@ import org.gradle.api.tasks.InputFiles
import org.gradle.api.tasks.Internal
import org.gradle.api.tasks.Nested
import org.gradle.api.tasks.Optional
import org.gradle.api.tasks.OutputDirectory
import org.gradle.api.tasks.OutputFile
import org.gradle.api.tasks.PathSensitive
import org.gradle.api.tasks.PathSensitivity
import org.gradle.api.tasks.SkipWhenEmpty
import org.gradle.api.tasks.SourceSet
import org.gradle.api.tasks.TaskAction
import org.gradle.api.tasks.OutputDirectories

import javax.annotation.Nullable
import javax.inject.Inject
Expand Down Expand Up @@ -258,11 +256,6 @@ public abstract class GenerateProtoTask extends DefaultTask {
this.outputBaseDir = outputBaseDir
}

@OutputDirectory
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you remove this, then you need to add in OutputFiles for the descriptor and for .zip outputs. The way I did that before:

  • For DescriptorSetOptions, make path an @Input and make a new method that returns the non-null output file name. That new method gets @OutputFile.
  • I made PluginOptions non-static which requires passing a factory method to domainObjectContainer for builtins and plugins. Then I made two @Optional methods in PluginOptions, one that was OutputFile and one that was `OutputDirectory. This approach takes some refactoring, but overall seemed cleaner
    • The quicker approach is to add a new method getOutputSourceFiles() that has @OutputFiles and returns the files filtered during getOutputSourceDirectories()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What problem are we trying to solve with this logic? Looks too complicated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All the outputs need to be tracked. If you remove the output base dir as an output, then you must track all the individual files/directories. But getOutputSourceDirectories() doesn't return any of the files. Thus you need some other mechanism to expose the output files. There's multiple ways to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Plugins/Buildins generate files in subfolders. Descriptor is tracked by getDescriptorSetOptionsForCaching. Does anything write files directly to outputBaseDir?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Plugins/Buildins generate files in subfolders

Often. But protoc can also generate a zip or jar file directly. Those outputs are filtered out from getOutputSourceDirectories().

Descriptor is tracked by getDescriptorSetOptionsForCaching

Not entirely. DescriptorSetOptions.path is an @OutputFile, but it is only used if the user overrides the path. If the user uses the default path then that path would be untracked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see comments in the code

protoc is capable of output generated files directly to a JAR file or ZIP archive if the output location ends with .jar/.zip

However, I do not see any documentation of this feature in the protobuf-gradle-plugin. I also do not see any tests for this feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not entirely. DescriptorSetOptions.path is an @OutputFile, but it is only used if the user overrides the path. If the user uses the default path then that path would be untracked.

Sounds like a bug, doesn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see any documentation of this feature in the protobuf-gradle-plugin

All the background I see: #480 and #533 .

Sounds like a bug, doesn't it?

Not on master. If there is an output descriptor, then it is either a child of the base directory or it is the user-configured name. This PR removes the base directory as an output so that introduces a bug.

String getOutputBaseDir() {
return outputBaseDir.get()
}

void setSourceSet(SourceSet sourceSet) {
checkInitializing()
Preconditions.checkState(!isAndroidProject.get(),
Expand Down Expand Up @@ -593,23 +586,7 @@ public abstract class GenerateProtoTask extends DefaultTask {
return "${outputBaseDir.get()}/${plugin.outputSubDir}"
}

/**
* Returns a {@code SourceDirectorySet} representing the generated source
* directories.
*/
@Internal
SourceDirectorySet getOutputSourceDirectorySet() {
String srcSetName = "generate-proto-" + name
SourceDirectorySet srcSet
srcSet = objectFactory.sourceDirectorySet(srcSetName, srcSetName)
srcSet.srcDirs providerFactory.provider {
getOutputSourceDirectories()
}
return srcSet
}

@Internal
@PackageScope
@OutputDirectories
Collection<File> getOutputSourceDirectories() {
Collection<File> srcDirs = []
builtins.each { builtin ->
Expand Down
94 changes: 27 additions & 67 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 @@ -66,11 +66,6 @@ class ProtobufPlugin implements Plugin<Project> {
'android-library',
]

private static final List<String> SUPPORTED_LANGUAGES = [
'java',
'kotlin',
]

private final FileResolver fileResolver
private Project project
private ProtobufExtension protobufExtension
Expand Down Expand Up @@ -122,11 +117,6 @@ class ProtobufPlugin implements Plugin<Project> {
}
}

private static void linkGenerateProtoTasksToTask(Task task, GenerateProtoTask genProtoTask) {
task.dependsOn(genProtoTask)
task.source genProtoTask.getOutputSourceDirectorySet().include("**/*.java", "**/*.kt")
}

private void doApply() {
boolean isAndroid = Utils.isAndroidProject(project)
// Java projects will extract included protos from a 'compileProtoPath'
Expand Down Expand Up @@ -249,7 +239,8 @@ class ProtobufPlugin implements Plugin<Project> {
* Creates Protobuf tasks for a sourceSet in a Java project.
*/
private void addTasksForSourceSet(final SourceSet sourceSet) {
Task generateProtoTask = addGenerateProtoTask(sourceSet.name, [sourceSet])
GenerateProtoTask generateProtoTask = addGenerateProtoTask(sourceSet.name, [sourceSet])
sourceSet.java.srcDirs(generateProtoTask)
generateProtoTask.sourceSet = sourceSet
generateProtoTask.doneInitializing()
generateProtoTask.builtins {
Expand All @@ -267,18 +258,14 @@ class ProtobufPlugin implements Plugin<Project> {
include '**/*.proto'
}
processResourcesTask.dependsOn(extractTask)

SUPPORTED_LANGUAGES.each { String lang ->
linkGenerateProtoTasksToTaskName(sourceSet.getCompileTaskName(lang), generateProtoTask)
}
}

/**
* Creates Protobuf tasks for a variant in an Android project.
*/
private void addTasksForVariant(final Object variant, boolean isTestVariant, Collection<Closure> postConfigure) {
// GenerateProto task, one per variant (compilation unit).
Task generateProtoTask = addGenerateProtoTask(variant.name, variant.sourceSets)
GenerateProtoTask generateProtoTask = addGenerateProtoTask(variant.name, variant.sourceSets)
generateProtoTask.setVariant(variant, isTestVariant)
generateProtoTask.flavors = ImmutableList.copyOf(variant.productFlavors.collect { it.name } )
if (variant.hasProperty('buildType')) {
Expand Down Expand Up @@ -320,21 +307,16 @@ class ProtobufPlugin implements Plugin<Project> {
setupExtractProtosTask(generateProtoTask, it.name)
}
}
if (isTestVariant) {
// unit test variants do not implement registerJavaGeneratingTask
Task javaCompileTask = variant.javaCompileProvider.get()
if (javaCompileTask != null) {
linkGenerateProtoTasksToTask(javaCompileTask, generateProtoTask)
}
} else {
postConfigure.add {
// This cannot be called once task execution has started.
variant.registerJavaGeneratingTask(generateProtoTask, generateProtoTask.getOutputSourceDirectories())
}
}
postConfigure.add {
linkGenerateProtoTasksToTaskName(
Utils.getKotlinAndroidCompileTaskName(project, variant.name), generateProtoTask)
// This cannot be called once task execution has start
variant.registerJavaGeneratingTask(generateProtoTask, generateProtoTask.getOutputSourceDirectories())

// registerJavaGeneratingTask do nothing with kotlin tasks. It's works only with java.
Task kotlinCompileTask = project.tasks.findByName("compile${variant.name.capitalize()}Kotlin")
if (kotlinCompileTask != null) {
kotlinCompileTask.dependsOn(generateProtoTask)
kotlinCompileTask.source(generateProtoTask.getOutputSourceDirectories())
}
}
}

Expand All @@ -348,7 +330,7 @@ class ProtobufPlugin implements Plugin<Project> {
* compiled. For Java it's the sourceSet that sourceSetOrVariantName stands
* for; for Android it's the collection of sourceSets that the variant includes.
*/
private Task addGenerateProtoTask(String sourceSetOrVariantName, Collection<Object> sourceSets) {
private GenerateProtoTask addGenerateProtoTask(String sourceSetOrVariantName, Collection<Object> sourceSets) {
String generateProtoTaskName = 'generate' +
Utils.getSourceSetSubstringForTaskNames(sourceSetOrVariantName) + 'Proto'
Provider<String> outDir = project.providers.provider {
Expand Down Expand Up @@ -452,21 +434,6 @@ class ProtobufPlugin implements Plugin<Project> {
generateTask.addIncludeDir(project.files(extractTask.destDir))
}

private void linkGenerateProtoTasksToTaskName(String compileTaskName, GenerateProtoTask genProtoTask) {
Task compileTask = project.tasks.findByName(compileTaskName)
if (compileTask != null) {
linkGenerateProtoTasksToTask(compileTask, genProtoTask)
} else {
// It is possible for a compile task to not exist yet. For example, if someone applied
// the proto plugin and then later applies the kotlin plugin.
project.tasks.configureEach { Task task ->
if (task.name == compileTaskName) {
linkGenerateProtoTasksToTask(task, genProtoTask)
}
}
}
}

private String getExtractedIncludeProtosDir(String sourceSetName) {
return "${project.buildDir}/extracted-include-protos/${sourceSetName}"
}
Expand All @@ -482,28 +449,21 @@ class ProtobufPlugin implements Plugin<Project> {
// The generated javalite sources have lint issues. This is fixed upstream but
// there is still no release with the fix yet.
// https://github.com/google/protobuf/pull/2823
if (isAndroid) {
// variant.registerJavaGeneratingTask called earlier already registers the generated
// sources for normal variants, but unit test variants work differently and do not
// use registerJavaGeneratingTask. Let's call addJavaSourceFoldersToModel for all tasks
// to ensure all variants (including unit test variants) have sources registered.
project.tasks.withType(GenerateProtoTask).each { GenerateProtoTask generateProtoTask ->
generateProtoTask.variant.addJavaSourceFoldersToModel(generateProtoTask.getOutputSourceDirectories())
}

// TODO(zpencer): add gen sources from cross project GenerateProtoTasks
// This is an uncommon project set up but it is possible.
// We must avoid using private android APIs to find subprojects that the variant depends
// on, such as by walking through
// variant.variantData.variantDependency.compileConfiguration.allDependencies
// Gradle.getTaskGraph().getDependencies() should allow us to walk the task graph,
// but unfortunately that API is @Incubating. We can revisit it when it becomes stable.
// https://docs.gradle.org/4.8/javadoc/org/gradle/api/execution/
// TaskExecutionGraph.html#getDependencies-org.gradle.api.Task-

// TODO(zpencer): find a way to make android studio aware of the .proto files
// Simply adding the .proto dirs via addJavaSourceFoldersToModel does not seem to work.
} else {
// TODO(zpencer): add gen sources from cross project GenerateProtoTasks
// This is an uncommon project set up but it is possible.
// We must avoid using private android APIs to find subprojects that the variant depends
// on, such as by walking through
// variant.variantData.variantDependency.compileConfiguration.allDependencies
// Gradle.getTaskGraph().getDependencies() should allow us to walk the task graph,
// but unfortunately that API is @Incubating. We can revisit it when it becomes stable.
// https://docs.gradle.org/4.8/javadoc/org/gradle/api/execution/
// TaskExecutionGraph.html#getDependencies-org.gradle.api.Task-

// TODO(zpencer): find a way to make android studio aware of the .proto files
// Simply adding the .proto dirs via addJavaSourceFoldersToModel does not seem to work.

if (!isAndroid) {
// Make the proto source dirs known to IDEs
project.sourceSets.each { sourceSet ->
SourceDirectorySet protoSrcDirSet = sourceSet.proto
Expand Down
22 changes: 22 additions & 0 deletions src/main/groovy/com/google/protobuf/gradle/Utils.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,28 @@ class Utils {
}
}

/**
* For each variant, agp creates a unique source set.
* The variant does not have a property to get a unique source set.
* For that reason, map the variant name to the source set name to find a unique source set.
*
* Patterns (variant -> sourceSet):
* <FLAVOUR><BUILD_TYPE> -> <FLAVOUR><BUILD_TYPE>
* <FLAVOUR><BUILD_TYPE>UnitTest -> test<FLAVOUR><BUILD_TYPE>
* <FLAVOUR><BUILD_TYPE>AndroidTest -> androidTest<FLAVOUR><BUILD_TYPE>
*
* @param variantName
* @return
*/
public static String variantNameToSourceSetName(final String variantName) {
if (variantName.endsWith("UnitTest")) {
return "test${variantName.replace("UnitTest", "").capitalize()}"
} else if (variantName.endsWith("AndroidTest")) {
return "androidTest${variantName.replace("AndroidTest", "").capitalize()}"
}
return variantName
}

private static Matcher parseVersionString(String version) {
Matcher matcher = version =~ "(\\d*)\\.(\\d*).*"
if (!matcher || !matcher.matches()) {
Expand Down