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

Jump to bottom

Do not use compileClasspath as source of proto files #631

Merged
merged 3 commits into from
Nov 4, 2022
Merged

Do not use compileClasspath as source of proto files #631

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
ae6f00e
Do not use compileClasspath as source of proto files
rougsig Oct 27, 2022
28fd3fd
Add failing test case for #624
rougsig Oct 27, 2022
e432f3e
Fix codenarc problems
rougsig Oct 28, 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
22 changes: 17 additions & 5 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 @@ -52,6 +52,8 @@ import org.gradle.api.file.SourceDirectorySet
import org.gradle.api.plugins.AppliedPlugin
import org.gradle.api.provider.Provider
import org.gradle.api.tasks.SourceSet
import org.gradle.api.tasks.SourceSetContainer
import org.gradle.api.tasks.TaskProvider
import org.gradle.language.jvm.tasks.ProcessResources
import org.gradle.util.GradleVersion

Expand Down Expand Up @@ -237,12 +239,22 @@ class ProtobufPlugin implements Plugin<Project> {
Configuration compileProtoPath, Collection<Closure> postConfigure) {
Provider<ProtobufExtract> extractProtosTask =
setupExtractProtosTask(sourceSet.name, protobufConfig)
// In Java projects, the compileClasspath of the 'test' sourceSet includes all the
// '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'.

// Pass include proto files from main to test.
// Process resource task contains all source proto files from a proto source set.
FileCollection testClassPathConfig = project.objects.fileCollection()
if (Utils.isTest(sourceSet.name)) {
TaskProvider<ProcessResources> mainProcessResources = project.tasks.named(
project.extensions.getByType(SourceSetContainer)
.getByName(SourceSet.MAIN_SOURCE_SET_NAME)
.processResourcesTaskName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why reference ProcessResources instead of the main proto source set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ProcessResources contains all sources. source code, jars, zips, gradle dependencies, etc. SourceSet contains only sources from source code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, you are also getting the output of extractProtosTask. So the useful parts are coming from how we plumb ProcessResources from the generateProtoTask (which is proto source set + extractProtos):

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

Lines 258 to 262 in e20df5b

project.tasks.named(sourceSet.getTaskName('process', 'resources'), ProcessResources).configure {
it.from(generateProtoTask.get().sourceDirs) { CopySpec cs ->
cs.include '**/*.proto'
}
}

I'm not all that confident that is an equivalent change because the resources will be individual .proto files, not the .jars. I am also not trusting now how people may be modifying gradle tasks.

I can live with this change, but I wonder if it'd be better to just copy from generateProtoTask sourceDirs, the same way as we do for processResources, but into test's generateProtoTask (not extractIncludeProtos). That'd prevent future surprises and seems could be done for Android as well. (We could in the future avoid task configuration for generateProtoTask if we wanted, but I don't know if it really matters.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need pass to test sourceSets from main sourceset: sources, includes. The best way is make singls source of truth. That source should contain all information about sources and includes. In this fix, I did the easiest way.

This fix can be done in the following ways:
1 pass main process Resources task
2 pass main generate proto task
3 pass the combination of, main sourceset and extract tasks.

Copy link
Collaborator

@ejona86 ejona86 Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Two details:

  1. Option (1) has more files in it than (2) and (3). (2) and (3) seem equivalent behaviorally at the moment. I can accept any of the answers here. I think (2) and (3) are more bug prone in terms of omission (forget to add something) but I think (1) is more prone to issues in user's builds and most easy to accidentally misuse. For example, with (1) if you add a .proto to src/main/java, you can include that proto in src/test/proto.
  2. Shouldn't we add these files as input to generateProtoTask, not to excludeIncludeProtos? This PR is doing excludeIncludeProtos. I'm surprised it works with excludeIncludeProtos, since we're passing .proto files and not .jars. This part is more important to me, because I don't understand how the PR is working now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I meant extract, not exclude.

If there are no jars, why are these files being passed to setupExtractIncludeProtosTask()? The purpose of that task is to extract proto files from zips. Yes, they shouldn't be a sourceDir for generateProtoTask, but it seems can be an include.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where it pased to extract task? I think i missed something. Could you please explain a bit more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mainProcessResources is added to testClassPathConfig which is passed:

      Provider<ProtobufExtract> extractIncludeProtosTask = setupExtractIncludeProtosTask(
          sourceSet.name, compileProtoPath, testClassPathConfig) // The last argument here

And setupExtractIncludeProtosTask creates a protobuf extract:

      return project.tasks.register(extractIncludeProtosTaskName, ProtobufExtract) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/google/protobuf-gradle-plugin/blob/master/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy#L132

If we pass directory, it wont be extracted. Just added to result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can pass main sources and extractMainProto as includes to generateTestProto and all will works fine. I did this logic in #632

ProcessResources
)
testClassPathConfig.from(mainProcessResources)
}

Provider<ProtobufExtract> extractIncludeProtosTask = setupExtractIncludeProtosTask(
sourceSet.name, compileProtoPath, sourceSet.compileClasspath)
sourceSet.name, compileProtoPath, testClassPathConfig)
Provider<GenerateProtoTask> generateProtoTask = addGenerateProtoTask(
sourceSet.name, protoSrcDirSet, project.files(extractProtosTask),
extractIncludeProtosTask) {
Expand Down
23 changes: 23 additions & 0 deletions src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,29 @@ class ProtobufJavaPluginTest extends Specification {
gradleVersion << GRADLE_VERSIONS
}

@Unroll
void "generateTestProto should not execute :compileJava task (java-only project) [gradle #gradleVersion]"() {
given: "project from testProject"
File projectDir = ProtobufPluginTestHelper.projectBuilder('testProject')
.copyDirs('testProjectBase', 'testProject')
.build()

when: "build is invoked"
BuildResult result = ProtobufPluginTestHelper.getGradleRunner(
projectDir,
gradleVersion,
"generateTestProto"
).build()

then: "it succeed"
result.task(":generateTestProto").outcome == TaskOutcome.SUCCESS
assert !result.output.contains("Task :classes")
assert !result.output.contains("Task :compileJava")

where:
gradleVersion << GRADLE_VERSIONS
}

@Unroll
void "testProject should be successfully executed (configuration cache) [gradle #gradleVersion]"() {
given: "project from testProject"
Expand Down