Do not use compileClasspath as source of proto files

[rougsig]

Background
a75becd adds sourceSet.compileClasspath as dependency to extractInclude task. compileClasspath depends on compile tasks. That means, if we try to resolve compileClasspath configuration compile tasks will be executed. This creates strange behavior described here #624.

Changes
Do not provide proto files via compileClasspath, use processResources. Proto files were in jar via processResources tasks. Just use it directly, without any proxies.

Not the best solution, I'm working on a better solution here #611

Test plan
Green pipelines.
Manual test on temporalio/sdk-java#1484. (no cycle found, but found licences problems. The same problem as here #625)

---

Fix for #624.

[ejona86]

a75becd adds sourceSet.compileClasspath as dependency to extractInclude task.

Actually, it didn't. That was pre-existing. I just made it more obvious:
a75becd#diff-3405f98848db3cae29f5eda1db42d03fe57eeedc8191b5bf2dd2bbfd08537240L452

I think the recent problems are caused by us adding GenerateProtoTask to the source sets. However, we want to keep that, so we can choose to stop using compileClasspath.

[ejona86]

Why reference ProcessResources instead of the main proto source set?

[rougsig]

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

[ejona86]

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

[rougsig]

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.

[ejona86]

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.

[ejona86]

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.

[rougsig]

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

[ejona86]

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) {

[rougsig]

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.

[rougsig]

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

[rougsig]

I think the recent problems are caused by us adding GenerateProtoTask to the source sets.

This cannot be. GenerateProtoTask is added to sourceSets as input, not output. The problem is in the output of the sourceSet. testCompileClasspath contains the output of mainSourceSet as input.

[rougsig]

Actually, it didn't. That was pre-existing. I just made it more obvious:
a75becd#diff-3405f98848db3cae29f5eda1db42d03fe57eeedc8191b5bf2dd2bbfd08537240L452

I think the recent problems are caused by us adding GenerateProtoTask to the source sets. However, we want to keep that, so we can choose to stop using compileClasspath.

Hm, you're right. It takes a bit more research to find a breaking change commit.

[rougsig]

This cannot be. GenerateProtoTask is added to sourceSets as input, not output. The problem is in the output of the sourceSet. testCompileClasspath contains the output of mainSourceSet as input.

Yes, you was right. The problem can be reproduced after #601. Classes task depends on spotless. Spotless resolves main and test sourceSet at one moment. generateTestProto task resolves main sourceSet. Main sourceSet will be resolved by classes task. Loop found.