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

Add explicit DuplicatesStrategy as required by gradle 7+ (#470) #487

Merged
merged 7 commits into from
Apr 16, 2021

Conversation

tom-haines
Copy link
Contributor

See gradle issue Make it an error to not handle duplicates explicitly: gradle/gradle#15759

Previous Gradle releases used a default 'include' strategy for
copy specs: if a duplicate was found, then Gradle would silently
include the duplicate, potentially overwriting the previous entry.

In Gradle 5, we introduced a warning whenever duplicates are found
and that no explicit deduplication strategy is set. 
** With Gradle 7, this now becomes an error. **

(emphasis added)

In ProtobufExtract.groovy, I've added DuplicatesStrategy.INCLUDE explicitly (which was the previous behaviour of protobuf plugin).

If you run the test (added to travis) without this change:

/gradlew test --tests com.google.protobuf.gradle.ProtobufKotlinDslGradle7PluginTest --stacktrace --info

you can replicate the failure that occurs under gradle 7.

With the change, the gradle 7 issue is remedied. This has no change to existing functionality of the plugin: it just makes it compatible with gradle 7 by declaring the behaviour explicitly.

Copy link
Collaborator

@voidzcy voidzcy left a comment

Choose a reason for hiding this comment

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

The change LGTM, although we may want to reuse existing tests to cover this for Gradle 7.

@tom-haines tom-haines force-pushed the 470-duplicate-strategy-grd7 branch from f08fcba to de7190b Compare April 16, 2021 04:27
*/
@CompileDynamic
class ProtobufKotlinDslCopySpecTest extends Specification {
private static final List<String> GRADLE_VERSIONS = ["5.6", "6.0", "6.7.1", "7.0"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests against full gradle suite

@@ -29,10 +29,10 @@ dependencies {
protobuf files("ext/")
testProtobuf files("lib/protos-test.tar.gz")

compile protobufDep
testCompile 'junit:junit:4.12'
implementation protobufDep
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change allows ProtobufJavaPluginTest suite to run under gradle 7, but issues relating to copy dirs are causing gradle 7 :jar task to fail (failure is in test itself, not connected to plugin). It requires some refactoring to enable 7.0 testing for ProtobufJavaPluginTest

Copy link
Collaborator

Choose a reason for hiding this comment

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

But ProtobufJavaPluginTest doesn't include Gradle 7 now, and ProtobufKotlinDslCopySpecTest is independent. So this change doesn't seem to be necessary now, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not necessary, no. It just means when someone does look at gradle 7, the tests compile, so thought it was better to make the fix preemptively as it's backwards compatible to gradle 5.6.
Should I revert it?

@tom-haines
Copy link
Contributor Author

@voidzcy I've rebased the change with a much lighter test project that tests against gradle 5.6 - 7.0.
I had a look at updating testProjectKotlinDslBase but there are test suite issues connected to the jar task, which looks like it might require some refactoring in order to prevent the merge/fold of the projects from having duplicates (which jar task complains about).

Anyway, please see updated commit, and let me know re any other improvements.

plugins {
java
id("java-library")
id("com.google.protobuf").version("0.8.15")
Copy link
Collaborator

@voidzcy voidzcy Apr 16, 2021

Choose a reason for hiding this comment

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

This doesn't seem right. You shouldn't specify the 0.8.15 version. Otherwise, it's not actually testing the plugin built from the project (tests still pass, which means the change isn't really get covered. Or the Gradle TestKit just ignores the version while still injecting the plugin built from code to the classpath of the test build? I am not sure, try delete this version setting and confirm you are really testing the code instead of the plugin pulled from Maven).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting. This is an interesting one, I will investigate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a sanity check to confirm that the test fails under 0.8.15 - pushed here:
https://github.com/tom-haines/protobuf-gradle-plugin/tree/example-project/prove_test_fail

// If `units` is negative, `nanos` must be negative or zero.
// For example $-1.75 is represented as `units`=-1 and `nanos`=-750,000,000.
int32 nanos = 3;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: leave an empty line at the end of file.

@@ -0,0 +1,8 @@
pluginManagement {

repositories {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be put directly into the repositories{} closure in the buildscript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will do

@@ -29,10 +29,10 @@ dependencies {
protobuf files("ext/")
testProtobuf files("lib/protos-test.tar.gz")

compile protobufDep
testCompile 'junit:junit:4.12'
implementation protobufDep
Copy link
Collaborator

Choose a reason for hiding this comment

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

But ProtobufJavaPluginTest doesn't include Gradle 7 now, and ProtobufKotlinDslCopySpecTest is independent. So this change doesn't seem to be necessary now, does it?

@tom-haines
Copy link
Contributor Author

@voidzcy
To confirm test is effective, see failed travis build 644 re above 78c6183.

Then see travis build 645, which should succeed via commit above cf0419b re-applying fix.

You were right about the classpath / test kit ignoring the version definition. Removed version number in any event.

All identified should be remedied, let me know if anything further.

Copy link
Collaborator

@voidzcy voidzcy left a comment

Choose a reason for hiding this comment

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

LGTM

@voidzcy voidzcy merged commit 76a1922 into google:master Apr 16, 2021
@voidzcy
Copy link
Collaborator

voidzcy commented Apr 16, 2021

Thanks for your contribution!

@tom-haines tom-haines deleted the 470-duplicate-strategy-grd7 branch April 22, 2021 17:45
@ejona86
Copy link
Collaborator

ejona86 commented Jun 3, 2021

I think this was a mistake and should be reverted. Gradle is changing its behavior because if you have duplicate files they may not be identical and then you just get "random" results; you won't actually know what file you are using. Duplicate files indicate there is a problem that needs to be addressed.

I investigated an instance of this in gRPC, and after some poking, it looks like this was happening due to a bug in the plugin in my case. It seems we'll need to fix the bug in the plugin before reverting this. If a user encounters this issue after we fix the bug, then that means their dependencies are accidentally duplicating files and should be addressed.

I'll reopen #470 and document the bug I discovered.

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