Use separate configuration for extracting included proto from dependencies #366
Conversation
…b uses java-library plugin.
…ath, work around for Android.
…ugin into feature/java_library_plugin_compatibility
| project.configurations.create(compileProtoConfigName) { | ||
| visible = false | ||
| transitive = true | ||
| extendsFrom = [project.configurations.findByName(compileClasspathConfigName)] |
There was a problem hiding this comment.
I had suggested extending compileOnly+implementation, to mirror compileClasspath. I guess this way works too. Is there any benefit to either approach, or is it basically all the same?
There was a problem hiding this comment.
Basically the same. I was thinking about compileClasspath is a "more extended" configuration, we would be less likely to miss out something. Do you think which approach is more reasonable?
There was a problem hiding this comment.
That "more extended" is what bothers me. I don't really like to be using things that I don't know I'm using, as someone could end up depending on them. But I see danger from both options. I'm most worried about all the strange things a user may change.
There was a problem hiding this comment.
Alright, let's be conservative and mirror how Java Base Plugin's tasks getting dependencies. I changed it to extend from compileOnly and implementation.
| // not exposing resources to consumers for compilation. | ||
| // Some Android sourceSet does not have 'compileClasspath' configuration, not even | ||
| // 'compile' or 'implementation'. | ||
| String compileProtoConfigName = Utils.getConfigName(sourceSetName, 'compileProto') |
There was a problem hiding this comment.
As I mentioned earlier, maybe we could name it compileProtoPath or compileProtoInclude? Doesn't matter much though.
There was a problem hiding this comment.
Sure, I had a hard time remembering the name you suggested 😢 when I was writing this ...
Configuration hierarchy for
javaandjava-libraryplugins are different:javaplugin:compile(deprecated) is the root configuration,implementationextends it andcompileClasspathextendsimplementation.java-library:apiis the root configuration,implementationextends it andcompileClasspathextendsimplementation.Currently, our plugin extracts included proto for compilation from dependencies in
compileconfiguration. This is wrong even forjavaplugin. Dependencies added toimplementationconfiguration will not be captured by our plugin. If your proto file imports some dependent proto from animplementationconfiguration, you will get an import proto file not found error. Plugin tasks should get dependencies fromcompileClasspathconfiguration, which inherits all dependencies added to its ancestor configurations. Thus, the plugin is able to capture all dependencies for compilation.However, extracting included proto from dependencies in
compileClasspathconfiguration is not enough if some dependency usesjava-libraryplugin. Projects depending on some upstream libraries that usejava-libraryplugin will only pull in classes folder instead of the full JAR for compilation. So proto files in the upstream libraries will not be extracted and consumer project's compilation will fail if it imports proto files from upstream libraries.Our solution, as explained in #363 (comment) is to create an internal configuration that mirrors
compileClasspathconfiguration (which extendscompileOnlyandimplementation) introduced byjava/java-libraryplugin and add an attribute to also includeresources(where the proto files are packaged). We are trying to avoid affectingjava/java-libraryby modifyingcompileClasspathconfiguration itself. This new configuration is for our plugin's internal usage and users should not directly add dependencies to it.Related issues: #363 (most clearly reveals the issue), #242, #338, #258