Fix input and output annotations #351

lptr · 2019-10-24T17:29:27Z

With Gradle 6.0 we are starting to enforce annotations on all task getters. Non ad-hoc tasks should use annotations in place of the Task.getInputs() and getOutputs() APIs. Due to these changes with Gradle 6.0 the applying the Protobuf plugin results in the following deprecation warnings being displayed:

   > Warning: Type 'GenerateProtoTask': property 'buildType' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'builtins' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'descriptorPath' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'descriptorSetOptions' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'fileResolver' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'flavors' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'generateDescriptorSet' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'isTest' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'isTestVariant' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'outputSourceDirectorySet' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'plugins' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'sourceFiles' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'sourceSet' is not annotated with an input or output annotation.
   > Warning: Type 'GenerateProtoTask': property 'variant' is not annotated with an input or output annotation.
   > Warning: Type 'ProtobufExtract': property 'destDir' is not annotated with an input or output annotation.
   > Warning: Type 'ProtobufExtract': property 'isTest' is not annotated with an input or output annotation.

This PR fixes the problems and the warnings are removed.

I tried to figure out the role of each property as best I could based on the code alone, but I'm not sure I got everything right.

zhangkun83 · 2019-10-24T18:08:12Z

src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

@@ -345,13 +386,12 @@ public class GenerateProtoTask extends DefaultTask {
  public void addSourceFiles(FileCollection files) {
    checkCanConfig()
    sourceFiles.from(files)
-    // Register the files as input so that Gradle will check their changes for incremental build
-    inputs.files(files).withPathSensitivity(PathSensitivity.RELATIVE)


What's the reason for these deletions?

They are now captured by these annotations instead:

https://github.com/google/protobuf-gradle-plugin/pull/351/files#diff-88190c144bef7b71456e5191c71bb2b3R252-R253

To clarify: Gradle queries the annotated getters automatically before running the task to determine up-to-dateness and cache key.

wolfs

The changes are a good first step! I added some comments, my main concern is about backwards compatibility of registering files for ProtobufExtract. If that isn't an issue, then the PR is good as it is.

build.gradle

src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

wolfs · 2019-10-28T12:20:02Z

src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy

  public boolean getIsTest() {
    Preconditions.checkNotNull(isTest)
    return isTest
  }

+  @InputFiles
+  @PathSensitive(PathSensitivity.NAME_ONLY)


I am not sure about the NAME_ONLY. Are you should or is RELATIVE safer?

It seems like the task is copying/extracing from the input files. Maybe it should have a copy-spec instead of plain input files instead?

It also seems like archives can be part of the inputFiles.

For now, using NAME_ONLY does not change the behavior, since that is what the plugin declared before.

Exactly. Let's not change behavior where we don't have to for now.

Please add a TODO here to revisit the correctness of PathSensitivity.

src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy

dcabasson · 2019-10-28T12:59:43Z

Looking forward to having the protobuf plugin leverage all the new features of Gradle 5+.

A good next step will be to make task configuration avoidance work because the plugin in its current form doesn't leverage that at all.

jfbibeau · 2019-10-28T13:27:03Z

Thank you for this PR! Will be nice to get the protobuf plugin more aligned with Gradle best practices 👍

lptr · 2019-10-29T17:46:35Z

@wolfs please take another look!

wolfs

LGTM!

lptr · 2019-11-04T08:48:04Z

Hi, can someone from the Protobuf team take a look at this please?

rpalcolea · 2019-11-07T00:31:02Z

Also #304 is relevant

rpalcolea · 2019-11-07T00:32:08Z

Hi @zhangkun83 , any chance you could look at this PR? also #304 would be nice as we have seen this plugin creating several tasks in configuration phase that might not be needed unless the user intent is to run the proto tasks

zhangkun83 · 2019-11-16T00:23:23Z

src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

@@ -86,6 +99,7 @@ public class GenerateProtoTask extends DefaultTask {
   *
   * Default: false
   */
+  @Internal


"Attached to a task property to indicate that the property is not to be taken into account for up-to-date checking."
It probably should be taken into account for up-to-date checking.

I added a clarifying comment.

zhangkun83 · 2019-11-16T00:26:10Z

src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

+  @Optional
+  @Nested
+  DescriptorSetOptions getDescriptorSetOptions() {
+    return generateDescriptorSet ? descriptorSetOptions : null


If generateDescriptorSet == false and this property is accessed, throwing an exception explaining what went wrong would be more helpful.

I've made this method protected as well and added a comment.

zhangkun83 · 2019-11-16T00:35:23Z

src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

+  }
+
+  @Nested
+  protected Collection<PluginOptions> getPluginsInternal() {


I suppose these two getters for exposing the two containers as properties so that they can be validated, since there public equivalents cannot be called after configuration is finished. A comment to explain that would be helpful.

I added clarifying comments to both the methods and to the @Internal annotations.

zhangkun83 · 2019-11-16T00:39:31Z

src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy

  public boolean getIsTest() {
    Preconditions.checkNotNull(isTest)
    return isTest
  }

+  @InputFiles
+  @PathSensitive(PathSensitivity.NAME_ONLY)


Please add a TODO here to revisit the correctness of PathSensitivity.

lptr · 2019-11-28T08:03:37Z

@zhangkun83 Do you want me to do anything else before this can be merged?

zhangkun83 · 2019-12-03T01:07:56Z

src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

+   */
+  @Optional
+  @Nested
+  protected DescriptorSetOptions getDescriptorSetOptions() {


Why is it not to be called directly? This is the public accessor for DescriptorSetOptions used in the example from README.

It seems that you cannot make it throw when generateDescriptorSet == false as I requested earlier, because Gradle will query this property unconditionally. This is a bit unfortunate as Gradle doesn't have an annotation for "this is public, but don't use it for up-to-date checks".

I'm not sure what you mean by throw when generateDescriptorSet == false. I reverted to the origin public property being @Internal (that's Gradle's annotation for "this is public, but don't use it for up-to-date checks").

If we made accessing descriptorSetOptions throw when generateDescriptorSet == false, the user would get more meaningful error message (see getDescriptorPath()), compared to getting null for descriptorSetOptions and then NPE down the road.

I find @Internal on a public property to be confusing, especially when you add the auxiliary properties like getDescriptorSetOptionsInternal() that also have "Internal" in the name. Can you rename them to *ForUpToDateCheck()?

lptr · 2019-12-11T13:21:36Z

Hi @zhangkun83!

The problem I'd like to address here is that the Protobuf plugin in its currently released form does not declare inputs and outputs for the tasks it creates correctly. I don't want to pressure you into any particular way wrt handling errors etc. Yet we seem to be locked in a cycle of you trying to explain to me how you want things to be which I then implement not to your liking. Considering the long round-trip times I feel this is not leading to the desired results.

Why don't we change this around, so you could add the annotations in a separate PR, and I can help review it? What matters for me is to wrap this up in a timely manner, so the many Gradle users that currently have issues with the plugin can be helped.

I'd like to invite you to the Gradle Community Slack workspace. Given the popularity of your plugin, it would be great if we had a more direct connection. Once we fixed the input/output handling there are other improvements that we'd like to discuss, too. Would you mind sending me your email address to [email protected] so that I can send an invite?

I'd also like to offer help face-to-face if that would work for you. I'm in the CET timezone, we can chat on Google Hangout or somesuch if you want.

zhangkun83 · 2019-12-11T20:17:15Z

@lptr sorry for the slow turnaround. The team have gone through some changes and I have been taking vacations. You idea is good, so please review #364.

@lptr

With Gradle 6.0 we are starting to enforce annotations on all task getters. Non ad-hoc tasks should use annotations in place of the `Task.getInputs()` and `getOutputs()` APIs. Due to these changes with Gradle 6.0 the applying the Protobuf plugin results in the following deprecation warnings being displayed: ```text > Warning: Type 'GenerateProtoTask': property 'buildType' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'builtins' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'descriptorPath' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'descriptorSetOptions' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'fileResolver' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'flavors' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'generateDescriptorSet' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'isTest' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'isTestVariant' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'outputSourceDirectorySet' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'plugins' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'sourceFiles' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'sourceSet' is not annotated with an input or output annotation. > Warning: Type 'GenerateProtoTask': property 'variant' is not annotated with an input or output annotation. > Warning: Type 'ProtobufExtract': property 'destDir' is not annotated with an input or output annotation. > Warning: Type 'ProtobufExtract': property 'isTest' is not annotated with an input or output annotation. ``` This PR fixes the problems and the warnings are removed. This is based on #351 by @lptr

lptr · 2019-12-13T12:13:06Z

This has been taken care of via #364.

lptr added 2 commits October 24, 2019 10:20

Add java-gradle-plugin

cbea434

Add missing annotations

afaa07e

zhangkun83 reviewed Oct 24, 2019

View reviewed changes

lptr added 4 commits October 24, 2019 12:36

Fix tests

c66a121

Use input property instead of Task.inputs to declare inputs

451c4ed

Specify path sensitivity and name for additional property

6d419b1

Set unique name for property

8646253

lptr requested a review from zhangkun83 October 28, 2019 07:28

wolfs suggested changes Oct 28, 2019

View reviewed changes

Address review feedback

8dc04af

wolfs approved these changes Oct 29, 2019

View reviewed changes

voidzcy mentioned this pull request Nov 12, 2019

Gradle 6.0 Deprecations #355

Closed

zhangkun83 reviewed Nov 16, 2019

View reviewed changes

Add comments and TODO

a003ea2

lptr requested a review from zhangkun83 November 18, 2019 12:49

zhangkun83 reviewed Dec 3, 2019

View reviewed changes

Keep descriptorSetOptions publicly available

fa584f0

lptr requested a review from zhangkun83 December 3, 2019 09:46

zhangkun83 mentioned this pull request Dec 11, 2019

Fix input and output annotations #364

Merged

lptr closed this Dec 13, 2019

lptr deleted the gradle/fix-inputs-outputs branch December 13, 2019 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix input and output annotations #351

Fix input and output annotations #351

lptr commented Oct 24, 2019

zhangkun83 Oct 24, 2019

lptr Oct 24, 2019

lptr Oct 24, 2019

wolfs left a comment

wolfs Oct 28, 2019

lptr Oct 29, 2019

zhangkun83 Nov 16, 2019

lptr Nov 18, 2019

dcabasson commented Oct 28, 2019

jfbibeau commented Oct 28, 2019

lptr commented Oct 29, 2019

wolfs left a comment

lptr commented Nov 4, 2019

rpalcolea commented Nov 7, 2019

rpalcolea commented Nov 7, 2019

zhangkun83 Nov 16, 2019

lptr Nov 18, 2019

zhangkun83 Nov 16, 2019

lptr Nov 18, 2019

zhangkun83 Nov 16, 2019

lptr Nov 18, 2019

zhangkun83 Nov 16, 2019

lptr commented Nov 28, 2019

zhangkun83 Dec 3, 2019

lptr Dec 3, 2019

zhangkun83 Dec 10, 2019

lptr commented Dec 11, 2019

zhangkun83 commented Dec 11, 2019

lptr commented Dec 13, 2019

Fix input and output annotations #351

Fix input and output annotations #351

Conversation

lptr commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcabasson commented Oct 28, 2019

jfbibeau commented Oct 28, 2019

lptr commented Oct 29, 2019

wolfs left a comment

Choose a reason for hiding this comment

lptr commented Nov 4, 2019

rpalcolea commented Nov 7, 2019

rpalcolea commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lptr commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lptr commented Dec 11, 2019

zhangkun83 commented Dec 11, 2019

lptr commented Dec 13, 2019