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.

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

Link only generated Java and Kotlin to compilation task by default #375

Merged
merged 6 commits into from
Sep 30, 2020

Conversation

voidzcy
Copy link
Collaborator

@voidzcy voidzcy commented Jan 3, 2020

Related issues: #333, #352

For non Java or Kotlin projects, when users configure with

generateProtoTasks {
    all().each { task ->
        task.builtins {
            remove java
        }
    }
}

that disables Java code generation, they might get an compileJava: no source files error if any Java plugin is applied (could be indirectly applied such as the Scala plugin).

This is because we always link the output of generateProtoTask to compilation task for Java and Kotlin. When generateProtoTask does not generate Java/Kotlin code (such as Scala only), the Java/Kotlin compilation task is configured with source that contains no Java/Kotlin code. This causes the error.

Java and Kotlin are the two primary languages supported by this plugin. Generated Java and Kotlin code are automatically added to compilation tasks. Previously, if user sets the protobufGradlePluginAdditionalLanguages property, it will try to add generated code to compilation task for that language. But this usage hasn't be documented anywhere and would not work for all languages (depends on language specific plugins). Instead, what most non-Java/Kotlin users are doing is to manually configure the sourceSet with generated code for that language, like this example.

With current design, there is no perfect way to automatically add generated code to compilation tasks for all languages without causing no source files errors given that some language plugin is an extended plugin of another language plugin, such as Scala's case. This solution seems to be the most suitable one that works for both Java/Kotlin and non-Java/Kotlin users.

Delete the usage of protobufGradlePluginAdditionalLanguages property. It's never publicly documented, the approach does not work for all languages, and its existence conflicts with this solution.

Added README description for non-Java/Kotlin projects.

Test for non-Java/Kotlin projects is not added (I mainly use this demo project provided by @augi for investigating the issue) as we primarily support Java and Kotlin.

@voidzcy voidzcy force-pushed the bugfix/work_with_non_java_project branch from acfbe45 to de9a514 Compare January 3, 2020 17:54
Copy link
Collaborator

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

How will other languages have task dependencies configured? Setting the sourceSet doesn't mean that the language-specific tasks will depend on the protobuf codegen task.

@voidzcy
Copy link
Collaborator Author

voidzcy commented Mar 11, 2020

How will other languages have task dependencies configured? Setting the sourceSet doesn't mean that the language-specific tasks will depend on the protobuf codegen task.

This was something I was trying to ask for suggestions from you. I was very frustrated when trying to solve this issue. This plugin requires Java or Android plugin to be applied while users use it for non-Java/Kotlin projects (the documentation supports users to do so by removing Java code generation). Most non-Java/Kotlin users (such as this one) just use this plugin to generate the code, they manually add the generated code into sourceSet so that compilation tasks are able to pick them up. Then everything works fine. Previously the plugin adds all generated code as source to the compilation task no matter if the language of generated code matches the compilation task (this causes build fail as it triggers compilation task to run with empty source of that language). And the default two supported languages are Java and Kotlin (although there is a property to allow users add other languages, I've never seen anyone using it and it is not documented anywhere. Also, I am suspecting that would be dangerous as some language plugin (such as C++) is very different and I doubt that can work together with this plugin). So I made the decision to only add generated Java/Kotlin code as source to (Java/Kotlin) compilation tasks by default. All the existing usages for other languages (without remove Java codegen) will still work as they add generated code to sourceSet manually, and they now can optimize their build by removing Java codegen if they don't need it.

An optional improvement to this change is to add Scala to our supported language. Scala plugin is special as it extends Java plugin and its task configurations are similar to Java plugin's (other than C++ plugin, which is very different). But I am not sure the risk if it would break Kotlin and the group of Scala users seem to be small.

@voidzcy voidzcy requested a review from ejona86 July 10, 2020 09:17
README.md Outdated Show resolved Hide resolved
private static void linkGenerateProtoTasksToTask(Task task, GenerateProtoTask genProtoTask) {
task.dependsOn(genProtoTask)
task.source genProtoTask.getOutputSourceDirectorySet()
task.source genProtoTask.getOutputSourceDirectorySet().include("**/*.java", "**/*.kt")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this fix the "no source files" error?

Copy link
Collaborator Author

@voidzcy voidzcy Jul 15, 2020

Choose a reason for hiding this comment

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

Applying the Java/Kotlin filter can avoid feeding non-Java/Kotlin source files to Java compilation task, which avoids task input change that triggers the task to run. If there are no Java/Kotlin source files, Java compilation tasks should not run (you should be able to see something like compileJava NO-SOURCE).

Copy link
Collaborator

@ejona86 ejona86 Jul 15, 2020

Choose a reason for hiding this comment

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

We put each language in its own folder. Why don't we put each language in its own SourceDirectorySet?

Copy link
Collaborator Author

@voidzcy voidzcy Jul 15, 2020

Choose a reason for hiding this comment

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

Each SourceSet can have multiple compile tasks as for different languages (see SourceSet's getComileTaskName(lang) API). However, each SourceSet has only one GenerateProtoTask, which generates code for all languages configured. So for each SourceSet, we are hooking the output of its GenerateProtoTask as the input to each of its compile tasks (for each language). The output of a GenerateProtoTask is a single SourceDirectorySet that contains generated code for plugin/builtin in separated directories. We are not able to map plugins/builtins to individual languages as they do not contain language information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants